Skip to content
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

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

likebreath
Copy link
Contributor

@likebreath likebreath commented Sep 5, 2024

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:

  • 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.

Signed-off-by: Bo Chen <chen.bo@intel.com>
Signed-off-by: Bo Chen <chen.bo@intel.com>
@likebreath
Copy link
Contributor Author

@rbradford @roypat PTAL. Thank you.

Copy link
Contributor

@TimePrinciple TimePrinciple left a 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 :)

@roypat
Copy link
Collaborator

roypat commented Sep 6, 2024

@rbradford @roypat PTAL. Thank you.

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 cargo update, without a need for a new kvm-ioctls release!

@roypat
Copy link
Collaborator

roypat commented Sep 6, 2024

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.

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)

@likebreath
Copy link
Contributor Author

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 :)

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.

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 cargo update, without a need for a new kvm-ioctls release!

@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.

@roypat
Copy link
Collaborator

roypat commented Sep 6, 2024

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 :)

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.

Yeah, I'd also keep the things in separate commits

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 cargo update, without a need for a new kvm-ioctls release!

@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.

cargo will treat dependencies that are specified as crate = "x.y.z" as being satisfy-able by any version of crate that is semver compatible with x.y.z. So if one crate requires kvm-bindings 0.9.0 and another requires 0.9.1 (or cargo update is ran to update a lockfile to 0.9.1), then both crates will just resolve to 0.9.1. However, if one crate depends on kvm-bindings 0.8.0 and another on 0.9.1, then cargo won't unify them, because 0.8.0 and 0.9.1 aren't semver compatible.

At least I think that's how it works

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.

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

@TimePrinciple
Copy link
Contributor

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 :)

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.

Yeah, I'd also keep the things in separate commits

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 cargo update, without a need for a new kvm-ioctls release!

@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.

cargo will treat dependencies that are specified as crate = "x.y.z" as being satisfy-able by any version of crate that is semver compatible with x.y.z. So if one crate requires kvm-bindings 0.9.0 and another requires 0.9.1 (or cargo update is ran to update a lockfile to 0.9.1), then both crates will just resolve to 0.9.1. However, if one crate depends on kvm-bindings 0.8.0 and another on 0.9.1, then cargo won't unify them, because 0.8.0 and 0.9.1 aren't semver compatible.

At least I think that's how it works

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.

If its fine for you, it might make sense to wait until the riscv64 stuffs are all merged. That will require a major release anyway, so we can avoid doing two releases in quick succession

The RISC-V kvm-bindings are expected to be merged on next Monday. As of RISC-V kvm-ioctls, it's finished locally but yet meet the standard of community, it's expected to be finished after I complete cloud-hypervisor's RISC-V support draft. Hopefully before October.

@likebreath
Copy link
Contributor Author

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.

cargo will treat dependencies that are specified as crate = "x.y.z" as being satisfy-able by any version of crate that is semver compatible with x.y.z. So if one crate requires kvm-bindings 0.9.0 and another requires 0.9.1 (or cargo update is ran to update a lockfile to 0.9.1), then both crates will just resolve to 0.9.1. However, if one crate depends on kvm-bindings 0.8.0 and another on 0.9.1, then cargo won't unify them, because 0.8.0 and 0.9.1 aren't semver compatible.
At least I think that's how it works

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.

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.

If its fine for you, it might make sense to wait until the riscv64 stuffs are all merged. That will require a major release anyway, so we can avoid doing two releases in quick succession

The RISC-V kvm-bindings are expected to be merged on next Monday. As of RISC-V kvm-ioctls, it's finished locally but yet meet the standard of community, it's expected to be finished after I complete cloud-hypervisor's RISC-V support draft. Hopefully before October.

That's good context. Totally make sense. Let me drop the release part of this PR then.

@likebreath likebreath changed the title Prepare 0.19.0 release Update kvm-bindings to 0.9.1 Sep 6, 2024
@roypat roypat merged commit 2835116 into rust-vmm:main Sep 9, 2024
19 checks passed
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.

4 participants