Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for stable -C emit-artifact-notifications rustc flag #60419

Closed
3 tasks
alexcrichton opened this issue Apr 30, 2019 · 16 comments · Fixed by #62766
Closed
3 tasks

Tracking issue for stable -C emit-artifact-notifications rustc flag #60419

alexcrichton opened this issue Apr 30, 2019 · 16 comments · Fixed by #62766
Labels
A-diagnostics Area: Messages for errors, warnings, and lints B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Apr 30, 2019

First added in #60006 the -Z emit-directives flag is currently intended to be paired with the --error-format=json flag. The emit-directives flag indicates that the compiler will emit a JSON directive for various things it does, currently only for when an rmeta metadata file is produced.

This issue is intended to track stabilizing this flag! The main use case for this is "pipelined compilation" which shows promise for accelerating Cargo's compile of multi-crate graphs.

Current known issues with this flag:

  • It's unclear how Cargo will handle its own --message-format short flag with this. Cargo will want to switch to -Z emit-directives by default but --message-format short in Cargo translates to --error-format short in rustc, which means Cargo can't simultaneously get a directive JSON plus a short error format
  • What should the stable form of this flag be called?
  • What should the structure of the JSON directive be? Currently it only gives a path to the artifact emitted, but should the --emit kind of that artifact also be emitted?

cc @nnethercote

@alexcrichton alexcrichton added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Apr 30, 2019
@ehuss
Copy link
Contributor

ehuss commented Apr 30, 2019

* how Cargo will handle its own `--message-format short`

I'm working on diagnostic caching and have the same problem with the short format. I'm wondering if maybe another flag could be added to make rustc include the rendered short form as well as the human form? It seems a little clunky, so I'm not sure if this is going in the wrong direction. The only other option I can think of is to update the JSON format to have full fidelity so that any form can be generated from the base JSON structure. And then have some crate which can take that format and generate the output in any form (colored, human, short, json, etc.). I think there is a diagnostics wg that is maybe looking at moving things into different crates, though I don't really know what their plans are or what the timeframes are.

@alexcrichton alexcrichton added the A-diagnostics Area: Messages for errors, warnings, and lints label Apr 30, 2019
bors added a commit that referenced this issue May 7, 2019
…ichton

rustc: rename -Z emit-directives to -Z emit-artifact-notifications and simplify the output.

This is my take on #60006 / #60419 (see #60006 (comment)).
I'm not too attached the "notifications" part, it's pretty much bikeshed material.
**EDIT**: for "artifact", @matklad pointed out Cargo already uses it (in #60464 (comment))

The first two commits are fixes that could be landed independently, especially the `compiletest` one, which removes the need for any of the normalization added in #60006 to land the test.

The last commit enables the emission for all outputs, which was my main suggestion for #60006, mostly to show that it's minimal and not really a "scope creep" (as suggested in #60006 (comment)).

cc @alexcrichton @nnethercote
@alexcrichton alexcrichton changed the title Tracking issue for -Z emit-directives Tracking issue for -Z emit-artifact-notification May 20, 2019
@alexcrichton alexcrichton changed the title Tracking issue for -Z emit-artifact-notification Tracking issue for -Z emit-artifact-notifications May 20, 2019
@alexcrichton
Copy link
Member Author

I'd like to propose FCP for this issue for stabilization. A summary of this feature to stabilize would be:

  • A new flag, -C emit-artifact-notifications is added to the compiler
  • When the compiler is otherwise in JSON printing mode, this causes the compiler to print a JSON blob per artifact that it produces.
  • JSON blobs have the form {"artifact":"path/to/the/artifact.rlib","emit":"link"}. The "artifact" key is the path to the file that is produced, and the "emit" key matches the --emit flag in the compiler. It's, for example, metadata for rmeta files, link for linked artifacts, etc.

There's only one other unresolved question with --error-format=short and how Cargo handles this, but I think that's best left to a different issue.

cc @rust-lang/compiler, could I poke one of y'all to propose stabilization of this feature?

@cramertj
Copy link
Member

@rfcbot fcp merge

See above comment for summary. @alexcrichton could you also provide a link to the tests for this feature?

@rfcbot
Copy link

rfcbot commented Jun 14, 2019

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 14, 2019
@eddyb
Copy link
Member

eddyb commented Jun 14, 2019

We could also enable this by default in JSON mode without a flag, to stabilize it.

@alexcrichton
Copy link
Member Author

@cramertj sure yeah, there's a simple smoke test in the repository which asserts the output of the flag, and Cargo thoroughly also excercises this in pipelined compilation so it'd break very quickly if anything went awry.

@eddyb I'd be fine with that as well! I would prefer to stabilize nothing and have Cargo simply use this internally with rustc, but that's not technically feasible right now. So long as Cargo has the option for this functionality I think that's ok.

@Zoxc
Copy link
Contributor

Zoxc commented Jun 15, 2019

I don't think this is a codegen option, so it shouldn't go under -C.

@alexcrichton
Copy link
Member Author

@Zoxc hm I find that comment sort of curt and not super helpful. Do you have a suggestion of where it might go? Do you have a suggestion about simply enabling this by default in --error-format=json mode? It'd be great to expand on your thoughts!

@eddyb
Copy link
Member

eddyb commented Jun 18, 2019

In #60987 (comment) I did mention I'd prefer if all the options related to the compiler's output were maybe grouped, like -C options are, but I didn't have a concrete suggestion.
I suspect @Zoxc might like that, but personally I wouldn't mind if we had the minimum amount of configurability and mostly just kept adding information to the JSON output without a flag.

@nikomatsakis
Copy link
Contributor

I tagged @michaelwoerister as they are on parental leave.

@alexcrichton
Copy link
Member Author

ping @Zoxc, @nagisa, @petrochenkov, @pnkfelix, y'all mind taking a look at this?

@Zoxc
Copy link
Contributor

Zoxc commented Jul 3, 2019

I think it would make sense to have an option which streams requested information to stdout in the form of JSON blobs, something like --json=short-errors,long-errors,artifacts, with each JSON blob indicating what kind of output it is with {"type": "artifact"} or something.

We could add to this list later if cargo, tools or IDEs want some more easily parsable information from rustc.

@alexcrichton
Copy link
Member Author

@Zoxc do you intend to hold off on stabilization of this feature until that issue is resolved? That seems like a nice feature to have, but something we could always add after the fact.

@rfcbot
Copy link

rfcbot commented Jul 11, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 11, 2019
@pnkfelix pnkfelix changed the title Tracking issue for -Z emit-artifact-notifications Tracking issue for stable emit-artifact-notifications rustc flag Jul 12, 2019
@pnkfelix pnkfelix changed the title Tracking issue for stable emit-artifact-notifications rustc flag Tracking issue for stable -C emit-artifact-notifications rustc flag Jul 12, 2019
@alexcrichton
Copy link
Member Author

While we're waiting for FCP to finish, I've proposed an implementation of this stabilization at #62766

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 21, 2019
@rfcbot
Copy link

rfcbot commented Jul 21, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 26, 2019
This commit stabilizes options in the compiler necessary for Cargo to
enable "pipelined compilation" by default. The concept of pipelined
compilation, how it's implemented, and what it means for rustc are
documented in rust-lang#60988. This PR is coupled with a PR against Cargo
(rust-lang/cargo#7143) which updates Cargo's support for pipelined
compliation to rustc, and also enables support by default in Cargo.
(note that the Cargo PR cannot land until this one against rustc lands).

The technical changes performed here were to stabilize the functionality
proposed in rust-lang#60419 and rust-lang#60987, the underlying pieces to enable pipelined
compilation support in Cargo. The issues have had some discussion during
stabilization, but the newly stabilized surface area here is:

* A new `--json` flag was added to the compiler.
* The `--json` flag can be passed multiple times.
* The value of the `--json` flag is a comma-separated list of
  directives.
* The `--json` flag cannot be combined with `--color`
* The `--json` flag must be combined with `--error-format=json`
* The acceptable list of directives to `--json` are:
  * `diagnostic-short` - the `rendered` field of diagnostics will have a
    "short" rendering matching `--error-format=short`
  * `diagnostic-rendered-ansi` - the `rendered` field of diagnostics
    will be colorized with ansi color codes embedded in the string field
  * `artifacts` - JSON blobs will be emitted for artifacts being emitted
    by the compiler

The unstable `-Z emit-artifact-notifications` and `--json-rendered`
flags have also been removed during this commit as well.

Closes rust-lang#60419
Closes rust-lang#60987
Closes rust-lang#60988
bors added a commit that referenced this issue Jul 30, 2019
…r=oli-obk

rustc: Stabilize options for pipelined compilation

This commit stabilizes options in the compiler necessary for Cargo to
enable "pipelined compilation" by default. The concept of pipelined
compilation, how it's implemented, and what it means for rustc are
documented in #60988. This PR is coupled with a PR against Cargo
(rust-lang/cargo#7143) which updates Cargo's support for pipelined
compliation to rustc, and also enables support by default in Cargo.
(note that the Cargo PR cannot land until this one against rustc lands).

The technical changes performed here were to stabilize the functionality
proposed in #60419 and #60987, the underlying pieces to enable pipelined
compilation support in Cargo. The issues have had some discussion during
stabilization, but the newly stabilized surface area here is:

* A new `--json` flag was added to the compiler.
* The `--json` flag can be passed multiple times.
* The value of the `--json` flag is a comma-separated list of
  directives.
* The `--json` flag cannot be combined with `--color`
* The `--json` flag must be combined with `--error-format=json`
* The acceptable list of directives to `--json` are:
  * `diagnostic-short` - the `rendered` field of diagnostics will have a
    "short" rendering matching `--error-format=short`
  * `diagnostic-rendered-ansi` - the `rendered` field of diagnostics
    will be colorized with ansi color codes embedded in the string field
  * `artifacts` - JSON blobs will be emitted for artifacts being emitted
    by the compiler

The unstable `-Z emit-artifact-notifications` and `--json-rendered`
flags have also been removed during this commit as well.

Closes #60419
Closes #60987
Closes #60988
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants