Skip to content
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

Merged
merged 4 commits into from
Jun 25, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 29, 2022

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, run make prepare first, then use the cargo in build/$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:

error: failed to load manifest for workspace member `/home/jnelson/rust-lang/rust/src/tools/rls`

Caused by:
  failed to read `/home/jnelson/rust-lang/rust/src/tools/rls/Cargo.toml`

Caused by:
  No such file or directory (os error 2)
failed to run: /home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /home/jnelson/rust-lang/rust/src/bootstrap/Cargo.toml

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 be cd 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-vendored rustc-src tarball.

Fixes #90764. Helps with #94829

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ehuss
Copy link
Contributor

ehuss commented May 29, 2022

Won't this cause problems for vendoring?

@bjorn3
Copy link
Member

bjorn3 commented May 29, 2022

Probably. src/bootstrap would need to be added to the list of crates to vendor dependencies for at https://github.com/rust-lang/rust/blob/396e63dbcaa085d7d5f9491ea95bd2f839dfbbe8/src/bootstrap/bootstrap.py#L983 and it would still need the submodule updates before performing the vendoring. Even right now rust-analyzer is checked out by bootstrap.py when vendoring, despite it being handled by rustbuild when not vendoring.

@jyn514
Copy link
Member Author

jyn514 commented May 29, 2022

@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.

it would still need the submodule updates before performing the vendoring

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 ...

@jyn514
Copy link
Member Author

jyn514 commented May 29, 2022

Btw, why is vendoring handled through x.py? Why don't we ask people to configure vendoring themselves if they care?

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented May 29, 2022

Btw, why is vendoring handled through x.py? Why don't we ask people to configure vendoring themselves if they care?

The source tarballs that distros probably build through are unconditionally vendored.

@jyn514 jyn514 marked this pull request as draft May 29, 2022 18:09
@bors
Copy link
Contributor

bors commented May 30, 2022

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

@Mark-Simulacrum Mark-Simulacrum 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 Jun 1, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jun 8, 2022

it would still need the submodule updates before performing the vendoring

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?

@jyn514 jyn514 force-pushed the submodule-handling branch 2 times, most recently from 8fbf51a to 969287c Compare June 8, 2022 03:41
@jyn514
Copy link
Member Author

jyn514 commented Jun 8, 2022

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jun 8, 2022

Or move vendoring to rustbuild somehow?

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; cargo vendor is a lot simpler than git submodule.

@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 8, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 8, 2022

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

@Mark-Simulacrum
Copy link
Member

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.

@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2022
This duplicates a lot of checking, and doesn't seem particularly useful -
these are already caught in review.

Note that this still keeps the license check.
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 23, 2022

📌 Commit 81482e6 has been approved by Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jun 23, 2022

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@bors 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 Jun 23, 2022
@bors
Copy link
Contributor

bors commented Jun 25, 2022

⌛ Testing commit 81482e6 with merge 20a6f3a...

@bors
Copy link
Contributor

bors commented Jun 25, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 20a6f3a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 25, 2022
@bors bors merged commit 20a6f3a into rust-lang:master Jun 25, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jun 25, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (20a6f3a): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-0.3% -0.3% 1
Improvements 🎉
(secondary)
-0.7% -0.8% 6
All 😿🎉 (primary) -0.3% -0.3% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-3.4% -3.4% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.1% 2.1% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Warning ⚠: The following benchmark(s) failed to build:

  • rustc

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@semarie
Copy link
Contributor

semarie commented Aug 19, 2022

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.

tmandry added a commit to tmandry/rust that referenced this pull request Aug 29, 2022
ehuss added a commit to ehuss/rust that referenced this pull request Jul 26, 2024
bootstrap.py handling of submodules was removed in
rust-lang#97513.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x.py test try to access network when the compiler builds using vendored crates