Skip to content

virtio-queue doctests fail due to build-time inclusion conditionals #136

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

Closed
slp opened this issue Feb 2, 2022 · 7 comments
Closed

virtio-queue doctests fail due to build-time inclusion conditionals #136

slp opened this issue Feb 2, 2022 · 7 comments

Comments

@slp
Copy link
Collaborator

slp commented Feb 2, 2022

While building doctests, neither the test nor the doctest features are enabled. This means features gated behind [cfg(test...)] will not be available in doctests code. In virtio-queue case, this has an impact on mock visibility and some Descriptor helper methods aren't available either.

This is being discussed by the Rust Community here.

For the time being, we can either ignore the affected doctests (descriptor::Descriptor (line 21) and iterator::AvailIter (line 26)) or ungate those features. Personally, I'm fine with either.

@lauralt
Copy link
Contributor

lauralt commented Feb 2, 2022

This doesn't seem to be a problem anymore if the tests are run with --all-features, i.e. cargo test --all-features, cargo test --doc --all-features. Do you think there's another option we should consider regarding running the tests?

@slp
Copy link
Collaborator Author

slp commented Feb 2, 2022

@lauralt Yes, the test-utils feature works around the issue if you pass --all-features or --features test-utils, but it fails with bare cargo test, which can both be confusing for final users and problematic when packaging the crate in a distro (this is how I've noticed the issue).

@lauralt
Copy link
Contributor

lauralt commented Feb 2, 2022

It is indeed confusing, the same approach is used in event-manager as well, but it is documented. But besides the fact that it is confusing (there was a whole thread regarding this test feature proposal, but I fail to find it), we didn't think of having the bare cargo test passing as a requirement. Is this required for the distro use case, or is it possible to use --all-features or something else?

@slp
Copy link
Collaborator Author

slp commented Feb 2, 2022

I managed to work around this for Fedora, so I guess it should be possible to do the same thing for other distros. Should we just simply document this somewhere?

@lauralt
Copy link
Contributor

lauralt commented Feb 3, 2022

Yes, I'll document it.

lauralt added a commit to lauralt/vm-virtio that referenced this issue Feb 4, 2022
Added more details about how to run the tests, and mentioned
about the need to use `--features test-utils` when running
the tests in the virtio-queue crate.
Fixes: rust-vmm#136.

Signed-off-by: Laura Loghin <[email protected]>
lauralt added a commit to lauralt/vm-virtio that referenced this issue Feb 4, 2022
Added more details about how to run the tests, and mentioned
about the need to use `--features test-utils` when running
the tests in the virtio-queue crate.
Fixes: rust-vmm#136.

Signed-off-by: Laura Loghin <[email protected]>
@lauralt
Copy link
Contributor

lauralt commented Feb 4, 2022

@slp I opened #137, please have a look!

@slp
Copy link
Collaborator Author

slp commented Feb 7, 2022

@lauralt I see that #137 has already been merged. Thanks a lot for working on this!

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

No branches or pull requests

2 participants