-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
Problem
After #11851 Cargo becomes a Cargo workspace. That means we can dogfood ourselves finally 🎉.
We left some TODOs in that pull request, one of them are making --workspace
usable for cargo itself. That is, making cargo build
and other subcommands happy with --workspace
flag, instead of using -p
flag everywhere in our CI workflow yaml.
Some workspace members are platform-dependant inside cargo workspace, i.e., cargo-credential-gnome-secret
and friends. We don't really have a good programming way to get the list other than cargo metadata | jq .workspace_members[] -r | cut -d' ' -f1
. As a consequence, we expands the list manually in a ugly way:
cargo/.github/workflows/main.yml
Lines 117 to 128 in 5861176
- run: cargo test -p cargo-test-support | |
- run: cargo test -p cargo-platform | |
- run: cargo test -p cargo-util | |
- run: cargo test -p home | |
- run: cargo test -p mdman | |
- run: cargo build -p cargo-credential-1password | |
- run: cargo build -p cargo-credential-gnome-secret | |
if: matrix.os == 'ubuntu-latest' | |
- run: cargo build -p cargo-credential-macos-keychain | |
if: matrix.os == 'macos-latest' | |
- run: cargo build -p cargo-credential-wincred | |
if: matrix.os == 'windows-latest' |
We want to make running command like cargo test --workspace
as easy as possible to reduce the friction in CI also in contributor experience.
Proposed Solution
In the long term, we could look into #5220 and also the comment here #11987 (comment).
An alternative workaround in the short term would be making those subcrates and dependencies platform specific via conditional compilation. For example, dependencies could be listed in target."cfg(not(windows))".dependencies
section. And have a lib.rs
doing nothing but delegating to unix/lib.rs
or windows/lib.rs
with #[cfg(…)]
, and one of them also does nothing.
Notes
I think at least the follow commands should be able to run with --workspace
for cargo itself.
cargo build
/cargo check
cargo test
cargo doc
cargo clippy
(this is blocked on RFC: Add a[lints]
table toCargo.toml
rfcs#3389, so we can more easily propagate our clippy settings)To pick up a draggable item, press the space bar. While dragging, use the arrow keys to move the item. Press space again to drop the item in its new position, or press escape to cancel.
Activity
epage commentedon Apr 17, 2023
I feel like I'm missing the distinction between #11526 and #5220, they look like duplicates to me. I also feel that conceptually, conditional workspace members are a bad idea and both should be rejected. As the syntax is laid out, I read it as cargo resolving differently depending on the platform which means the lockfile is no longer cross-platform.
For me, I'd like a variant of #9406 which is instead
package.required-target
which behaves like<build-target>.required-features
in that we automatically skip those packages if we aren't building for the right target. We could also skip resolving any target-specific dependencies that do not matchpackage.required-target
orpackage.forced-target
, helping in cases like #7058 (and the related minimal lockfile issues).#9406 and
required-target
remind me of rust-lang/rfcs#3374 andrequired-features
.That said, I see this as short-term and long-term. Longer-term, we should do something like this but we likely don't want to wait on design and stablizxation, so we should do conditional builds in the short term.
weihanglo commentedon Apr 17, 2023
Totally valid. I was waiting for a response from #11526 but no. Closed.
And yep, clarified the short-term and long-term fixes in the PR descriptions. Thanks for the reply!
Auto merge of #11993 - epage:member, r=weihanglo
heisen-li commentedon Apr 26, 2023
I'll try to solve the
cargo test --workspace
for cargo itself.NanthR commentedon Jun 11, 2023
Hey, I wanted to work on this. I just had a couple of questions before I get started.
When you say target, this is talking about platform targets, correct?
weihanglo commentedon Jun 11, 2023
Yes. Please read relevant issues to track their progress.
This pull request is more like blocked on other pull requests or features. For exammple, I don't think there is an easy way to skip
cargo-credential-gnome-secret
without explicit exclusion, likecargo test --workspace --exclude "cargo-credential-gnome-secret"
. I don't think there is much we can do at this moment. Maybe only focus on reducing CI yaml? Anyway, feel free to drop your idea.NanthR commentedon Jun 11, 2023
Ok. Thanks. I was just thinking of adding an option to the package field to set the required platforms and having that be the filter of whether a package needs to be built. I read through the previous suggestions, and from my understanding, this is what seems to be the expected solution?
weihanglo commentedon Jun 11, 2023
Thanks for being interested in this! However
required-targets
is just one of the proposed solutions out there. We haven't decided on a proper way to solve this issue.Note that this is labeled as
A-infrastructure
, meaning that it focuses on making cargo the project infra better with existing functionality in cargo. If we want to resolve this issue with new features, please discuss in their own issues respectively, or open a new issue for a new feature proposal.Sorry for the confusion 😔.
--workspace
#12342Auto merge of #12342 - epage:ci, r=<try>
Auto merge of #12342 - epage:ci, r=weihanglo
Auto merge of #12342 - epage:ci, r=weihanglo
weihanglo commentedon Nov 27, 2023
This should be resolved by
[lints]
stabilization and conditional compilation forcargo-credential-*
crates.