-
Notifications
You must be signed in to change notification settings - Fork 109
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
Update kvm-bindings to 0.9.1 #277
Conversation
Signed-off-by: Bo Chen <chen.bo@intel.com>
Signed-off-by: Bo Chen <chen.bo@intel.com>
@rbradford @roypat PTAL. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work! IMHO since this release contains some changes like marking functions unsafe
would break existing code, which makes it backward incompatible. If we follow the guidelines of semver
, the major version number ought to be bumped. But in this repo, the minor version number is somehow treated as major, it would be fine if maintainers say so.
Still, wouldn't it be better to incorporate remove trailing whitespace
to prepare release
commit, it doesn't contain much important information to substantiate it as a separate commit in my view :)
Am happy to do this release, but since we did a patch release for kvm-bindings, y'all should just be able to pick it up with |
For crates that have not yet reached a 1.0.0 release, semver says that the minor version plays the role of the major version. E.g. doing API changes going from version 0.18 to 0.19 is perfectly fine (https://doc.rust-lang.org/cargo/reference/semver.html#change-categories) |
I normally have each commit to be self-contained for the sake of maintainability (e.g. easier to trace changes to a certain module or backporting, etc.). I am normally not squashing changes from different components into one even they are trivial. That seems to be the practice here as far as I see from the change history of changelog. Of course, I am open to different practices from this community.
@roypat Ah, you are right. I was worried bumping the top-level version of kvm-bindings to v0.9.1 only would introduce two versions of kvm-bindings from our dependency tree since kvm-ioctl/vfio-ioctl still points to v0.9.0. But it didn't happen. Very interesting. I encountered this kind of multi-version issues from rust-vmm dependencies many times before, the most recently one was about bumping kvm-ioctls IIRC. Not sure what makes a difference here. With that, I will let you decide if this is a good time to cut a release. If not, I will make this PR only for bumping kvm-bindings to v0.9.1. |
Yeah, I'd also keep the things in separate commits
cargo will treat dependencies that are specified as At least I think that's how it works
If its fine for you, it might make sense to wait until the riscv64 stuff is all merged. That will require a major release anyway, so we can avoid doing two releases in quick succession |
The RISC-V |
That makes sense. The update of kvm-bindings from 0.9.0 to 0.9.1 is only a patch update, and hence semver compatible. A minor or major version bump would require all components to be updated accordingly, such as the recent vm-memory crate bump from 0.14.1 to 0.15.0.
That's good context. Totally make sense. Let me drop the release part of this PR then. |
927e202
to
d79cdfa
Compare
Summary of the PR
This PR updated to kvm-bindings v0.9.1 to include a recent bug fixes [1].
[1] rust-vmm/kvm-bindings#113
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.