Add tools to sysroot after build. #102010

WaffleLapkin · 2022-09-19T12:17:28Z

As the title suggest, this makes bootstrap add tools to sysroot after building them. This allows the tools to be used with rustup, e.g.

; x b rustfmt
; rustfmt +stage1 ./t.rs

Resolves #97762
Resolves #81431

r? @jyn514

I copied the implementation from the same thing with rustdoc (link), however there are some questions/problems:

Q: Do we need miri in the sysroot? It seems like the only way to run miri is throw cargo-miri...
Q: Do we need both cargo-clippy and clippy-driver in the sysroot?

P: when building any tool, all other tools are deleted for some reason that I can't seem to grasp

; x b cargo-miri
; ls build/x86_64-unknown-linux-gnu/stage1/bin
cargo-miri  rustc
; x b rustfmt
; ls build/x86_64-unknown-linux-gnu/stage1/bin
rustc  rustfmt
; x b clippy
; ls build/x86_64-unknown-linux-gnu/stage1/bin
clippy-driver  rustc

WaffleLapkin · 2022-09-19T12:39:21Z

With the power of ~~friendship~~ debug prints, I was able to find out that Sysroot step deletes tools from the bin:

rust/src/bootstrap/compile.rs

Lines 1107 to 1108 in 503e19d

    
           let _ = fs::remove_dir_all(&sysroot); 
        
           t!(fs::create_dir_all(&sysroot));

jyn514 · 2022-09-19T14:13:01Z

when building any tool, all other tools are deleted for some reason

Hmm, I don't recall why we delete the whole sysroot, but copying the whole stage-N-tools directly rather than just the newly built binary seems like a reasonable fix :)

jyn514 · 2022-09-19T14:14:42Z

Q: Do we need miri in the sysroot? It seems like the only way to run miri is throw cargo-miri...
Q: Do we need both cargo-clippy and clippy-driver in the sysroot?

✨ Try it and see ✨ :P can you run cargo +stage1 clippy and cargo +stage1 miri to see if they work?

I do think we should support miri, but it sounds like you're asking specifically about miri vs cargo-miri.

WaffleLapkin · 2022-09-19T16:11:53Z

Hmm, I don't recall why we delete the whole sysroot, but copying the whole stage-N-tools directly rather than just the newly built binary seems like a reasonable fix :)

Hm, have you meant stageN-tools-bin or stageN-tools/TARGET/release? The former doesn't seem to contain everything (e.g. there is no cargo-clippy even though I'm sure I've built it...), while the latter seems to contain too much...

; ls build/x86_64-unknown-linux-gnu/stage1-tools-bin                             
cargo-miri  clippy-driver  miri  rust-analyzer-proc-macro-srv  rustfmt
; ls build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release
build           cargo-fmt.d    clippy-driver.d  git-rustfmt.d  librustfmt_nightly.d     rust-analyzer-proc-macro-srv    rustfmt-format-diff
cargo-clippy    cargo-miri     deps             incremental    librustfmt_nightly.rlib  rust-analyzer-proc-macro-srv.d  rustfmt-format-diff.d
cargo-clippy.d  cargo-miri.d   examples         libmiri.d      miri                     rustfmt
cargo-fmt       clippy-driver  git-rustfmt      libmiri.rlib   miri.d                   rustfmt.d

jyn514 · 2022-09-19T16:15:43Z

I meant stageN-tools-bin, it's weird that it doesn't have cargo-clippy. Maybe dig into why that isn't there? The logic for copying it to stageN-tools-bin is going to be essentially the same as copying it to the sysroot, so I wouldn't spend too much time trying to filter down stuff from TARGET/release

Mark-Simulacrum · 2022-09-19T16:24:00Z

I don't think we should copy everything - e.g., compiletest shouldn't get copied into the sysroot. In general, the reason we delete the directory is to make sure that we don't have cases where order of running x.py commands matters.

I would expect the deletion to happen before we copy into the directory though, because it should only happen once per run and we should invoke the relevant Step before copying things into it.

WaffleLapkin · 2022-09-20T00:39:33Z

I would expect the deletion to happen before we copy into the directory though, [...]

It does happen before the copy, but only the just-built thing and rustc are copied. Building cargo-miri adds it to the bin, building rustfmt after that removes cago-miri and adds rustfmt, etc.

WaffleLapkin · 2022-09-20T13:16:14Z

In general, the reason we delete the directory is to make sure that we don't have cases where order of running x.py commands matters.

How does this help? It seems like all files in stageN/bin are independent from each other, so even if we don't delete the directory, it'll be still fine

Mark-Simulacrum · 2022-09-20T13:27:10Z

How does this help? It seems like all files in stageN/bin are independent from each other, so even if we don't delete the directory, it'll be still fine

I don't think this is necessarily true (e.g., you might have one binary executing another), and it's much easier for something that is kept around persistently to e.g. have a stale binary that you don't realize should have been rebuilt and giving you very confusing results.

It does happen before the copy, but only the just-built thing and rustc are copied. Building cargo-miri adds it to the bin, building rustfmt after that removes cago-miri and adds rustfmt, etc.

Presumably this doesn't happen if you build both at once, e.g., x.py build src/tools/miri src/tools/rustfmt? It doesn't seem unreasonable to me to expect the user to do that, and not try to guess that you "still want" the old miri (particularly if that means e.g. that an edit to rustc either leaves a stale miri in place or magically detects it and starts building miri even though you've not asked for it).

WaffleLapkin · 2022-09-22T11:19:53Z

./x.py build src/tools/miri src/tools/rustfmt does work, but I don't think it's reasonable to expect users to do that. I personally would build tools when I need them, and if I first need tool-A, then tool-B, then I would run x b a, then use tool-A, then x b b and then I would be confused why tool-A stopped working.

IMO it would make more sense if we would clear the directory only when rustc is rebuilt.

; x b compiler # clears the directory
; x b miri     # leaves the directory as-is
; x b rustfmt  # leaves the directory as-is

; nano compiler/... # change compiler
; x b miri          # compiler is rebuilt => clears the directory

bors · 2022-09-22T12:28:53Z

☔ The latest upstream changes (presumably #102028) made this pull request unmergeable. Please resolve the merge conflicts.

Mark-Simulacrum · 2022-09-22T13:15:39Z

I suppose that's possible, but I'm typically very wary of conditional clearing for that kind of directory. It's currently the case that all sysroots managed by rustbuild are always re-created on every build (AFAIK), which is a really convenient property for the implementation. In most cases if the goal is a "stable" sysroot, I sort of want to say "x.py install" or similar is the right way to get that, not trying to make the existing sysroots more durable.

Part of the reason I worry here is that "only when rustc is rebuilt" is a pretty annoying property to actually get; we don't directly know in rustbuild when something is rebuilt, cargo does caching for us. We do have some clear_if_dirty and such, with stamps, but that's really something that we're trying to move away from over time because getting them right is really tricky. (For an example: changing codegen can mean that you've not modified the rustc binary, so its timestamp hasn't changed, but the librustc_codegen_llvm.so has, and we're not tracking that, so you end up with an inconsistent state where newly built binaries (e.g., tools, etc.) are linking against a potentially old std built by the old rustc. It is much simpler to always re-hard-link everything and gives you a lot of nice properties as a result.

jyn514 · 2022-10-01T21:18:54Z

@WaffleLapkin let's require for now that people need to use build rustfmt miri to get both at once; if we agree on a way to make that simpler in the future we can make it better then. Having a way to get the tools is better than nothing :)

jyn514

Thanks! LGTM other than the one question :)

src/bootstrap/tool.rs

WaffleLapkin · 2022-10-02T15:12:42Z

I can't get miri working, after x build miri cargo-miri, cargo +stage1 miri test fails with the following:

thread 'main' panicked at 'failed to determine underlying rustc version of Miri: CommandError { stdout: "", stderr: "/home/waffle/rust-b/build/x86_64-unknown-linux-gnu/stage1/bin/miri: error while loading shared libraries: librustc_driver-152a9025dac882e1.so: cannot open shared object file: No such file or directory\n" }', src/tools/miri/cargo-miri/src/util.rs:116:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

WaffleLapkin · 2022-10-02T15:21:42Z

clippy doesn't work either, although for a different reason. x build clippy only builds (?copied) clippy-driver, but not cargo-clippy, while x build cargo-clippy doesn't work at all:

error: no `build` rules matched ["cargo-clippy"]
help: run `x.py build --help --verbose` to show a list of available paths
note: if you are adding a new Step to bootstrap itself, make sure you register it with `describe!`
Build completed unsuccessfully in 0:00:00

WaffleLapkin · 2022-10-02T16:12:56Z

clippy mystery solved: both cargo-clippy and clippy-driver use the same path:

rust/src/bootstrap/tool.rs

Lines 869 to 870 in 91931ec

    
           CargoClippy, "src/tools/clippy", "cargo-clippy", stable=true, in_tree=true, {}; 
        
           Clippy, "src/tools/clippy", "clippy-driver", stable=true, in_tree=true, {};

We then only allow one thing to be called with a given path:

rust/src/bootstrap/builder.rs

Lines 316 to 326 in 91931ec

    
           // Handle all test suite paths. 
        
           // (This is separate from the loop below to avoid having to handle multiple paths in `is_suite_path` somehow.) 
        
           paths.retain(|path| { 
        
               for (desc, should_run) in v.iter().zip(&should_runs) { 
        
                   if let Some(suite) = should_run.is_suite_path(&path) { 
        
                       desc.maybe_run(builder, vec![suite.clone()]); 
        
                       return false; 
        
                   } 
        
               } 
        
               true 
        
           });

So only the first one (happens to be clippy-driver) is being built.

If I change paths to be different bootstrap tries to build both (I say tries, because error: manifest path /home/waffle/rust-b/src/tools/clippy/src/driver.rs/Cargo.toml does not exist).

Not sure what to do with this though

bors · 2022-10-05T11:52:57Z

☔ The latest upstream changes (presumably #102691) made this pull request unmergeable. Please resolve the merge conflicts.

jyn514 · 2022-10-05T23:30:24Z

I can't get miri working, after x build miri cargo-miri, cargo +stage1 miri test fails with the following:

thread 'main' panicked at 'failed to determine underlying rustc version of Miri: CommandError { stdout: "", stderr: "/home/waffle/rust-b/build/x86_64-unknown-linux-gnu/stage1/bin/miri: error while loading shared libraries: librustc_driver-152a9025dac882e1.so: cannot open shared object file: No such file or directory\n" }', src/tools/miri/cargo-miri/src/util.rs:116:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I think that's a bug in miri itself:

(bash@pop-os) ~/rust-lang/rust2 [17:54:06]
; cargo +r2-stage1 miri test
thread 'main' panicked at 'failed to determine underlying rustc version of Miri: CommandError { stdout: "", stderr: "/home/jnelson/rust-lang/rust2/build/x86_64-unknown-linux-gnu/stage1/bin/miri: error while loading shared libraries: librustc_driver-780f2316356d3243.so: cannot open shared object file: No such file or directory\n" }', src/tools/miri/cargo-miri/src/util.rs:116:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
(bash@pop-os) ~/rust-lang/rust2 [17:54:24]
; fd librustc_driver-780f2316356d3243.so build/x86_64-unknown-linux-gnu/
build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_driver-780f2316356d3243.so
build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_driver-780f2316356d3243.so
build/x86_64-unknown-linux-gnu/stage2/lib/librustc_driver-780f2316356d3243.so
; LD_LIBRARY_PATH=build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib cargo +r2-stage1 miri setup
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)...
error: could not determine rustc version  # strace shows that it's looking for the same `librustc_driver-780f2316356d3243.so`
note: run with `RUST_BACKTRACE=1` for a backtrace
fatal error: failed to run xargo, see error details above

We can worry about fixing that once your PR is merged :)

I agree the clippy stuff looks like a bug in bootstrap itself, I've pushed a commit that should fix it.

jyn514 · 2022-10-05T23:31:14Z

r? @Mark-Simulacrum for the commit that allows multiple steps to share the same path

Mark-Simulacrum · 2022-11-06T21:40:35Z

r=me, seems okay. I don't think there's anything inherently wrong in multiple things sharing the same path, though it will further hinder the efforts to make --exclude and such actually run (or stop running) a specific thing.

jyn514 · 2022-11-06T22:41:37Z

Great, thank you :)

@WaffleLapkin this is ready to merge once you fix the conflicts :)

In particular, this allows `CargoClippy` and `Clippy` to both use `src/tools/clippy` as the path.

jyn514 · 2022-11-07T20:07:35Z

@bors r+

bors · 2022-11-07T20:07:37Z

📌 Commit 472c3a4 has been approved by jyn514

It is now in the queue for this repository.

jyn514 · 2022-11-08T15:55:35Z

@WaffleLapkin I found that even clippy has the sysroot bug you mentioned for miri - I think we need to be doing some extra magic (maybe with -rpath?) to make sure clippy can find the sysroot.

@bors r- this isn't useful as-is

bors · 2023-01-14T06:27:15Z

☔ The latest upstream changes (presumably #106520) made this pull request unmergeable. Please resolve the merge conflicts.

jyn514 · 2023-01-16T15:27:00Z

cc https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/building.20miri.20out.20of.20a.20rustc.20branch/near/317849329, #53650, https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Stage.20numbering.20for.20tools

jyn514 · 2023-04-20T14:48:20Z

I'm going to close this in favor of #110365. Thank you for your work on this! It made the ongoing work easier :)

WaffleLapkin · 2023-04-20T16:16:29Z

It's nice to hear someone is working on this again! Especially because it did not look like I'd be able to work on this anytime soon 😅

rust-highfive assigned jyn514 Sep 19, 2022

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2022

jyn514 approved these changes Oct 2, 2022

View reviewed changes

src/bootstrap/tool.rs Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2022

WaffleLapkin force-pushed the bootsraps_your_tools branch from f79a1c0 to e8020e2 Compare October 2, 2022 10:34

WaffleLapkin mentioned this pull request Oct 3, 2022

cargo fmt and other tools don't work for a linked toolchain. #81431

Closed

rust-highfive assigned Mark-Simulacrum and unassigned jyn514 Oct 5, 2022

jyn514 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 5, 2022

jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2022

Mark-Simulacrum assigned jyn514 and unassigned Mark-Simulacrum Nov 6, 2022

Add tools to sysroot after build.

48a6cc7

rust-cloud-vms bot force-pushed the bootsraps_your_tools branch from 9791b9f to 97aaa15 Compare November 7, 2022 11:51

Allow multiple Steps to have the same path

472c3a4

In particular, this allows `CargoClippy` and `Clippy` to both use `src/tools/clippy` as the path.

rust-cloud-vms bot force-pushed the bootsraps_your_tools branch from 97aaa15 to 472c3a4 Compare November 7, 2022 16:33

WaffleLapkin requested a review from jyn514 November 7, 2022 17:19

bors added S-waiting-on-bors 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 Nov 7, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2022

jyn514 mentioned this pull request Nov 28, 2022

Support documenting cargo #105036

Merged

jyn514 mentioned this pull request Mar 15, 2023

Remove type ascription from parser and diagnostics #109128

Merged

onur-ozkan mentioned this pull request Apr 20, 2023

ship tools with sysroot #110365

Merged

jyn514 closed this Apr 20, 2023

WaffleLapkin deleted the bootsraps_your_tools branch April 20, 2023 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tools to sysroot after build. #102010

Add tools to sysroot after build. #102010

WaffleLapkin commented Sep 19, 2022 •

edited

Loading

WaffleLapkin commented Sep 19, 2022 •

edited

Loading

jyn514 commented Sep 19, 2022

jyn514 commented Sep 19, 2022

WaffleLapkin commented Sep 19, 2022

jyn514 commented Sep 19, 2022

Mark-Simulacrum commented Sep 19, 2022

WaffleLapkin commented Sep 20, 2022

WaffleLapkin commented Sep 20, 2022

Mark-Simulacrum commented Sep 20, 2022

WaffleLapkin commented Sep 22, 2022

bors commented Sep 22, 2022

Mark-Simulacrum commented Sep 22, 2022

jyn514 commented Oct 1, 2022

jyn514 left a comment

WaffleLapkin commented Oct 2, 2022

WaffleLapkin commented Oct 2, 2022

WaffleLapkin commented Oct 2, 2022

bors commented Oct 5, 2022

jyn514 commented Oct 5, 2022

jyn514 commented Oct 5, 2022

Mark-Simulacrum commented Nov 6, 2022

jyn514 commented Nov 6, 2022

jyn514 commented Nov 7, 2022

bors commented Nov 7, 2022

jyn514 commented Nov 8, 2022

bors commented Jan 14, 2023

jyn514 commented Jan 16, 2023

jyn514 commented Apr 20, 2023

WaffleLapkin commented Apr 20, 2023

Add tools to sysroot after build. #102010

Add tools to sysroot after build. #102010

Conversation

WaffleLapkin commented Sep 19, 2022 • edited Loading

WaffleLapkin commented Sep 19, 2022 • edited Loading

jyn514 commented Sep 19, 2022

jyn514 commented Sep 19, 2022

WaffleLapkin commented Sep 19, 2022

jyn514 commented Sep 19, 2022

Mark-Simulacrum commented Sep 19, 2022

WaffleLapkin commented Sep 20, 2022

WaffleLapkin commented Sep 20, 2022

Mark-Simulacrum commented Sep 20, 2022

WaffleLapkin commented Sep 22, 2022

bors commented Sep 22, 2022

Mark-Simulacrum commented Sep 22, 2022

jyn514 commented Oct 1, 2022

jyn514 left a comment

Choose a reason for hiding this comment

WaffleLapkin commented Oct 2, 2022

WaffleLapkin commented Oct 2, 2022

WaffleLapkin commented Oct 2, 2022

bors commented Oct 5, 2022

jyn514 commented Oct 5, 2022

jyn514 commented Oct 5, 2022

Mark-Simulacrum commented Nov 6, 2022

jyn514 commented Nov 6, 2022

jyn514 commented Nov 7, 2022

bors commented Nov 7, 2022

jyn514 commented Nov 8, 2022

bors commented Jan 14, 2023

jyn514 commented Jan 16, 2023

jyn514 commented Apr 20, 2023

WaffleLapkin commented Apr 20, 2023

WaffleLapkin commented Sep 19, 2022 •

edited

Loading

WaffleLapkin commented Sep 19, 2022 •

edited

Loading