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

emit feature help in cheat mode (fix nightlies) #36678

Merged
merged 8 commits into from
Sep 27, 2016
Merged

Conversation

TimNN
Copy link
Contributor

@TimNN TimNN commented Sep 23, 2016

This should fix the distcheck failure in the latest nightly.

cc #36539

It's probably not ideal to check the environment that often and the code ist duplicated from librustc/session/config.rs but this was the easiest fix I could think of.

A cleaner solution would probably be to move the unstable_features from Options to ParseSess and change the diag parameter of emit_feature_err to take ParseSess instead of a Handler.

@rust-highfive
Copy link
Collaborator

r? @sfackler

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

@TimNN
Copy link
Contributor Author

TimNN commented Sep 23, 2016

The failure, for the record:

failures:

---- [ui] ui/span/issue-36530.rs stdout ----
    ui: /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/tmp/distcheck/rustc-nightly/src/test/ui/span/issue-36530.rs
normalized stderr:
error: The attribute `foo` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
  --> $DIR/issue-36530.rs:11:1
   |
11 | #[foo]
   | ^^^^^^

error: The attribute `foo` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
  --> $DIR/issue-36530.rs:13:5
   |
13 |     #![foo]
   |     ^^^^^^^

error: aborting due to 2 previous errors



expected stderr:
error: The attribute `foo` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
  --> $DIR/issue-36530.rs:11:1
   |
11 | #[foo]
   | ^^^^^^
   |
   = help: add #![feature(custom_attribute)] to the crate attributes to enable

error: The attribute `foo` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
  --> $DIR/issue-36530.rs:13:5
   |
13 |     #![foo]
   |     ^^^^^^^
   |
   = help: add #![feature(custom_attribute)] to the crate attributes to enable

error: aborting due to 2 previous errors

@brson
Copy link
Contributor

brson commented Sep 24, 2016

Whew, it took me a while to understand what was going on here:

  • We only want to emit the warning on beta/stable
  • CFG_DISABLE_UNSTABLE_FEATURES indicates a compiler built for beta/stable
  • distcheck is always run as a stable build (this is a recent change, and one that in retrospect is incorrect, but that's besides the point).
  • the test suite unconditionally expects the nightly version of compiler output
  • feature_gate decides whether to emit the help message based on which channel it's compiled for
  • so during distcheck the compiler thinks it's on the stable channel and emits messages that don't satisfy the strict ui test

So this fixes it by changing feature_gate to emit the help any time the compiler is instructed to disable feature gating, meaning when the bootstrap key is set during the stable build, as is happening here in distcheck. In isolation I think this is a reasonable thing to do, so we should do it to fix this problem.

We don't strictly need to duplicate the get_unstable_features_setting logic here though because the value produced by that function is already passed into maybe_stage_features as the unstable variable. So the proper thing to do is snake that variable through the entire pass. I think that can be done by storing it in Context and GatedCfg; but unfortunately to get into GatedCfg will require snaking it through syntax::attr and syntax::config as well. It doesn't look that hard, but still it's work. Do you mind trying that?

Now, I don't think this error should have happened at all in distcheck, and that's why I said earlier that "distcheck running as a stable build is ... in retrospect incorrect". We started doing that in this PR. Anytime the build happens from the tarball --release-channel is set to "stable". After seeing this failure I think that previous commit was bogus. The reason being, if somebody downloads our nightly tarballs and makes a build they are going to get a compiler that claims to be a stable compiler, and that's just not true.

But that doesn't invalidate the need for this patch. Even if we were to revert that patch we would still hit this problem when doing the beta/stable builds down the road - the compiler needs to always produce consistent results when running the test suite, and at the moment that means always pretending it's a "nightly" compiler. I'll file a separate bug about this.

@brson
Copy link
Contributor

brson commented Sep 24, 2016

Here's the bug about the tarball config being incorrect: #36690

@TimNN
Copy link
Contributor Author

TimNN commented Sep 24, 2016

Ah, sorry for not explaining the issue / fix a bit more.

We don't strictly need to duplicate the get_unstable_features_setting logic here though because the value produced by that function is already passed into maybe_stage_features as the unstable variable. So the proper thing to do is snake that variable through the entire pass. I think that can be done by storing it in Context and GatedCfg; but unfortunately to get into GatedCfg will require snaking it through syntax::attr and syntax::config as well. It doesn't look that hard, but still it's work. Do you mind trying that?

I don't mind trying that (and event though about it but didn't have the time to that). Also I think passing through a ParseSess is better since emit_feature_err is called from about a dozen places all over the compiler. The only place where a ParseSess is not immediately available (from a quick glance) is the mentioned Context, however snaking it through to there doesn't look difficult.

I can likely do this this afternoon / evening (CEST).

@TimNN
Copy link
Contributor Author

TimNN commented Sep 24, 2016

@brson: I updated the PR by passing a ParseSess to emit_feature_err and adding an unstable_features field to the ParseSess.

This also includes some UnstableFeatures related refactorings to avoid code duplication.

@bors
Copy link
Contributor

bors commented Sep 25, 2016

☔ The latest upstream changes (presumably #36616) made this pull request unmergeable. Please resolve the merge conflicts.

@TimNN
Copy link
Contributor Author

TimNN commented Sep 26, 2016

Rebased.

@alexcrichton
Copy link
Member

@bors: r+ p=1 (looking to fix nightlies)

Thanks @TimNN! Patch looks good to me, and I'm gonna just assume that @brson's comments were addressed. @brson if you still have comments though on this PR feel free to r-!

@bors
Copy link
Contributor

bors commented Sep 26, 2016

📌 Commit f0e1738 has been approved by alexcrichton

@brson
Copy link
Contributor

brson commented Sep 26, 2016

sgtm

@bors
Copy link
Contributor

bors commented Sep 27, 2016

⌛ Testing commit f0e1738 with merge d0623cf...

bors added a commit that referenced this pull request Sep 27, 2016
emit feature help in cheat mode (fix nightlies)

This should fix the `distcheck` failure in the latest nightly.

cc #36539

It's probably not ideal to check the environment that often and the code ist duplicated from `librustc/session/config.rs` but this was the easiest fix I could think of.

A cleaner solution would probably be to move the `unstable_features` from `Options` to `ParseSess` and change the `diag` parameter of `emit_feature_err` to take `ParseSess` instead of a `Handler`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants