-
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
Fully remove submodule handling from bootstrap.py #97513
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
25d75fa
to
396e63d
Compare
This comment has been minimized.
This comment has been minimized.
Won't this cause problems for vendoring? |
Probably. |
@bjorn3 I added bootstrap to the tidy check, but I think we might not actually need to be so restrictive with bootstrap as with the other tools? We don't distribute bootstrap, it's only used in-tree, so I don't think it adds additional obligations to users building from source. @Mark-Simulacrum or someone else from core might know more.
Hmm, good point. Can we only vendor rustbuild itself in bootstrap.py, and add the rest of the vendoring in rustbuild? Merging the .cargo/config files seems kind of tricky, though ... |
Btw, why is vendoring handled through x.py? Why don't we ask people to configure vendoring themselves if they care? |
This comment has been minimized.
This comment has been minimized.
The source tarballs that distros probably build through are unconditionally vendored. |
☔ The latest upstream changes (presumably #97546) made this pull request unmergeable. Please resolve the merge conflicts. |
If we need to maintain the submodule code in bootstrap.py, it kind of defeats the point of this change :/ I wonder if we can somehow get git to only fetch the Cargo.toml / Cargo.lock files that we need? Or move vendoring to rustbuild somehow? |
8fbf51a
to
969287c
Compare
I'm going to move this to waiting-on-review since I'm not sure the right approach to take for vendoring when submodules haven't yet been cloned. @rustbot ready |
Ok, so here's a terrible idea: have multiple vendor directories, one only for bootstrap and nothing else, and one for the workspace. Bootstrap.py will manage the first, rustbuild will manage the second. This has the upside of letting us get rid of submodule handling; it has the downside that now we have to duplicate all the vendoring code in rustbuild. That's probably not too terribly hard; |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #97887) made this pull request unmergeable. Please resolve the merge conflicts. |
It looks like we're trying to support vendoring sources in x.py, but I'm not actually sure that's necessary. Supporting running with vendored sources makes sense to me (i.e., if you downloaded a src tarball, we shouldn't need further network from there). But if you want to produce such a tarball, then I think it's either on you to use x.py dist rust-src... I'm not sure there's a strong use case for enabling vendoring when working locally and having x.py vendor just before the build. I'd be OK saying that if folks want that they can manually find the command and run it, which removes the need to support vendoring from x.py, I think; it just leaves the need to support using a vendor directory, which is much simpler (and doesn't need duplication). @cuviper -- is my impression that we don't need to support vendoring from a regular checkout correct? I don't know what would push someone to want that. |
@bors r+ rollup=never |
📌 Commit 81482e6 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (20a6f3a): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression Footnotes |
I am having problems with using vendoring crates and downloaded tarball (currently with nighty and beta), and I am wondering if the commit could be linked to the problem. see #100364 for details. |
Needed after changes in rust-lang#97513.
bootstrap.py handling of submodules was removed in rust-lang#97513.
do not remove `.cargo` directory If vendoring isn't used bootstrap removes `.cargo` directory, which prevents developers from setting certain options in the `.cargo/config.toml` file. This was introduced in rust-lang#97513 (specifically in [this commit](rust-lang@345eb14)). Also, since rust-lang#123942, vendoring is now possible even in git sources, which means we shouldn't remove `.cargo` directory in git sources anymore.
do not remove `.cargo` directory If vendoring isn't used bootstrap removes `.cargo` directory, which prevents developers from setting certain options in the `.cargo/config.toml` file. This was introduced in rust-lang#97513 (specifically in [this commit](rust-lang@345eb14)). Also, since rust-lang#123942, vendoring is now possible even in git sources, which means we shouldn't remove `.cargo` directory in git sources anymore.
Rollup merge of rust-lang#132054 - onur-ozkan:cargo-config, r=Kobzol do not remove `.cargo` directory If vendoring isn't used bootstrap removes `.cargo` directory, which prevents developers from setting certain options in the `.cargo/config.toml` file. This was introduced in rust-lang#97513 (specifically in [this commit](rust-lang@345eb14)). Also, since rust-lang#123942, vendoring is now possible even in git sources, which means we shouldn't remove `.cargo` directory in git sources anymore.
NOTE: If you are coming here because of the link in the changelog, the new way to vendor the workspace is by running
cargo vendor --sync ./src/tools/rust-analyzer/Cargo.toml --sync ./compiler/rustc_codegen_cranelift/Cargo.toml --sync ./src/bootstrap/Cargo.toml
manually. If you want to use the pre-downloaded cargo, runmake prepare
first, then use the cargo inbuild/$TARGET/stage0/bin/cargo
. Unlike the rest of bootstrapping, this is mostly independent of the version and should work even with fairly old versions of cargo.These submodules were previously updated in python because Cargo gives a hard error if toml files
are missing from the workspace:
However, bootstrap doesn't actually need to be part of the workspace.
Remove it so we can move submodule handling fully to Rust, avoiding duplicate code between Rust and Python.
Note that this does break
cargo run
; it has to becd src/bootstrap && cargo run
now.Given that we're planning to make the main entrypoint a shell script (or rust binary),
I think this is a good tradeoff for reduced complexity in bootstrap.py.
To get this working, I also had to remove support for vendoring when using the git sources, because
cargo vendor
requires all submodules to be checked out. I think this is ok; people who care about this are likely already using the pre-vendoredrustc-src
tarball.Fixes #90764. Helps with #94829