-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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: don't resolve symlinks for initial_cargo #121341
Conversation
r? @clubby789 rustbot has assigned @clubby789. Use r? to explicitly pick a reviewer |
This PR modifies If appropriate, please update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stopping bootstrapping on the invalid configuration is actually quite useful (so people doesn't waste time on building when configuration is invalid). I would rather change this validation to be running a simple cargo command and checking if it's working or not. With this approach, your usecase should be working just fine and we can keep validating the initial cargo.
r? onur-ozkan |
@rustbot review |
☔ The latest upstream changes (presumably #121400) made this pull request unmergeable. Please resolve the merge conflicts. |
bdaf186
to
94d6b72
Compare
} | ||
PathBuf::from(cargo) | ||
} else { | ||
config.download_beta_toolchain(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this fixes a bug. Thanks :)
I will take this to the merge queue once it's been rebased and the commits have been squashed/unified. Having the first commit no longer makes sense. |
+ Do not resolve symlinks (as this may break rustup) + Check version not just for rustc, but also for cargo. + Check that the program's self-reported name matches the expected name (such as "rustc" or "cargo").
94d6b72
to
c42057f
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (71ff1c6): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 651.286s -> 650.172s (-0.17%) |
Thank you a lot for the review and the feedback! Really appreciate it ❤️ |
I have put the following in my
config.toml
:I have rustup installed from Arch's repos, which has all of the above paths be symlinks to
/usr/bin/rustup
. This works just fine with theargv[0]
trick that rustup uses.However,
bootstrap
resolves symlinks to check whethercargo
exists and then uses the resolved path, so it ends up callingrustup
directly expecting it to behave likecargo
. Which it doesn't.This PR removes the canonicalization step, in turn fixing the issue, but sacrificing a pretty error message. However, this exact thing is checked by
x.py
in advance, so I hope it is not a big deal?