If local-rust is the same as the current version, then force a local-rebuild #34779
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I use |
How is that supported in any way? AFAIK the only supported versions are "previous release" and "exact same as the source being compiled". cc @rust-lang/tools |
|
If you're setting the The local-rebuild option also affects how unstable features are used internally. If you don't use this, then the first pass will get |
| find ${PREFIX}/${LIB_DIR} -type f -name ${LIB_PREFIX}std-*${LIB_SUF} \ | ||
| | sed -ne 's,^.*/'${LIB_PREFIX}'std-\(.*\)'${LIB_SUF}'$,\1,gp' \ | ||
| | head -n1 | ||
| ;; |
There was a problem hiding this comment.
I can't tell what this is doing. Can you add more comments?
|
Hm yeah I believe the intention of these two flags was:
Along those lines the two situations you've described in the description I think are covered by these two flags? When you're bootstrapping from a previous release |
|
@TimNN unfortunately that doesn't work for stable builds. @alexcrichton Thanks, yes Going forward I could: a) revert this whole PR and instead improve the documentation to be a bit clearer about what distributors are supposed to do with those flags |
|
@infinity0 Then I probably misunderstood something, this script behaves as expected for me (tested with stable 1.9.0 & 1.10.0): the first |
|
Again, the bootstrap key isn't the whole story. Using I think just improving the documentation is the better choice, if it's not clear what these are doing. |
|
FWIW, so far I'm not trying to get fancy detecting the local version for the Fedora spec. I just have a rebuild flag to manually toggle in the spec, and then |
|
@infinity0 actually both of your suggestions sound great to me! I think we could definitely use some bolstered docs here, and I'd love to see the I think the script should:
|
|
@cuviper Could you explain in more detail this comment: "Using |
|
@infinity0 Sometimes there are breaking changes in syntax that hasn't stabilized yet, and |
|
@TimNN whoops my bad, your snippet works fine. I was confused and expected @cuviper OK thanks for the explanation. I wasn't expecting those sorts of behaviour changes, but that makes sense. Based on everyone's comments, my new idea is to set Technical details: I have to do this in the makefiles because Naming suggestion: I'd also suggest to rename |
|
I actually like the idea of inferring Also yeah doing this in the makefiles is fine, but could you also be sure to add support in rustbuild ( |
infinity0
force-pushed
the
master
branch
3 times, most recently
from
a6f5030
to
d98a256
Compare
|
@alexcrichton @brson Updated commits, rewriting the original ones. Now I'm comparing These new commits make it harder for distributors to say "I really only want to use the {previous,current} version, fail if local-rust is the {current,previous} version", but I assume that this won't be a common use-case. The build still fails for any other versions of local-rust, and one could always check the build logs for the presence of the "auto-detected local-rebuild" message to see what was actually done. I've left the actual config options in for now, but could also remove those. |
infinity0
changed the title
|
Patch is looking better. Nice work. |
| let local_version = String::from_utf8( | ||
| Command::new(&self.rustc) | ||
| .arg("--version").output() | ||
| .expect("failed to execute rustc --version").stdout).unwrap(); |
There was a problem hiding this comment.
I believe there's a build_helper::output function which should help with running a command and getting stdout as a string
|
Nice! Looking great to me as well |
| # If local-rust is the same as the current version, then force a local-rebuild | ||
| ifdef CFG_ENABLE_LOCAL_RUST | ||
| ifeq (rustc $(CFG_RELEASE),\ | ||
| $(shell $(Q)$(S)src/etc/local_stage0.sh --print-rustc-version $(CFG_LOCAL_RUST_ROOT))) |
There was a problem hiding this comment.
OK, adding on to my note about --version, I see you are actually matching the "rustc" prefix at least. The optional git info would throw it off. I still think we should be a little more precise, because we might want vendor info to show in the basic --version string too, the same way gcc calls itself "gcc version 6.1.1 20160621 (Red Hat 6.1.1-3) (GCC)".
There was a problem hiding this comment.
Understood, it does look like --verbose is more specifically designed to be machine-parsed. I will go ahead and make that change.
|
OK, I've made some fixes. I only forced-pushed some of the later commits, but it looks like Github thinks the whole set is "outdated" - they aren't, the 3 initial commits with all your comments are still part of the PR. Anyway, let me know if you want me to do any commit squashing. |
| case "$TARG_DIR" in | ||
| --print-rustc-release) | ||
| # not actually copying to TARG_DIR, just print the local rustc version and exit | ||
| ${PREFIX}/bin/rustc${BIN_SUF} --version --verbose | sed -ne 's/^release: \(.*\)/\1/gp' |
There was a problem hiding this comment.
Not a big deal, but the wildcard is unnecessary. Just s/^release: //p will work.
|
Apart from my sed-golfing, it looks good to me, thanks! |
|
Ack, have fixed the sed expressions as well. |
|
Looks good to me, thanks! Want to squash the commits down as well? |
|
Sure, done: old (reviewed) commit: bf4cfb413b5ec08344f738a42ef5e7324899148d |
|
⌛ Testing commit bbff336 with merge 9f9b415... |
|
Argh, I forgot to update the first post of this PR, and now it's included in the merge commit's commit message. |
|
💡 This pull request was already approved, no need to approve it again.
|
|
⛄ The build was interrupted to prioritize another pull request. |
|
Oh hm, I may have just gotten confused... |
If local-rust is the same as the current version, then force a local-rebuild In Debian, we would like the option to build/rebuild the current release from *either* the current or previous stable release. So we use enable-local-rust instead of enable-local-rebuild, and read the bootstrap key dynamically from whatever is installed locally. In general, it does not make much sense to allow enable-local-rust without also setting the bootstrap key, since the build would fail otherwise. (The way I detect "the bootstrap key of [the local] rustc installation" is a bit hacky, suggestions welcome.)
In Debian, we would like the option to build/rebuild the current release from
either the current or previous stable release. So we use enable-local-rust
instead of enable-local-rebuild, and read the bootstrap key dynamically from
whatever is installed locally.
In general, it does not make much sense to allow enable-local-rust without also
setting the bootstrap key, since the build would fail otherwise.
(The way I detect "the bootstrap key of [the local] rustc installation" is a bit hacky, suggestions welcome.)