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

Bootstrap key logic is too strict #36548

Closed
brson opened this issue Sep 16, 2016 · 11 comments
Closed

Bootstrap key logic is too strict #36548

brson opened this issue Sep 16, 2016 · 11 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@brson
Copy link
Contributor

brson commented Sep 16, 2016

Right now the bootstrap key is a hash of CFG_RELEASE (and also CFG_EXTRA_FILENAME if that's set). CFG_RELEASE contains the channel label and the prerelease version. So today the master compiler will build with a compiler called 1.12.0-beta.1 and that's it.

One place where this is problematic is when trying to build master from a local copy of the stable release. To do this you need to bootstrap beta, then bootstrap master. But today beta is 1.12.0-beta.3 - and it can't bootstrap master because they keycheck fails. Furthermore, if you were to build beta with --release-channel=stable you again create a mismatched bootstrap key.

This is frustrating. The logic is too strict for no particular reason. The scheme for deriving the bootstrap key is quite trivial now, and nobody is abusing it. We may just want to turn on the bootstrap whenever the env var is set at all and not worry about this any more.

cc @alexcrichton this caused @glandium quite a bit of pain recently and I think we should solve it relatively soon.

@brson brson added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-build labels Sep 16, 2016
@brson
Copy link
Contributor Author

brson commented Sep 16, 2016

Hm in light of #36544, mysterious errors when bootstrapping with the wrong compiler, I think we should be defensive about which compiler we bootstrap from. So actually I think we should loosen the value of the key, but we might also put checks in the build system that the compiler has a version number with a chance of building stage0 correctly. Probably just a warning, e.g. rustbuild could do a semver comparison of the stage0 compiler and warn if it's old.

@nagisa
Copy link
Member

nagisa commented Sep 16, 2016

cough #32812 cough

@brson
Copy link
Contributor Author

brson commented Sep 16, 2016

@nagisa That looks about right. Nice.

@nagisa
Copy link
Member

nagisa commented Sep 16, 2016

@alexcrichton @nrc you both expressed negativeness towards the PR linked 2 comments above. Does/would your opinion change in the light of this issue, or are you still convinced that we need a bumpier so called “speed bump”?

@brson
Copy link
Contributor Author

brson commented Sep 16, 2016

Another scheme that would discourage abuse but not be so troublesome: have two random compile-time values, CFG_CURRENT_BOOTSTRAP_KEY, CFG_NEXT_BOOTSTRAP_KEY; the compiler disables gating when either are provided; the build system sets RUSTC_BOOTSTRAP_KEY=CFG_CURRENT_BOOTSTRAP_KEY; every 6 months or so we promote CFG_NEXT_BOOTSTRAP_KEY to CFG_CURRENT_BOOTSTRAP_KEY and redefine CFG_NEXT_BOOTSTRAP_KEY.

So there's a long, overlapping window where either key is valid, and rotating them discourages anyone from getting in the habit of setting them to work around problems. Not very tricky or prone to error, and doesn't require much maintenance.

@glandium
Copy link
Contributor

Another scheme would be to keep the key and have the build system figure out the key from the output of rustc -v -V (assuming all the required info is in there ; it is for stable compilers, I don't know if a beta compiler prints out beta.n).

@nrc
Copy link
Member

nrc commented Sep 18, 2016

I still think that just having a env var will lead to people using unstable features on stable/beta and that would be bad (it is a real slippery slope thing - "I just want this one feature that will be stable soon anyway") and we'll suffer for getting locked in to unstable features. So I still want a speed bump. However, I don't want to cause unnecessary pain, so I'm open to different speedbumps.

@cuviper
Copy link
Member

cuviper commented Sep 20, 2016

Perhaps the hash should be of just the version, and not include the channel? The level of dev/beta/stable is not really relevant here.

For that matter, the RUSTC_BOOTSTRAP_KEY could even be the unhashed version number. That's still a minor speedbump since it changes every release; it just wouldn't be MD5-obfuscated.

@brson
Copy link
Contributor Author

brson commented Sep 21, 2016

Tools team discussed this some and agreed it's fine to just set it to a boolean and see what happens; though @alexcrichton was not present. If people start to abuse it we can tighten it up again. The makefiles will need to continue using the current scheme for the stage0 bootstrap while also supporting the new scheme for --enable-local-rebuild.

@nagisa
Copy link
Member

nagisa commented Sep 23, 2016

To anybody who wants to tackle this bug: taking #32812 and rebasing it on current master is a good starting point. You will have to add some extra logic to pass beta the bootstrap key calculated using current method for a cycle, as beta will be still using the old bootstrap key approach.

@alexcrichton
Copy link
Member

#36548 (comment) sounds reasonable to me. I'd prefer to stick with the current scheme if possible, but if it causes pain unnecessarily it's not worth it.

eddyb added a commit to eddyb/rust that referenced this issue Oct 19, 2016
Allow bootstrapping without a key. Fixes rust-lang#36548

This will make it easier for packagers to bootstrap rustc when they happen
to have a bootstrap compiler with a slightly different version number.

It's not ok for anything other than the build system to set this environment variable.

r? @alexcrichton
eddyb added a commit to eddyb/rust that referenced this issue Oct 19, 2016
Allow bootstrapping without a key. Fixes rust-lang#36548

This will make it easier for packagers to bootstrap rustc when they happen
to have a bootstrap compiler with a slightly different version number.

It's not ok for anything other than the build system to set this environment variable.

r? @alexcrichton
cuviper added a commit to cuviper/rust that referenced this issue Oct 19, 2016
This is a follow-up to pull request rust-lang#32812, according to the new
decision in issue rust-lang#36548 to relax bootstrap key logic.  Now *any*
non-empty bootstrap key is allowed to circumvent the feature gate.

For now, the build system still goes through the same motions to set
RUSTC_BOOTSTRAP_KEY according to the prior system.  This is necessary
at least until the next stage0 base includes this relaxed key check.
It also makes it simpler to revert this if we change our mind.
@bors bors closed this as completed in d3c5905 Oct 19, 2016
brson added a commit to brson/rust that referenced this issue Nov 3, 2016
This will make it easier for packagers to bootstrap rustc when they happen
to have a bootstrap compiler with a slightly different version number.

It's not ok for anything other than the build system to set this environment variable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

6 participants