bootstrap: skip llvm-tools install when externally provided #109196
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ozkanonur (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
src/bootstrap/install.rs
Outdated
| if let Some(tarball) = builder.ensure(dist::LlvmTools { target: self.target }) { | ||
| install_sh(builder, "llvm-tools", self.compiler.stage, Some(self.target), &tarball); | ||
| } else { | ||
| // LlvmTools can be externally provided | ||
| builder.info( | ||
| &format!("skipping Install LlvmTools stage{} ({})", self.compiler.stage, self.target), | ||
| ); | ||
| } |
There was a problem hiding this comment.
We should check if llvm is provided or not. If llvm isn't provided externally and there is a problem with tarballs, we want to panic the process instead of skipping it. With this diff any problem with tarball will skip the process regardless.
There was a problem hiding this comment.
I agree, but I don't know how to do what you suggest.
All other installs (Miri, etc..) either panic or skip if the tarball is not defined.
There is no example of a conditional skip.
Edit: I based my fix on a recent and similar fix: e5c92bc
There was a problem hiding this comment.
You can implement a function in Config that checks if any of the configure_args items starts with --llvm-root, and returns bool value depend on that.
There was a problem hiding this comment.
Done. Had to move some code around.
|
@rustbot author |
rustbot
added
S-waiting-on-author
| @@ -251,6 +251,11 @@ install!((self, builder, _config), | |||
| }; | |||
| ); | |||
|
|
|||
| fn should_build_llvm(config: &Config) -> bool { | |||
| // additionaly check that llvm is not externally provided | |||
| LlvmTools::should_build(config) && config.is_rust_llvm(config.build) | |||
There was a problem hiding this comment.
That code is not correct : the config.build is not the one we want...
There was a problem hiding this comment.
if any of the configure_args items starts with --llvm-root
Can you do this check in here? It should do the work
| @@ -251,6 +251,11 @@ install!((self, builder, _config), | |||
| }; | |||
| ); | |||
|
|
|||
| fn should_build_llvm(config: &Config) -> bool { | |||
| // additionaly check that llvm is not externally provided | |||
There was a problem hiding this comment.
Can you write this as a doc-comment on top of this function? It will be better for other devs to directly read this via LSP/rust-analyzer.
|
☔ The latest upstream changes (presumably #109284) made this pull request unmergeable. Please resolve the merge conflicts. |
|
fixed by #109256 |
fixes #109194