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 unstable `--json-rendered` flag #60987

Open
alexcrichton opened this issue May 20, 2019 · 27 comments

Comments

Projects
None yet
8 participants
@alexcrichton
Copy link
Member

commented May 20, 2019

This issue is intended to be a tracking issue for the --json-rendered flag implemented in #59128. The --json-rendered flag can either take plain or termcolor, and affects whether ANSI color codes are embedded into the rendered field of JSON diagnostics.

Known issues with this:

  • flag not supported in rustdoc yet

Future features:

  • Html output (discussion: #59128 (comment))
  • Short output (equal to --error-format=short)

cc @oli-obk, original author of the feature

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

I'd like to propose that this feature is stabilized in an effort to stabilize pipelined compilation. As a brief summary of how this flag works today, when passed to the compiler the --json-rendered flag will affect the "rendered" field of JSON diagnostics printed by the compiler.

I would propose the following stabilization:

  • Add a new flag, --json-rendered
  • Hide this flag from rustc -h unless the --verbose flag is passed
  • Accept the following values for --json-rendered=value:
    • plain - whatever rustc would print on the console but without colors
    • ansi-colors - whatever rustc would print on the console with ANSI color codes embedded in the string
    • short - whatever rustc would print on the console for --error-format=short, but without colors
    • short-ansi-colors - combination of the two options above

This should be sufficient for all of Cargo's usage, and we'd mirror the option into rustdoc as well as rustc for processing there.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Oh and cc @rust-lang/compiler, could I persuade one of y'all to kick of FCP for stabilization? I wrote a summary above for what I'm proposing to be stabilized here.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I feel like we're accumulating a bunch of flags that are related but not explicitly grouped (like -C flags are), maybe there is a better way to set this up? Other than that I'm on board!

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Instead of having a a non-color flag and a color flag for each variant we can also reuse the --color flag that modifies how non-json output is colored.

@jsgf

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Would it be possible to have an option to include all rendering styles in the case where it is being cached and we don't know how its going to be used or consumed?

Is color vs. non-color just a matter of stripping out ctrl characters?

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@jsgf With the ANSI output, you'd need to have a small parser to find where the escape sequence stops (i.e. it always starts with a \x1b but can be followed by a varying number of characters).
While possible, it might be too much work, so it could be a good idea to have a different field, rendered_ansi_colors which contains the colored version.
This would also be backwards-compatible, if that's a concern.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

@eddyb Cargo already implements ansi parsing and stripping when necessary. It uses the strip_ansi_escapes package for stripping and fwdansi for converting to old-Windows color controls.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

I'm a little concerned about stabilizing this without having a strategy for supporting the caching use case. It needs a single JSON format that can be converted to all other output formats. Currently it works for everything except short messages. I see a few options here:

  1. Support ability to include multiple rendered formats in JSON, which could influence the format of the --json-rendered flag.
  2. Use an external library for rendering messages, and share that between rustc and cargo. This would probably require a new JSON format altogether. Once this is in place, cargo could stop using --json-rendered.

ping @rust-lang/wg-diagnostics. I'm curious what the feasibility of 2 is. Is it on the table as an option? Is it something that can happen within a year?

I also think it's a little strange to have separate color controls from the --color flag.

@jsgf

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@ehuss Yes, that's exactly my concern - I'd like to cache the json output and be able to generate any of the other forms on demand, without having separate implementation of diagnostics rendering.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@rust-lang/wg-diagnostics is working on integrating the annotate-snippet-rs crate into rustc and making it the go-to rendering engine (see #59346). Implementing a json -> rendered scheme is not on the table at the momemnt, but I see nothing that would speak against it. All relevant information should be available.

We could even extend --json-rendered to also have a not/none variant that just produces null for the rendered field (or an empty string) in order to reduce the amount of data in json that you don't need.

So it definitely is doable, but we have our hands full with just using annotate-snippet-rs in the compiler at all.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

@jsgf and @ehuss can y'all explain more why you think that a diagnostic JSON message should contain all possible renderings? I understand the idea of wanting it cached, but why would you want it cached and dynamically switched between builds? This seems like a setting that is rarely flipped and wouldn't really benefit from an A+ user-experience of caching as much as possible.

I would personally be fine interpreting --color always as simply "put ansi codes into the json rendered field". This is largely a means to an end of pipelined compilation, and I wish that nothing had to be stabilized anyway.

To that end could I propose that we simply stabilize the intent of interpreting --color always as "put ansi codes into the json rendered field" and table the question of caching short/long at the same time?

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

There are some use cases where switching outputs is common. For example, my editor uses JSON, whereas when I run cargo manually in a terminal it uses human output. It would be bad if the cached output spit JSON into the terminal. The cache implementation in cargo handles this, except for the "short" case. It would be weird if you asked for "short" output and suddenly got the verbose human output, or vice-versa.

That being said, I wonder how often the short output is used. I suspect it is extremely rare, in which case perhaps leaving it a little broken might be ok.

Thinking about it more, I think my concern can be addressed later and independently. Such as making --json-rendered a comma-separated list of values, or using an external library; neither should affect this, so I'm OK with this proposal.

One nitpick about naming, perhaps use human instead of plain to be consistent with --error-format?

Also, making it use the --color flag might be slightly tricky. Cargo always issues that flag. Cargo will need to be updated to not do that if the user requests the JSON format, and instead only if --color is passed. Otherwise the JSON will suddenly start including color codes for everyone. A minor issue, but needs to be handled if we go that route.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Ah ok I think I see what you mean as well with caching and stale messages. It's a good point as well with --color and being a bit trickier with Cargo but that seems surmountable. I would personally be most in favor of that strategy!

@jsgf

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

In our environment, Buck caches are very persistent and shared among multiple users. A cache entry could have been generated by CI, and then reused as part of a user's manually invoked build, or in a rustfix-style linter build. In other words, the original cached form must be reusable for all these other use-cases.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@jsgrf I'm curious, does Buck normally cache the console output from compilers, or is this something unique for the rust integration?

@jsgf

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@ehuss It doesn't at present, but it's something I'd like to add. I've been writing tooling to auto-apply fix suggestions in error messages, and its very annoying to have to deliberately break caching in order to get diagnostics. Also it falls out of distributed builds (Remote Execution) pretty naturally, because all the output artifacts are transferred via a content store, including diagnostics.

@luser

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@eddyb Cargo already implements ansi parsing and stripping when necessary. It uses the strip_ansi_escapes package for stripping and fwdansi for converting to old-Windows color controls.

FWIW, this use case is the exact reason I wrote the strip-ansi-escapes crate in the first place--so sccache could invoke compilers with --color=always and cache their output, and then replay that output with or without colors depending on the situation.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

Ok in an attempt to prod this again, I'd like to propose that this feature is stabilized in an effort to stabilize pipelined compilation. Given the discussion here I would propose a different stabilization path than the one I mentioned above, namely:

  • If --color always is passed to the compiler and --error-format=json is also specified then the rendered field of the JSON blob for error message will embed ANSI color codes.

That should be all that's necessary for Cargo (and this would be mirrored in rustdoc). No new flags or anything like that, just reinterpreting what we're already given.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@rfcbot fcp merge

I am going to propose that we stabilize @alexcrichton's proposal, where --color=always affects the rendering of JSON output:

If --color=always is passed to the compiler and --error-format=json is also specified then the rendered field of the JSON blob for error message will embed ANSI color codes.

We are going however to need:

  • Implementation
  • Tweaks to the documentation for --color=always
  • Ideally an update to the rustc documentation
@rfcbot

This comment has been minimized.

Copy link

commented Jul 8, 2019

Team member @nikomatsakis 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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@rfcbot fcp cancel

On second though, I'm going to cancel this proposal because I feel like it is mildly backwards incompatible, since --color and JSON output can already be combined.

@rfcbot

This comment has been minimized.

Copy link

commented Jul 8, 2019

@nikomatsakis proposal cancelled.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@rfcbot fcp merge

Instead I am going to propose the we stabilize @alexcrichton's original proposal.

Key points:

There was a short discussion on Zulip between @oli-obk and @Zoxc and I in which we discuss a few mild variations. I would be happy with any of these but I'm going to propose that we go forward with what we have today in the interests of expediency, and because all of these changes seem "backwards compatible" or just not especially important. Still, I think if somebody opened a PR for one of the changes below, we could accept that and still stabilize.

  • Rename to --json instead of --json-rendered
  • Possibly tweak initial values to begin with diagnostic-foo instead of just foo (e.g., diagnostic-plain instead of plain). This would leave room for future values that are unrelated to diagnostics (e.g., to emit json artifacts of another kind).
  • Error if --json-rendered is combined with --color.
@rfcbot

This comment has been minimized.

Copy link

commented Jul 8, 2019

Team member @nikomatsakis 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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

(I tagged @michaelwoerister while they're on PTO)

@rfcbot

This comment has been minimized.

Copy link

commented Jul 15, 2019

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

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 17, 2019

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 rust-lang#60988. This PR is coupled with a PR against Cargo
(rust-lang/cargo#FIXME) 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

Closes rust-lang#60419
Closes rust-lang#60987
Closes rust-lang#60988

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 17, 2019

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 rust-lang#60988. This PR is coupled with a PR against Cargo
(rust-lang/cargo#FIXME) 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

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 17, 2019

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 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

@alexcrichton alexcrichton referenced a pull request that will close this issue Jul 17, 2019

Open

rustc: Stabilize options for pipelined compilation #62766

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

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

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jul 18, 2019

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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.