Skip to content

small fixups to cargo-all-features#194

Merged
roypat merged 3 commits intorust-vmm:mainfrom
roypat:clippy-all-features
Oct 1, 2025
Merged

small fixups to cargo-all-features#194
roypat merged 3 commits intorust-vmm:mainfrom
roypat:clippy-all-features

Conversation

@roypat
Copy link
Member

@roypat roypat commented Oct 1, 2025

  • Turns out any cargo subcommand can be ran like this, instead of just the ones documented on the cargo-all-features crates.io page, so run clippy like this as well
  • convert one last forgotten step (unittests-gnu)
  • revert riscv64 tests back to using --all-features, as compiling multiple permutations is simply too slow in the emulated environment :(

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Turns out any cargo subcommand can be ran like this, instead of just the
ones documented on the cargo-all-features crates.io page.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Fixes: 6f7d365 ("ci: convert build and test steps to use cargo-all-features")
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
ShadowCurse
ShadowCurse previously approved these changes Oct 1, 2025
Due to us running riscv tests in an emulated environment, compiling
multiple permutations of crates is simply too slow and causes buildkite
timeouts. Revert to using --all-features here.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat changed the title ci: run clippy using cargo-all-features small fixups to cargo-all-features Oct 1, 2025
@roypat roypat enabled auto-merge (rebase) October 1, 2025 09:34
@stefano-garzarella
Copy link
Member

@roypat LGTM, but my question now is, should we use cargo-all-features everywhere, or for example just with clippy and test ?

Can we just do some steps (e.g. build) with just a subset instead of all combinations?

@roypat roypat requested a review from ShadowCurse October 1, 2025 09:43
@roypat
Copy link
Member Author

roypat commented Oct 1, 2025

@roypat LGTM, but my question now is, should we use cargo-all-features everywhere, or for example just with clippy and test ?

Mh, yeah, I suppose if we do clippy, then we don't strictly need it for build as well, but then, what would the default build test be (because I wouldn't be completely comfortable to not have one at all)? Because if it falls back to --all-features, it won't work for vhost.

Can we just do some steps (e.g. build) with just a subset instead of all combinations?

I don't think so sadly :(

@stefano-garzarella
Copy link
Member

@roypat LGTM, but my question now is, should we use cargo-all-features everywhere, or for example just with clippy and test ?

Mh, yeah, I suppose if we do clippy, then we don't strictly need it for build as well, but then, what would the default build test be (because I wouldn't be completely comfortable to not have one at all)? Because if it falls back to --all-features, it won't work for vhost.

Default features?

Can we just do some steps (e.g. build) with just a subset instead of all combinations?

I don't think so sadly :(

I don't have a strong opinion on this, just an idea to avoid consuming too much CI time.

@roypat
Copy link
Member Author

roypat commented Oct 1, 2025

@roypat LGTM, but my question now is, should we use cargo-all-features everywhere, or for example just with clippy and test ?

Mh, yeah, I suppose if we do clippy, then we don't strictly need it for build as well, but then, what would the default build test be (because I wouldn't be completely comfortable to not have one at all)? Because if it falls back to --all-features, it won't work for vhost.

Default features?

Mh, yeah, I could be convinced of that. Do you feel strongly about changing this, or are you also happy to leave things as they are?

Can we just do some steps (e.g. build) with just a subset instead of all combinations?

I don't think so sadly :(

I don't have a strong opinion on this, just an idea to avoid consuming too much CI time.

I think this is fine, on vm-memory, which has a ton of features, the runtime of doing all the permutation steps is still quicker than build test on riscv without permutations, so generally I don't expect CI time to increase. For crates like vhost that don't support riscv, the build of all permutations takes 5 minutes on my laptop, which is indeed an increase, but hopefully fine? :o

@roypat roypat merged commit 58dab8b into rust-vmm:main Oct 1, 2025
3 checks passed
@stefano-garzarella
Copy link
Member

@roypat LGTM, but my question now is, should we use cargo-all-features everywhere, or for example just with clippy and test ?

Mh, yeah, I suppose if we do clippy, then we don't strictly need it for build as well, but then, what would the default build test be (because I wouldn't be completely comfortable to not have one at all)? Because if it falls back to --all-features, it won't work for vhost.

Default features?

Mh, yeah, I could be convinced of that. Do you feel strongly about changing this, or are you also happy to leave things as they are?

Not strong as I mentioned :-)

Can we just do some steps (e.g. build) with just a subset instead of all combinations?

I don't think so sadly :(

I don't have a strong opinion on this, just an idea to avoid consuming too much CI time.

I think this is fine, on vm-memory, which has a ton of features, the runtime of doing all the permutation steps is still quicker than build test on riscv without permutations, so generally I don't expect CI time to increase. For crates like vhost that don't support riscv, the build of all permutations takes 5 minutes on my laptop, which is indeed an increase, but hopefully fine? :o

Yep, let's see how it will go, we can eventually implement this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants