virtio-bindings: drop bindings for older kernels #192
Conversation
alyssais
requested review from
alexandruag,
andreeaflorescu,
jiangliu,
slp and
stsquad
as code owners
alyssais
changed the title
alyssais
changed the title
alyssais
changed the title
alyssais
force-pushed
the
drop-bindings
branch
from
a3e14dc to
54127c3
Compare
|
The CI failures seem to be due to the bindings being from a very old bindgen. That will be one of my next PRs, but it's not related to this one. I could add that onto the end of this PR to make CI happy if that's required. |
| mod bindings_v4_14_0; | ||
| #[cfg(feature = "virtio-v5_0_0")] | ||
| mod bindings_v5_0_0; | ||
| #![allow(clippy::all)] |
There was a problem hiding this comment.
I don't think this works as you intend it to work, which is also why the tests are failing. We want to allow clippy::all only in the bindings, now you are actually allowing it on the virtio_blk module which is the one that is immediately after the clippy macro.
There was a problem hiding this comment.
Actually I think this was correct. My understanding is that #! applies to the whole of the current module, whereas # applies only to the next item.
The tests were failing with normal Rust warnings, not clippy ones, because I had accidentally dropped this line:
vm-virtio/crates/virtio-bindings/src/bindings_v5_0_0/mod.rs
Lines 8 to 9 in c5895a6
Now that I've added it back as well, they're passing.
(That issue in bindgen has now been fixed, so when I regenerate the bindings (next in my stack) we can remove that attribute.)
alyssais
force-pushed
the
drop-bindings
branch
2 times, most recently
from
54f69e4 to
c10d05e
Compare
These were lost when virtio-bindings was imported into the vm-virtio repository. Without these features declared, crates that depended on these features wouldn't compile. Signed-off-by: Alyssa Ross <[email protected]>
alyssais
force-pushed
the
drop-bindings
branch
from
dad2622 to
45c7635
Compare
As pointed out by Sebastien Boeuf [1], the kernel headers are backwards compatible, so there's no need to maintain the bindings for multiple kernel versions. And doing so will make updating the bindings easier, as it won't require adding a new feature each time. It's unfortunately not possible for us to emit a deprecation warning for the features, as in Rust deprecation warnings have to be applied to functions/constants/etc. (not modules or re-exports [2]), so any deprecation warning in this crate would mean editing the generated bindings files. But it doesn't really do any harm to keep the feature declarations in the Cargo.toml around indefinitely. [1]: rust-vmm/virtio-bindings#35 (comment) [2]: rust-lang/rust#30827 Signed-off-by: Alyssa Ross <[email protected]>
alyssais
dismissed stale reviews from andreeaflorescu and sboeuf
via
f98465e
alyssais
force-pushed
the
drop-bindings
branch
from
45c7635 to
f98465e
Compare
Summary of the PR
As pointed out by @sboeuf, the kernel headers are backwards compatible, so there's no need to maintain the bindings for multiple kernel versions. And doing so will make updating the bindings easier, as it won't require adding a new feature each time.
But before we can do that, we actually have to re-add the kernel version features to the Cargo.toml, because they were lost when virtio-bindings was imported into vm-virtio, and this would break existing users of the crate next time a release is made.
It's unfortunately not possible for us to emit a deprecation warning for the features, as in Rust deprecation warnings have to be applied to functions/constants/etc. (not modules or re-exports](rust-lang/rust#30827)), so any deprecation warning in this crate would mean editing the generated bindings files. But it doesn't really do any harm to keep the feature declarations in the Cargo.toml around indefinitely.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
unsafecode is properly documented.