De-couple Python and bootstrap slightly #76544
Conversation
rust-highfive
added
the
S-waiting-on-review
|
cc @jyn514 |
jyn514
added
the
T-bootstrap
Mark-Simulacrum
force-pushed
the
less-python
branch
from
4b73660
to
18fd982
Compare
|
How crazy would it be to move all the logic to Rust and leave Something like this should allow updating submodules: rust-lang/cargo#1248 (comment) |
|
AFAICT, there is no way for Cargo to resolve the workspace without submodules being checked out -- the manifest for Cargo (src/tools/Cargo/Cargo.toml) is just not available. We also cannot easily make rustbuild its own workspace, because that prevents the top-level workspace from using If we want to move the logic, I think we will need to stop trying to build bootstrap locally on everyone's computer and instead download the binaries from CI -- we already know that bootstrap is only useful if your build triple has Rust (as we need a stage 0 compiler), so this is not a painful requirement I suspect. I would prefer to hold off on doing that, though, and the duplication in this PR is not major; most of the code it duplicates has been largely unchanged since it was introduced. I plan to explore moving our submodule dependencies to Cargo-based git dependencies, or similar, where possible -- that would let Cargo resolve everything normally and, hopefully, avoid some of the current pain points with submodules. I suspect this should be possible for all of the crates we need for workspace resolution, but have not checked. IMO asking people to do a |
This sounds really amazing! Maybe to avoid the duplication we could land Cargo-based git dependencies first? I don't feel super strongly about that but I do think submodules are much more painful than |
|
I do not yet know whether the submodules are even feasible to remove, it's something I do want to investigate. In any case, it seems like x.py is a hard blocker for some developers and submodules are likely not that (I've never really needed more than some git submodule update --init and git -C src/tools/cargo reset --hard). The duplication again is pretty minor. |
Mark-Simulacrum
force-pushed
the
less-python
branch
2 times, most recently
from
5d30a89
to
9305555
Compare
Mark-Simulacrum
added
S-waiting-on-author
|
Moving this over to waiting-on-author until I fix CI here, it looks like there's going to be a number of bugs in various components that were relying on the environment variables set for just rustbuild's consumption. |
Mark-Simulacrum
force-pushed
the
less-python
branch
from
9305555
to
e1b3fe0
Compare
|
Okay, this is now ready for review, marking as such. |
Mark-Simulacrum
added
S-waiting-on-review
|
What's the example usage for this? Can you run |
|
Yes, and no need for |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
less-python
branch
2 times, most recently
from
5b37cd0
to
10592e1
Compare
|
Okay, rebased. |
There was a problem hiding this comment.
I've left some comments inline, but overall I'm a little confused by the purpose of this PR. Is there a vision for where the build system is going to end up? I'm not sure what the end goal is myself and it feels like this is still pretty far from it but it's already bending over quite a lot and contorting various pieces here and there to try to make something work. I'd generally want to make sure that we know where this is all going to land before landing some of the weirder parts of this.
There's a couple of pieces though that seem good to land as it's just moving some stuff around and generally more into Rust than Python.
src/bootstrap/config.rs
Outdated
| } | ||
|
|
||
| let build_rustc = build.rustc; | ||
| config.initial_rustc = env::var_os("RUSTC") |
There was a problem hiding this comment.
Could this unconditionally be whatever rustc compiled the build script? (the build script should have a RUSTC env var)
There was a problem hiding this comment.
I think there's probably a cargo bug (or maybe rustup?), but for me that environment variable gets set to just "rustc" when compiling through rustup/cargo, even with +beta, which would mean that we don't use the right rustc. Not a problem with x.py, because that sets RUSTC in the environment which fixes things I believe.
I'm down for removing this whole section and trying to fix the upstream bug, wherever that is, though.
There was a problem hiding this comment.
Oh that's probably because Cargo just executes rustc itself, in that case could the build script read RUSTC and find it in PATH?
src/bootstrap/config.rs
Outdated
| // Make sure that our variables, pulled from Cargo, are equivalent to | ||
| // configuration. | ||
| if let Some(cargo) = build.cargo.map(PathBuf::from) { | ||
| assert_eq!(config.initial_cargo, cargo.canonicalize().expect("cargo exists")); |
There was a problem hiding this comment.
Could initial_cargo be set to the CARGO env var used to compile the build script?
There was a problem hiding this comment.
We do that here -- https://github.com/rust-lang/rust/pull/76544/files#diff-1731657fe6a8025cbe7a498b30e93890R478 -- this is just checking that if you've done that and also configured initial-cargo in the toml file they match. Maybe we shouldn't bother though and just treat the command line as an override?
There was a problem hiding this comment.
Ah yeah in that case I'd just treat this as an override.
src/bootstrap/lib.rs
Outdated
| .arg("build") | ||
| .arg("--manifest-path") | ||
| .stdout(std::process::Stdio::null()) | ||
| .arg(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml")); |
There was a problem hiding this comment.
I must admit, I don't really understand the motivation of this PR, but blocks like this seem pretty weird. This seems like it's bending over really far backwards to try to get cargo run to work. Cargo was never intended to be a full-fledged general purpose build system though, and building the compiler is just compilcated due to how much stuff needs to get built in various ways. This isn't the end of the world to have this here but this feels ripe for bugs and other issues.
(and this is just the beginning of trying to get cargo run to work...)
There was a problem hiding this comment.
Much of this discussion has been happening on Zulip. This PR probably doesn't make sense without having followed along there.
I will say that, regardless of where Cargo started or what was intended back in the day, it needs to become a general build system eventually, or at least become far closer to being one. Because Cargo is how every single Rust programmer is told to interact with their Rust projects. So when Cargo can't handle a task, it's suddenly astoundingly painful to do that task with a Rust project. This doesn't need to happen today or tomorrow, but it needs to be a long term goal.
There was a problem hiding this comment.
Much of this discussion has been happening on Zulip.
There was a problem hiding this comment.
Much of this discussion has been happening on Zulip
Ah ok, thanks for the heads up. For reviewing this though I don't have time to read up on the whole stream, so I'm gonna stick to what's in this PR.
it needs to become a general build system eventually
This PR is probably not the best location to push this idea.
src/bootstrap/config.rs
Outdated
| .out | ||
| .components() | ||
| .flat_map(|component| { | ||
| if component.as_os_str() == "$ROOT" { |
There was a problem hiding this comment.
This... seems like a misfeature. I guess it's supported in the python code though...
There was a problem hiding this comment.
This was added in #71994, but yeah I'm not entirely 100% sure it's something we truly need (vs. forcing absolute paths or whatever in configuration).
There was a problem hiding this comment.
I'd be ok with removing this. What I'd really like is for the build directory to always be relative to the source directory, not the current directory, but I don't think anyone is using $ROOT.
|
FWIW, with this PR locally I have So in that sense while I agree that this is definitely bending over backwards in some places it's not that much and does put us in a place where people who get pushed away by the Python entry point (I am told that on Windows, Python usage is a red flag by @Lokathor, though I don't personally have experience there to expand on that). I'm happy to drop some parts of this (or extract others into separate PRs), though -- I'm not personally feeling strongly that this set of commits and the duplication/contortions they introduce are all that desirable. Though admittedly I have a hard time evaluating whether it's worth it when I haven't myself experienced the Windows Python issues... |
|
cc also @Kixiron and @RDambrosio016 (#75156) |
|
@Mark-Simulacrum With simply proper documentation of the expected build environment, the python usage on Windows would be much less of a red flag to potential newcomers. However, I think that in (say) the next 5 years or so, moving towards the compiler and standard library working with just cargo and not an external tool would be very valuable. But I'm totally willing to believe that even with that as a goal, "it's not yet time for those changes". |
|
Python has the tendency to be "finicky" on windows, to say the least. Even past the issues I've encountered (and recognize are pretty fringe), having the build system in Rust is great since it lowers the barrier of entry since everyone contributing to the Rust compiler probably knows Rust to some capacity |
Mark-Simulacrum
force-pushed
the
less-python
branch
from
afed561
to
cf33aad
Compare
|
@bors r=alexcrichton |
|
📌 Commit cf33aad has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup=never (prone to failures on various builders) |
|
☀️ Test successful - checks-actions, checks-azure |
Same rationale as rust-lang#76544; it would be nice to make python entirely optional at some point. This also removes $ROOT as an option for the build directory; I haven't been using it, and like Alex said in rust-lang#76544 (comment) it seems like a misfeature. This allows running `cargo run` from src/bootstrap, although that still gives lots of compile errors if you don't use the beta toolchain.
…lacrum Move some more bootstrap logic from python to rust Same rationale as rust-lang#76544; it would be nice to make python entirely optional at some point. This also removes $ROOT as an option for the build directory; I haven't been using it, and like Alex said in rust-lang#76544 (comment) it seems like a misfeature. This allows running `cargo run` from src/bootstrap, although that still gives lots of compile errors if you don't use the beta toolchain. It's not exactly the same as using `x.py`, since it won't have `BOOTSTRAP_DOWNLOAD_RUSTC` set, but it's pretty close. Doing this from the top-level directory requires rust-lang/cargo#7290 to be fixed, or using `cargo run -p bootstrap`. The next steps for making python optional are to move download-ci-llvm and download-rustc support into rustbuild, likely be shelling out as the python scripts do today. It would also be nice (although not required) to move submodule support there, but that would require taking bootstrap out of the workspace to avoid errors from crates that haven't been cloned yet. r? `@Mark-Simulacrum`
This revises rustbuild's entry points from Python to rely less on magic environment variables, preferring to use Cargo-provided environment variables where feasible.
Notably, BUILD_DIR and BOOTSTRAP_CONFIG are not moved, because both more-or-less have some non-trivial discovery logic and replicating it in rustbuild seems unfortunate; if it moved to Cargo that would be a different story.
Best reviewed by-commit.