-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add support for -Zbuild-std
to cargo fetch
#10129
Conversation
src/cargo/ops/cargo_fetch.rs
Outdated
// Fetch the dependencies for `test` just in case | ||
// (cargo could later decide to build libtest even if it wasn't specified in the flags) | ||
to_download.push(std_resolve.query("test")?); | ||
packages.add_set(std_package_set); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was very non-trivial to try and figure out whether test
would be necessary ahead of time. (I think it may not even be possible to tell since you need to know whether cargo is checking or building?)
I'd like to test that |
72ed22d
to
d082d0b
Compare
Ah, I think |
d082d0b
to
e6f0d65
Compare
7f185c6
to
7df467c
Compare
// easy to get the host triple in BuildConfig. Consider changing | ||
// requested_target to an enum, or some other approach. | ||
anyhow::bail!("-Zbuild-std requires --target"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't delete this, it's here for a reason. The -Zbuild-std
unit graph is not correct unless --target
is passed and a lot more is needed to get this working without it. Simply because the test suite doesn't fail doesn't mean this can be deleted.
7df467c
to
85aa602
Compare
Following up on this, I continue to believe that the hardcoding of |
@rustbot ready |
I tried to address Alex's concerns. |
6e2c001
to
56c4f7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to pick this up without Alex help 🥲
So far as I read the conversation, it seems that you've resolved Alex's concerns. Thanks for the help!
Could you squash some old commits? It may confuse people when tracing the history. Thanks! |
This allows downloading the dependencies for libstd in advance, which can be useful in e.g. sandboxed build environments. - Abstract check for `--target` out into a function - Try to abstract `test` special-casing into a function This avoids hard-coding crate names in multiple places. - Unify handling of checks for `--target` in `BuildConfig::new` This makes sure it's checked consistently, without requiring each new command to check it explicitly. - Share more code between `fetch` and `build` by adding `std_crates()` - Warn about `--build-plan` and `-Zbuild-std` consistently, not just for `build` Currently only `build` uses build-plan. But cargo may choose to add it to new commands in the future (e.g. check and doc). Future-proof it, since it's simple to do.
Looks good to go. Thanks! @bors r+ |
📌 Commit 55b680c has been approved by |
☀️ Test successful - checks-actions |
Update cargo 7 commits in f63f23ff1f1a12ede8585bbd1bbf0c536e50293d..a44758ac805600edbb6ba51e7e6fb81a6077c0cd 2022-04-28 03:15:50 +0000 to 2022-05-04 02:29:34 +0000 - Add support for `-Zbuild-std` to `cargo fetch` (rust-lang/cargo#10129) - Migrate tests of `cargo-init` to snapbox (rust-lang/cargo#10620) - dedupe toml_edit crate, followup rust-lang/cargo#10603 (rust-lang/cargo#10619) - Update GitHub Actions actions/checkout@v2 to v3 (rust-lang/cargo#10618) - Integrate snapbox in with cargo-test-support (rust-lang/cargo#10581) - Fix zsh completion (rust-lang/cargo#10613) - Update documentation for workspace inheritance (rust-lang/cargo#10611)
This allows downloading the dependencies for libstd in advance, which
can be useful in e.g. sandboxed build environments.
Fixes rust-lang/wg-cargo-std-aware#22.
r? @ehuss