Fix handling of `--output-format json` flag #82497

jyn514 · 2021-02-24T21:25:22Z

Don't treat it as deprecated on stable and beta channels. Before, it
would give confusing and incorrect output:

warning: the 'output-format' flag is considered deprecated
  |
  = warning: see issue #44136 <https://github.com/rust-lang/rust/issues/44136> for more information

error: json output format isn't supported for doc generation

Both of those are wrong: output-format isn't deprecated, and json
output is supported.

Require -Z unstable-options for --output-format json

Previously, it was allowed by default on nightly, which made it hard
to realize the flag wouldn't be accepted on beta or stable.

To get the test working I had to remove -Z unstable-options, which x.py passed to compiletest unconditionally. It was first added in 8c2ec68 so -Z miri would be allowed. -Z miri is no longer passed unconditionally, so hopefully removing it won't break anything.

r? @aDotInTheVoid cc @HeroicKatora @CraftSpider

Thanks to @memoryruins for pointing it out on Discord!

cc @Mark-Simulacrum for the change to compiletest.

src/librustdoc/config.rs

Nemo157 · 2021-02-24T21:43:20Z

This can probably be simplified now too:

https://github.com/rust-lang/rust/blob/134bbac7dd7ac3196c6cd673bd89e5158c17d88e/src/librustdoc/config.rs#L547-L552

jyn514 · 2021-02-24T21:53:03Z

This can probably be simplified now too:

This is the sort of thing that makes me wish we had a way to test channel differences :/

jyn514 · 2021-02-24T22:44:26Z

I think I'll wait to fix the test errors until I hear from @Mark-Simulacrum about whether it's ok to remove -Z unstable-options from compiletest.

Mark-Simulacrum · 2021-02-25T01:18:28Z

I'm guessing things will need manual application of unstable-options, but that seems good. I'm not sure I understand the -Zmiri claim - -Z flags shouldn't also need unstable options passed I think?

But change seems fine to me, presuming you do the legwork to get CI to like it.

jyn514 · 2021-02-25T01:28:34Z

I'm not sure I understand the -Zmiri claim - -Z flags shouldn't also need unstable options passed I think?

Hmm, that sounds reasonable, but I can't imagine any other reason the flag would have been added. I don't think it's super important to know, though.

aDotInTheVoid · 2021-02-25T10:15:21Z

src/bootstrap/test.rs

@@ -1131,7 +1131,6 @@ note: if you're sure you want to do this, please open an issue as to why. In the
            }
        }
        flags.push(format!("-Cdebuginfo={}", builder.config.rust_debuginfo_level_tests));
-        flags.push("-Zunstable-options".to_string());


Why was this changed? It seems like the most likely candidate for the CI failing.

See the PR description:

To get the test working I had to remove -Z unstable-options, which x.py passed to compiletest unconditionally. It was first added in 8c2ec68 so -Z miri would be allowed. -Z miri is no longer passed unconditionally, so hopefully removing it won't break anything.

That caused a lot more breakage than I expected so I might remove the test for now :/ or wait to merge this until I can make a separate PR removing unstable-options.

That caused a lot more breakage than I expected so I might remove the test for now :/ or wait to merge this until I can make a separate PR removing unstable-options.

I removed the test for now.

src/librustdoc/config.rs

aDotInTheVoid · 2021-02-25T10:38:59Z

Other than CI failing and linking to the tracking issue, this looks good to me. However I dont have r+ rights, so you'll need someone else to review and approve it.

jyn514 · 2021-04-06T20:16:57Z

@aDotInTheVoid are you comfortable with landing this without a test for missing -Z unstable-options? Or do you want me to add a run-make test?

jyn514 · 2021-04-06T20:17:16Z

Oh err I think they said they're busy with school since a few days ago.

r? @CraftSpider

CraftSpider

Left a question on one thing. I would feel a little more comfortable with a test, but don't think it's completely necessary.

CraftSpider · 2021-04-06T20:48:31Z

src/test/rustdoc-ui/feature-gate-output-format.stderr

@@ -0,0 +1,2 @@
+error: the -Z unstable-options flag must be passed to enable --output-format for documentation generation


This appears to be an expected output without the associated test, I'm guessing the one you removed due to issues. should the expected output also get removed, as I don't think this actually does anything?

Oops, good catch. I removed it and added a run-make test :)

- Don't treat it as deprecated on stable and beta channels. Before, it would give confusing and incorrect output: ``` warning: the 'output-format' flag is considered deprecated | = warning: see issue rust-lang#44136 <rust-lang#44136> for more information error: json output format isn't supported for doc generation ``` Both of those are wrong: output-format isn't deprecated, and json output is supported. - Require -Z unstable-options for `--output-format json` Previously, it was allowed by default on nightly, which made it hard to realize the flag wouldn't be accepted on beta or stable. Note that this still allows `--output-format html`, which has been stable since 1.0. - Remove unnecessary double-checking of the feature gate when parsing the output format - Add custom run-make test since compiletest passes -Zunstable-options by default

CraftSpider · 2021-04-07T14:52:22Z

@bors r+

bors · 2021-04-07T14:52:24Z

📌 Commit ffd7094 has been approved by CraftSpider

Fix handling of `--output-format json` flag - Don't treat it as deprecated on stable and beta channels. Before, it would give confusing and incorrect output: ``` warning: the 'output-format' flag is considered deprecated | = warning: see issue rust-lang#44136 <rust-lang#44136> for more information error: json output format isn't supported for doc generation ``` Both of those are wrong: output-format isn't deprecated, and json output is supported. - Require -Z unstable-options for `--output-format json` Previously, it was allowed by default on nightly, which made it hard to realize the flag wouldn't be accepted on beta or stable. To get the test working I had to remove `-Z unstable-options`, which x.py passed to compiletest unconditionally. It was first added in rust-lang@8c2ec68 so `-Z miri` would be allowed. -Z miri is no longer passed unconditionally, so hopefully removing it won't break anything. r? `@aDotInTheVoid` cc `@HeroicKatora` `@CraftSpider` Thanks to `@memoryruins` for pointing it out on Discord! cc `@Mark-Simulacrum` for the change to compiletest.

Fix handling of `--output-format json` flag - Don't treat it as deprecated on stable and beta channels. Before, it would give confusing and incorrect output: ``` warning: the 'output-format' flag is considered deprecated | = warning: see issue rust-lang#44136 <rust-lang#44136> for more information error: json output format isn't supported for doc generation ``` Both of those are wrong: output-format isn't deprecated, and json output is supported. - Require -Z unstable-options for `--output-format json` Previously, it was allowed by default on nightly, which made it hard to realize the flag wouldn't be accepted on beta or stable. To get the test working I had to remove `-Z unstable-options`, which x.py passed to compiletest unconditionally. It was first added in rust-lang@8c2ec68 so `-Z miri` would be allowed. -Z miri is no longer passed unconditionally, so hopefully removing it won't break anything. r? ``@aDotInTheVoid`` cc ``@HeroicKatora`` ``@CraftSpider`` Thanks to ``@memoryruins`` for pointing it out on Discord! cc ``@Mark-Simulacrum`` for the change to compiletest.

Rollup of 5 pull requests Successful merges: - rust-lang#82497 (Fix handling of `--output-format json` flag) - rust-lang#83689 (Add more info for common trait resolution and async/await errors) - rust-lang#83952 (Account for `ExprKind::Block` when suggesting .into() and deref) - rust-lang#83965 (Add Debug implementation for hir::intravisit::FnKind) - rust-lang#83974 (Fix outdated crate names in `rustc_interface::callbacks`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2021-04-08T02:43:19Z

⌛ Testing commit ffd7094 with merge 2118526...

Nemo157 reviewed Feb 24, 2021

View reviewed changes

src/librustdoc/config.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jyn514 force-pushed the json branch 2 times, most recently from 28881b3 to a4a9a84 Compare February 24, 2021 21:52

jyn514 force-pushed the json branch from a4a9a84 to 7e65bd4 Compare February 24, 2021 22:04

This comment has been minimized.

Sign in to view

aDotInTheVoid reviewed Feb 25, 2021

View reviewed changes

src/librustdoc/config.rs Outdated Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2021

jyn514 mentioned this pull request Feb 28, 2021

Don't pass -Z unstable-options by default for UI tests #82639

Merged

jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 28, 2021

This comment has been minimized.

Sign in to view

jyn514 force-pushed the json branch 2 times, most recently from 6121f2d to 477aaeb Compare April 6, 2021 20:15

rust-highfive assigned CraftSpider Apr 6, 2021

jyn514 removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 6, 2021

jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2021

CraftSpider reviewed Apr 6, 2021

View reviewed changes

jyn514 force-pushed the json branch from 477aaeb to ffd7094 Compare April 6, 2021 22:41

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 Apr 7, 2021

GuillaumeGomez mentioned this pull request Apr 7, 2021

Rollup of 3 pull requests #83978

Closed

Dylan-DPC-zz mentioned this pull request Apr 7, 2021

Rollup of 5 pull requests #83979

Closed

Dylan-DPC-zz mentioned this pull request Apr 7, 2021

Rollup of 5 pull requests #83986

Merged

bors merged commit cbe3eba into rust-lang:master Apr 8, 2021

rustbot added this to the 1.53.0 milestone Apr 8, 2021

jyn514 deleted the json branch April 8, 2021 02:53

jyn514 mentioned this pull request Apr 8, 2021

rustdoc: Rename --show-coverage to --output-format coverage (and coverage-json) #84012

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of `--output-format json` flag #82497

Fix handling of `--output-format json` flag #82497

jyn514 commented Feb 24, 2021 •

edited

Loading

This comment has been minimized.

Nemo157 commented Feb 24, 2021

jyn514 commented Feb 24, 2021

This comment has been minimized.

jyn514 commented Feb 24, 2021

Mark-Simulacrum commented Feb 25, 2021

jyn514 commented Feb 25, 2021

aDotInTheVoid Feb 25, 2021

jyn514 Feb 25, 2021

jyn514 Apr 6, 2021

aDotInTheVoid commented Feb 25, 2021

This comment has been minimized.

jyn514 commented Apr 6, 2021

jyn514 commented Apr 6, 2021

CraftSpider left a comment

CraftSpider Apr 6, 2021

jyn514 Apr 6, 2021

CraftSpider commented Apr 7, 2021

bors commented Apr 7, 2021

bors commented Apr 8, 2021

		@@ -0,0 +1,2 @@
		error: the -Z unstable-options flag must be passed to enable --output-format for documentation generation

Fix handling of --output-format json flag #82497

Fix handling of --output-format json flag #82497

Conversation

jyn514 commented Feb 24, 2021 • edited Loading

This comment has been minimized.

Nemo157 commented Feb 24, 2021

jyn514 commented Feb 24, 2021

This comment has been minimized.

jyn514 commented Feb 24, 2021

Mark-Simulacrum commented Feb 25, 2021

jyn514 commented Feb 25, 2021

aDotInTheVoid Feb 25, 2021

Choose a reason for hiding this comment

jyn514 Feb 25, 2021

Choose a reason for hiding this comment

jyn514 Apr 6, 2021

Choose a reason for hiding this comment

aDotInTheVoid commented Feb 25, 2021

This comment has been minimized.

jyn514 commented Apr 6, 2021

jyn514 commented Apr 6, 2021

CraftSpider left a comment

Choose a reason for hiding this comment

CraftSpider Apr 6, 2021

Choose a reason for hiding this comment

jyn514 Apr 6, 2021

Choose a reason for hiding this comment

CraftSpider commented Apr 7, 2021

bors commented Apr 7, 2021

bors commented Apr 8, 2021

Fix handling of `--output-format json` flag #82497

Fix handling of `--output-format json` flag #82497

jyn514 commented Feb 24, 2021 •

edited

Loading