-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Search for target_triple.json only if builtin target not found #58601
Conversation
src/librustc_target/spec/mod.rs
Outdated
match load_specific(target_triple) { | ||
Ok(t) => return Ok(t); | ||
LoadTargetError::BuiltinTargetNotFound(_) => (), | ||
LoadTargetError::Other(e) => return Err(e), |
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.
I'm not so sure about these load errors, it seems to me that they are a compiler bug. Either we panic on them and test them, or we allow the user to work around them by specifying a json file. I'd prefer the former.
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.
I'm not so sure about these load errors, it seems to me that they are a compiler bug.
When you target, say, an ios
target, creating the Target executes code like this: https://github.com/rust-lang/rust/blob/master/src/librustc_target/spec/apple_ios_base.rs#L29
Which can fail, say on Linux, if that tool is not available.
So in that case, load target failed, for a reason that AFAICT isn't a compiler bug, but an user bug: its running the compiler without the appropriate tools in the path.
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.
I don't know if reporting those errors breaks somebody's workflow. E.g. if they are relying on the builtin target failing for external reasons to override it with a json file. But that seems very brittle, and that use case is better addressed with a --target-file=file.json
(or just --target=file.json
) CLI option that explicitly requests obtaining the target from a json file.
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.
that makes sense
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Before this commit, if the builtin target was found, but an error happened when instantiating it (e.g. validating the target specification file failed, etc.), then we ignored those errors and proceeded to try to find a `target_triple.json` file, and if that failed, reported that as an error. With this commit, if rustc is supposed to provide the builtin target, and something fails while instantiating it, that error will get properly propagated.
@bors r+ rollup |
📌 Commit 103ed0c has been approved by |
Search for target_triple.json only if builtin target not found Before this commit, if the builtin target was found, but an error happened when instantiating it (e.g. validating the target specification file failed, etc.), then we ignored those errors and proceeded to try to find a `target_triple.json` file, and if that failed, reported that as an error. With this commit, if rustc is supposed to provide the builtin target, and something fails while instantiating it, that error will get properly propagated. r? @oli-obk
Search for target_triple.json only if builtin target not found Before this commit, if the builtin target was found, but an error happened when instantiating it (e.g. validating the target specification file failed, etc.), then we ignored those errors and proceeded to try to find a `target_triple.json` file, and if that failed, reported that as an error. With this commit, if rustc is supposed to provide the builtin target, and something fails while instantiating it, that error will get properly propagated. r? @oli-obk
Rollup of 17 pull requests Successful merges: - #57656 (Deprecate the unstable Vec::resize_default) - #58059 (deprecate before_exec in favor of unsafe pre_exec) - #58064 (override `VecDeque::try_rfold`, also update iterator) - #58198 (Suggest removing parentheses surrounding lifetimes) - #58431 (fix overlapping references in BTree) - #58555 (Add a note about 2018e if someone uses `try {` in 2015e) - #58588 (remove a bit of dead code) - #58589 (cleanup macro after 2018 transition) - #58591 (Dedup a rustdoc diagnostic construction) - #58600 (fix small documentation typo) - #58601 (Search for target_triple.json only if builtin target not found) - #58606 (Docs: put Future trait into spotlight) - #58607 (Fixes #58586: Make E0505 erronous example fail for the 2018 edition) - #58615 (miri: explain why we use static alignment in ref-to-place conversion) - #58620 (introduce benchmarks of BTreeSet.intersection) - #58621 (Update miri links) - #58632 (Make std feature list sorted) Failed merges: r? @ghost
Before this commit, if the builtin target was found, but an error
happened when instantiating it (e.g. validating the target
specification file failed, etc.), then we ignored those errors
and proceeded to try to find a
target_triple.json
file, and ifthat failed, reported that as an error.
With this commit, if rustc is supposed to provide the builtin target,
and something fails while instantiating it, that error will
get properly propagated.
r? @oli-obk