Update rust-vmm-ci to start using cargo-all-features, and drop downstream test definitions. #326

roypat · 2025-10-01T11:22:26Z

Summary of the PR

With rust-vmm-ci now using cargo-all-features, all the custom test definitions for dealing with the incompatibility of the xen and postcopy features can be removed in favor of simply declaring that combination of features incompatible in Cargo.toml

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.

stefano-garzarella · 2025-10-01T12:03:12Z

@roypat love this!

About CI issue, I guess the doc issue is pre-existing and we see it just because we have the new test in the CI, right?

About the vhost_kern::net::tests::test_net_ioctls failure for musl, no idea. Do we have a way to understand which features are enabled?

stefano-garzarella · 2025-10-01T12:04:42Z

@roypat love this!

About CI issue, I guess the doc issue is pre-existing and we see it just because we have the new test in the CI, right?

Ah no wait, it seems it's related to --all-features and the compile_error!()

About the vhost_kern::net::tests::test_net_ioctls failure for musl, no idea. Do we have a way to understand which features are enabled?

roypat · 2025-10-01T12:13:21Z

@roypat love this!
About CI issue, I guess the doc issue is pre-existing and we see it just because we have the new test in the CI, right?

Ah no wait, it seems it's related to --all-features and the compile_error!()

Ah, heh, seems like the official build on docs.rs is actually failing for the very same reason (https://docs.rs/crate/vhost/0.14.0/builds/2193201). Let me look into whether there's some Cargo.toml stuff that specifically configures cargo doc.

About the vhost_kern::net::tests::test_net_ioctls failure for musl, no idea. Do we have a way to understand which features are enabled?

Yeah, a bit further up it says

     Running test crate=vhost features=[vhost-net]
   Compiling vhost v0.14.0 (/workdir/vhost)
   Compiling vhost-user-backend v0.20.0 (/workdir/vhost-user-backend)
    Finished `release` profile [optimized] target(s) in 2.45s
     Running unittests src/lib.rs (/workdir/target/x86_64-unknown-linux-musl/release/deps/vhost-3e91f0280e01fecd)

so in this case, its --no-default-features --features vhost-net.

vhost-user-backend/Cargo.toml

Some crates, such as vhost, use compile_error!() to indicate that mutually exclusive feature have been specified. This leads to problems when running cargo doc --all-features, which will inevitably trigger these conditions and then lead to documentation not building (for this reason, the vhost documentation currently fails to build on docs.rs for example). Add a custom cfg that gets passed via RUSTFLAGS in the cargo doc CI step on whose absense these compile_error!() macros can be gated, so that they dont cause issues when building documentation in CI See also rust-vmm/vhost#326 Signed-off-by: Patrick Roy <[email protected]>

Some crates, such as vhost, use compile_error!() to indicate that mutually exclusive feature have been specified. This leads to problems when running cargo doc --all-features, which will inevitably trigger these conditions and then lead to documentation not building (for this reason, the vhost documentation currently fails to build on docs.rs for example). Add a custom cfg that gets passed via RUSTFLAGS in the cargo doc CI step on whose absense these compile_error!() macros can be gated, so that they dont cause issues when building documentation in CI A custom cfg is needed because cargo doc does not pass `--cfg doc` down to dependencies. See also rust-vmm/vhost#326 Signed-off-by: Patrick Roy <[email protected]>

stefano-garzarella · 2025-10-02T09:02:54Z

@roypat thanks!

LGTM, but I'd move f22cc23 on top and add also a changelog line for it, WDYT ?

BTW we didn't fix https://docs.rs/crate/vhost/0.14.0/builds/2193201 right? (we should do in another PR, but not sure how)

roypat · 2025-10-02T09:22:07Z

@roypat thanks!

LGTM, but I'd move f22cc23 on top and add also a changelog line for it, WDYT ?

Ack

BTW we didn't fix https://docs.rs/crate/vhost/0.14.0/builds/2193201 right? (we should do in another PR, but not sure how)

We did actually! That's what the [package.metadata.docs.rs] stuff in the "fix docs" commit is about. I'll move it into a separate commit though.

stefano-garzarella · 2025-10-02T09:33:30Z

BTW we didn't fix https://docs.rs/crate/vhost/0.14.0/builds/2193201 right? (we should do in another PR, but not sure how)

We did actually! That's what the [package.metadata.docs.rs] stuff in the "fix docs" commit is about. I'll move it into a separate commit though.

Yep, but it doesn't work for deps as before, no?

I tried cargo doc --all-features --workspace --no-deps :

   Compiling userfaultfd-sys v0.6.0
    Checking vhost v0.14.0 (/home/stefano/repos/vhost-review/vhost)
 Documenting vhost v0.14.0 (/home/stefano/repos/vhost-review/vhost)
error: Both `postcopy` and `xen` features can not be enabled at the same time.
  --> vhost/src/lib.rs:63:1
   |
63 | compile_error!("Both `postcopy` and `xen` features can not be enabled at the same time.");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `vhost` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

but again, I don't know how to fix it in a clean way and IMHO it's outside of this PR

stefano-garzarella · 2025-10-02T09:35:02Z

@roypat thanks!
LGTM, but I'd move f22cc23 on top and add also a changelog line for it, WDYT ?

Ack

Sorry, on top I meant before updating rust-vmm-ci

roypat · 2025-10-02T09:36:15Z

BTW we didn't fix https://docs.rs/crate/vhost/0.14.0/builds/2193201 right? (we should do in another PR, but not sure how)

We did actually! That's what the [package.metadata.docs.rs] stuff in the "fix docs" commit is about. I'll move it into a separate commit though.

Yep, but it doesn't work for deps as before, no?

I tried cargo doc --all-features --workspace --no-deps :
   Compiling userfaultfd-sys v0.6.0
    Checking vhost v0.14.0 (/home/stefano/repos/vhost-review/vhost)
 Documenting vhost v0.14.0 (/home/stefano/repos/vhost-review/vhost)
error: Both `postcopy` and `xen` features can not be enabled at the same time.
  --> vhost/src/lib.rs:63:1
   |
63 | compile_error!("Both `postcopy` and `xen` features can not be enabled at the same time.");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `vhost` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
but again, I don't know how to fix it in a clean way and IMHO it's outside of this PR

Yes, just doing cargo doc --all-features still wont work, but what the new Cargo.toml section does (or is supposed to do, according to docs.rs documentation) is causing docs.rs to pass the same flag (in RUSTFLAGS) to cargo doc that our CI is passing to remove the compile_error!(), and so everything should render fine on docs.rs again

Sorry, on top I meant before updating rust-vmm-ci

oh oops, yes, can do that too haha, sorry

stefano-garzarella

Sorry, I hope I'm not too picky :-)

I left a comment and I'd ask you to add "vhost: " prefix on 61c6152 and 9e1611b and move both of them before the rust-vmm-ci update. IMO the "vhost: " prefix it's useful in a workspace during bisection, backporting, etc. to understand better which crate is affected.

.buildkite/README.md

.buildkite/rust-vmm-ci-tests.json

By using ioctl_with_ref() instead of ioctl_with_mut_ref(), we attempted to mutate through an immutable reference, so rustc was well within its rights to assume that `vring_state` does not change across the ioctl call, and hence optimize the return value of the function to simply be the value that `vring_state.num` was initialized to (which is 0). Signed-off-by: Patrick Roy <[email protected]>

Signed-off-by: Patrick Roy <[email protected]>

pick up the commit that introduces cargo-all-features. Signed-off-by: Patrick Roy <[email protected]>

Set it up so that it does not try to combine the xen and postcopy features, and also ignore the test-only test_utils feature. Signed-off-by: Patrick Roy <[email protected]>

Now that rust-vmm-ci uses cargo-all-features, we can deal with the incompatiblity of the xen and postcopy features by simply marking them as incompatible in Cargo.toml, instead of needing to define separate CI steps. note: this commit currently symlinks rust-vmm-ci-tests.json to rust-vmm-ci/.buildkite/test_descriptions.json, and removes all tests from custom-tests.json. This is to illustrate that the new setup works without changes to the buildkite pipeline. Once that is confirmed, the pipeline should be updated to just use the rust-vmm-ci test generaiton routine, and the entire .buildkite directory can be removed. Signed-off-by: Patrick Roy <[email protected]>

Introduce a dummy feature to hack around `cargo doc --all-features` always hitting the compile_error!(...) in vhost/src/lib.rs about incompatible features. This is happening because when documenting vhost-user-backend, cargo does not pass --cfg doc to dependencies, meaning the cfg(not(doc)) attribute that is supposed to eliminate this compile_error!() invokation when building docs does not actually trigger. Hence cargo doc fails. Introduce a custom cfg, which we tell docs.rs to set during documentation build, which disables the compile_error!(). Our CI also sets this flag for the rustdoc step (via an ugly `sed` in the pipeline definition). This cfg is also set by rust-vmm-ci for the cargo doc step. Note that we need both cfg(not(doc)) and cfg(not(RUTSDOC_disable_feature_compat_errors)), because lib.rs gets processed twice, onces by rustc (where the --cfg is passed via RUSTFLAGS), and once by rustdoc itself, where RUSTFLAGS are ignored, and instead the cfg(doc) macro comes into play (but rustdoc is only ran on the crate for which we are actually generating docs, not the dependencies). Signed-off-by: Patrick Roy <[email protected]>

Instruct the docs.rs documentation builder to pass --cfg RUTSDOC_disable_feature_compat_errors in RUSTFLAGS when building documentation, so that builds dont fail because of compile_error!() and incompatible features [1]. [1]: https://docs.rs/crate/vhost/0.14.0/builds/2193201 Signed-off-by: Patrick Roy <[email protected]>

roypat · 2025-10-02T09:54:33Z

Sorry, I hope I'm not too picky :-)

I left a comment and I'd ask you to add "vhost: " prefix on 61c6152 and 9e1611b and move both of them before the rust-vmm-ci update. IMO the "vhost: " prefix it's useful in a workspace during bisection, backporting, etc. to understand better which crate is affected.

done :)

stefano-garzarella

@roypat thanks for the patience and the work!

Just a note for maintainers, when we will merge this PR we will need to update buildkite pipeline, see d3cfd78

note: this commit currently symlinks rust-vmm-ci-tests.json to
rust-vmm-ci/.buildkite/test_descriptions.json, and removes all tests
from custom-tests.json. This is to illustrate that the new setup works
without changes to the buildkite pipeline. Once that is confirmed, the
pipeline should be updated to just use the rust-vmm-ci test generaiton
routine, and the entire .buildkite directory can be removed.

roypat requested review from Ablu, epilys, eryugey, germag, jiangliu, sboeuf, slp and stefano-garzarella as code owners October 1, 2025 11:22

roypat force-pushed the cargo-all-features branch 2 times, most recently from 0b51e91 to 8ed4403 Compare October 1, 2025 13:46

stefano-garzarella reviewed Oct 1, 2025

View reviewed changes

vhost-user-backend/Cargo.toml Outdated Show resolved Hide resolved

roypat force-pushed the cargo-all-features branch 2 times, most recently from eb9c470 to b871d2d Compare October 1, 2025 15:46

roypat marked this pull request as draft October 1, 2025 15:54

roypat marked this pull request as ready for review October 1, 2025 15:54

roypat mentioned this pull request Oct 2, 2025

ci: add --cfg for cargo doc to disable compile_error!() rust-vmm/rust-vmm-ci#195

Merged

4 tasks

roypat force-pushed the cargo-all-features branch 2 times, most recently from 443c900 to 02cae1b Compare October 2, 2025 08:48

roypat force-pushed the cargo-all-features branch from 02cae1b to 8d0ade3 Compare October 2, 2025 09:26

roypat force-pushed the cargo-all-features branch from 8d0ade3 to 61469b7 Compare October 2, 2025 09:36

stefano-garzarella reviewed Oct 2, 2025

View reviewed changes

.buildkite/README.md Show resolved Hide resolved

.buildkite/rust-vmm-ci-tests.json Show resolved Hide resolved

roypat added 7 commits October 2, 2025 10:50

vhost: fix unused function warnings on some feature combos

ee8de6a

Signed-off-by: Patrick Roy <[email protected]>

chore: update rust-vmm-ci

021606e

pick up the commit that introduces cargo-all-features. Signed-off-by: Patrick Roy <[email protected]>

configure cargo-all-features

be29efb

Set it up so that it does not try to combine the xen and postcopy features, and also ignore the test-only test_utils feature. Signed-off-by: Patrick Roy <[email protected]>

roypat force-pushed the cargo-all-features branch from 61469b7 to 1f6aafb Compare October 2, 2025 09:52

stefano-garzarella approved these changes Oct 2, 2025

View reviewed changes

stefano-garzarella enabled auto-merge (rebase) October 2, 2025 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update rust-vmm-ci to start using cargo-all-features, and drop downstream test definitions. #326

Update rust-vmm-ci to start using cargo-all-features, and drop downstream test definitions. #326

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

stefano-garzarella commented Oct 1, 2025

Uh oh!

stefano-garzarella commented Oct 1, 2025

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

Uh oh!

stefano-garzarella commented Oct 2, 2025

Uh oh!

roypat commented Oct 2, 2025

Uh oh!

stefano-garzarella commented Oct 2, 2025

Uh oh!

stefano-garzarella commented Oct 2, 2025

Uh oh!

roypat commented Oct 2, 2025

Uh oh!

stefano-garzarella left a comment

Uh oh!

Uh oh!

Uh oh!

roypat commented Oct 2, 2025

Uh oh!

stefano-garzarella left a comment

Uh oh!

Uh oh!

Update rust-vmm-ci to start using cargo-all-features, and drop downstream test definitions. #326

Are you sure you want to change the base?

Update rust-vmm-ci to start using cargo-all-features, and drop downstream test definitions. #326

Uh oh!

Conversation

roypat commented Oct 1, 2025

Summary of the PR

Requirements

Uh oh!

stefano-garzarella commented Oct 1, 2025

Uh oh!

stefano-garzarella commented Oct 1, 2025

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

Uh oh!

stefano-garzarella commented Oct 2, 2025

Uh oh!

roypat commented Oct 2, 2025

Uh oh!

stefano-garzarella commented Oct 2, 2025

Uh oh!

stefano-garzarella commented Oct 2, 2025

Uh oh!

roypat commented Oct 2, 2025

Uh oh!

stefano-garzarella left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

roypat commented Oct 2, 2025

Uh oh!

stefano-garzarella left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!