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

Add support for customizing JSON diagnostics from Cargo #7214

Merged
merged 1 commit into from
Aug 7, 2019

Conversation

alexcrichton
Copy link
Member

Cargo has of #7143 enabled pipelined compilation by default which
affects how the compiler is invoked, especially with respect to JSON
messages. This, in some testing, has proven to cause quite a few issues
with rustbuild's current integration with Cargo. This commit is aimed at
adding features to Cargo to solve this issue.

This commit adds the ability to customize the stream of JSON messages
coming from Cargo. The new feature for Cargo is that it now also mirrors
rustc in how you can configure the JSON stream. Multiple
--message-format arguments are now supported and the value specified
is a comma-separated list of directives. In addition to the existing
human, short, and json directives these new directives have been
added:

  • json-render-diagnostics - instructs Cargo to render rustc
    diagnostics and only print out JSON messages for artifacts and Cargo
    things.

  • json-diagnostic-short - indicates that the rendered field of rustc
    diagnostics should use the "short" rendering.

  • json-diagnostic-rendered-ansi - indicates that the rendered field of rustc
    diagnostics should embed ansi color codes.

The first option here, json-render-diagnostics, will be used by
rustbuild unconditionally. Additionally json-diagnostic-short will be
conditionally used based on the input to rustbuild itself.

This should be enough for external tools to customize how Cargo is
invoked and how all kinds of JSON diagnostics get printed, and it's
thought that we can relatively easily tweak this as necessary to extend
it and such.

@rust-highfive
Copy link

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2019
@alexcrichton
Copy link
Member Author

r? @ehuss

I'm curious to get your thoughts on this before proposing we merge, but before merging I think we'll want to do an FCP sign-off since this adds stable functionality

@rust-highfive rust-highfive assigned ehuss and unassigned Eh2406 Aug 5, 2019
@alexcrichton alexcrichton added the T-cargo Team: Cargo label Aug 5, 2019
@Mark-Simulacrum
Copy link
Member

So to clarify json-render-diagnostics will opt-in to color support as per usual (via tty detection and such)?

@alexcrichton
Copy link
Member Author

Correct yes, json-render-diagnostics says "do whatever you would normally do for rustc but emit json blobs for Cargo things and non-diagnostics related things from rustc"

Also for comparison, these are the rustbuild changes necessary to leverage this feature.

@alexcrichton
Copy link
Member Author

I should also note that I all rustbuild really needs here is json-render-diagnostics, and we could arguably remove all the other support and/or just gate this behind a -Z flag for now.

@ehuss
Copy link
Contributor

ehuss commented Aug 5, 2019

This looks good to me. I think the flag options here seem good. It is conceivable they will be useful to other people.

There are a few test failures that need to be addressed.

If we do an fcp, it needs to be done quickly. There is not much time before beta branch, and it may take a while to update. Or stage0 promotion can be delayed, I'm not sure how much people care about that.

Cargo has of rust-lang#7143 enabled pipelined compilation by default which
affects how the compiler is invoked, especially with respect to JSON
messages. This, in some testing, has proven to cause quite a few issues
with rustbuild's current integration with Cargo. This commit is aimed at
adding features to Cargo to solve this issue.

This commit adds the ability to customize the stream of JSON messages
coming from Cargo. The new feature for Cargo is that it now also mirrors
rustc in how you can configure the JSON stream. Multiple
`--message-format` arguments are now supported and the value specified
is a comma-separated list of directives. In addition to the existing
`human`, `short`, and `json` directives these new directives have been
added:

* `json-render-diagnostics` - instructs Cargo to render rustc
  diagnostics and only print out JSON messages for artifacts and Cargo
  things.

* `json-diagnostic-short` - indicates that the `rendered` field of rustc
  diagnostics should use the "short" rendering.

* `json-diagnostic-rendered-ansi` - indicates that the `rendered` field of rustc
  diagnostics should embed ansi color codes.

The first option here, `json-render-diagnostics`, will be used by
rustbuild unconditionally. Additionally `json-diagnostic-short` will be
conditionally used based on the input to rustbuild itself.

This should be enough for external tools to customize how Cargo is
invoked and how all kinds of JSON diagnostics get printed, and it's
thought that we can relatively easily tweak this as necessary to extend
it and such.
@alexcrichton
Copy link
Member Author

Ok!

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 5, 2019

Team member @alexcrichton 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 An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 5, 2019
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 7, 2019

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

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Aug 7, 2019
@ehuss
Copy link
Contributor

ehuss commented Aug 7, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 7, 2019

📌 Commit 45699e9 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2019
@bors
Copy link
Collaborator

bors commented Aug 7, 2019

⌛ Testing commit 45699e9 with merge 42a8c0a...

bors added a commit that referenced this pull request Aug 7, 2019
Add support for customizing JSON diagnostics from Cargo

Cargo has of #7143 enabled pipelined compilation by default which
affects how the compiler is invoked, especially with respect to JSON
messages. This, in some testing, has proven to cause quite a few issues
with rustbuild's current integration with Cargo. This commit is aimed at
adding features to Cargo to solve this issue.

This commit adds the ability to customize the stream of JSON messages
coming from Cargo. The new feature for Cargo is that it now also mirrors
rustc in how you can configure the JSON stream. Multiple
`--message-format` arguments are now supported and the value specified
is a comma-separated list of directives. In addition to the existing
`human`, `short`, and `json` directives these new directives have been
added:

* `json-render-diagnostics` - instructs Cargo to render rustc
  diagnostics and only print out JSON messages for artifacts and Cargo
  things.

* `json-diagnostic-short` - indicates that the `rendered` field of rustc
  diagnostics should use the "short" rendering.

* `json-diagnostic-rendered-ansi` - indicates that the `rendered` field of rustc
  diagnostics should embed ansi color codes.

The first option here, `json-render-diagnostics`, will be used by
rustbuild unconditionally. Additionally `json-diagnostic-short` will be
conditionally used based on the input to rustbuild itself.

This should be enough for external tools to customize how Cargo is
invoked and how all kinds of JSON diagnostics get printed, and it's
thought that we can relatively easily tweak this as necessary to extend
it and such.
@bors
Copy link
Collaborator

bors commented Aug 7, 2019

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 42a8c0a to master...

@bors bors merged commit 45699e9 into rust-lang:master Aug 7, 2019
bors added a commit to rust-lang/rust that referenced this pull request Aug 8, 2019
Update cargo

9 commits in 26092da337b948719549cd5ed3d1051fd847afd7..42a8c0adf91323c01228268c651aef5366b25b69
2019-07-31 23:24:32 +0000 to 2019-08-07 20:41:07 +0000
- Add support for customizing JSON diagnostics from Cargo (rust-lang/cargo#7214)
- Bump rustfix (rust-lang/cargo#7221)
- Fix remap-path-prefix from failing. (rust-lang/cargo#7219)
- Clean up build script stuff and documentation. (rust-lang/cargo#7215)
- Remove debug panic in package-features. (rust-lang/cargo#7213)
- Fix an old test. (rust-lang/cargo#7208)
- Remove unused AstBuilder (rust-lang/cargo#7210)
- Revert "Release a jobserver token while locking a file" (rust-lang/cargo#7201)
- improve error message for unmatched prerelease dependencies (rust-lang/cargo#7191)
@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Aug 17, 2019
@alexcrichton alexcrichton deleted the json-diagnostics branch September 16, 2019 18:24
@ehuss ehuss added this to the 1.38.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants