Bootstrap: Don't get output if lldb --version errors by ChrisDenton · Pull Request #127112 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
fixes #126892
Command can error in two ways: the OS can fail to run the binary at all or else the binary can return an error exit code. Unfortunately the distinction between the two is not clear cut. The OS may succeed in starting the binary but it may still error before main (e.g. if a necessary library fails to load) and this will be reported via the exit code.
Fortunately this case is simpler. We can assume that lldb --version will only ever error if there's a startup issue of some kind. so both kinds of errors are caused by the OS. Thus it's safe for us to treat them equally for the sake of this specific check.
r? @Kobzol
rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
I don't understand how this fixes the crash. It seems just to change the returned error? The original issue showed a crash on line 1820, inside the run function, but that isn't used in this lldb invocation. It is probably crashing elsewhere (?) :) Probably line 1841.
Sorry, I didn't explain that very well. The code is a bit spaghetti but the run function is only invoked after the lldb --version is, It's guarded by the if let Some(ref vers) = lldb_version { block.
And lldb --version already fails but we don't actually check the exit status before reading the output of lldb --version so we're getting nonsense. Well probably Some("") because it doesn't print anything.
I've pushed a new commit that hopefully makes the code a bit clearer.
Aah, I understand now, thanks! Yeah, this command invocation code is very messy. I will nuke it soon (probably in the next couple of days) anyway with my bootstrap command refactoring PRs. But it doesn't hurt to fix the issue for now, it will help me understand the intent of the original code better.
Thanks!
@bors rollup r+
📌 Commit a6ef91e has been approved by Kobzol
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
I will nuke it soon (probably in the next couple of days) anyway with my bootstrap command refactoring PRs.
That'll be great!
bors added a commit to rust-lang-ci/rust that referenced this pull request
…llaumeGomez
Rollup of 7 pull requests
Successful merges:
- rust-lang#126805 (Migrate
pdb-alt-path,mismatching-target-triplesandmingw-export-call-conventionrun-maketests to rmake) - rust-lang#126995 (Migrate
pretty-print-with-dep-file,pretty-print-to-fileandlibtest-paddingrun-maketests to rmake) - rust-lang#127041 (Migrate
run-make/override-aliased-flagstormake.rs) - rust-lang#127072 (docs: say "includes" instead of "does include")
- rust-lang#127073 (Remove unnecessary SeqCst in
impl fmt::Pointer for AtomicPtr) - rust-lang#127112 (Bootstrap: Don't get output if
lldb --versionerrors) - rust-lang#127116 (Migrate
run-make/return-non-c-like-enumtormake.rs)
Failed merges:
- rust-lang#127050 (Make mtime of reproducible tarballs dependent on git commit)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#127112 - ChrisDenton:lldb, r=Kobzol
Bootstrap: Don't get output if lldb --version errors
fixes rust-lang#126892
Command can error in two ways: the OS can fail to run the binary at all or else the binary can return an error exit code. Unfortunately the distinction between the two is not clear cut. The OS may succeed in starting the binary but it may still error before main (e.g. if a necessary library fails to load) and this will be reported via the exit code.
Fortunately this case is simpler. We can assume that lldb --version will only ever error if there's a startup issue of some kind. so both kinds of errors are caused by the OS. Thus it's safe for us to treat them equally for the sake of this specific check.
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)