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

Deprecate GuestMemory stopgap "iterator" methods #144

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Deprecate GuestMemory stopgap "iterator" methods #144

merged 1 commit into from
Mar 11, 2021

Conversation

defunctio
Copy link
Contributor

@defunctio defunctio commented Mar 5, 2021

  • Deprecates with_regions with_regions_mut and map_and_fold.
  • Update unit tests

Resolves #133

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "vm-memory"
version = "0.5.0"
version = "0.5.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a break change, better to bump to "0.6"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, it's deprecating instead of removing. "0.5.1" should be ok:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically update the version when doing the release. Do we plan to do a release once this is merged? We should update the changelog as well, and specify which functions were deprecated.

Copy link
Contributor Author

@defunctio defunctio Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think we would want a minor version update just to indicate when they were deprecated. I'll update the changelog for now. Let me know if you would like me to revert the version back to 0.5.0 to avoid a release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to update the version on the same PR when we are doing the release to follow the same (undocumented 😆 ) process as before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case would the deprecated macro still work?

Copy link
Contributor Author

@defunctio defunctio Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, still works. It would seem that when the deprecated version is a future version, the current version/branch also displays the deprecation warning. Do you have a version number preference for deprecation?

Note: this will also cause check-warnings-* in CI to fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the next release we want to include dirty page tracking, so I think we will have it at 0.6.0. I would just add a no-op issue to validate that we are releasing the version specified in the deprecate macros.

@defunctio defunctio marked this pull request as ready for review March 5, 2021 06:48
src/mmap.rs Show resolved Hide resolved
Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "vm-memory"
version = "0.5.0"
version = "0.5.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically update the version when doing the release. Do we plan to do a release once this is merged? We should update the changelog as well, and specify which functions were deprecated.

@andreeaflorescu
Copy link
Member

The warnings look legit to me. Should we update the atomic.rs tests to use iter instead of with_regions?

@defunctio
Copy link
Contributor Author

The warnings look legit to me. Should we update the atomic.rs tests to use iter instead of with_regions?

That would be the tests for atomic.rs and mmap.rs I updated in the original PR 😅 . Let me add them back.

@andreeaflorescu
Copy link
Member

The warnings look legit to me. Should we update the atomic.rs tests to use iter instead of with_regions?

That would be the tests for atomic.rs and mmap.rs I updated in the original PR . Let me add them back.

Sorry about that. I didn't realize that the reason you were changing the tests was because of the warnings.

@defunctio
Copy link
Contributor Author

defunctio commented Mar 9, 2021

Sorry about that. I didn't realize that the reason you were changing the tests was because of the warnings.

No worries, it wasn't clear :)

Just code coverage complaints from CI now.

@andreeaflorescu
Copy link
Member

Just code coverage complaints from CI now.

Can you update the coverage score? You can do that by changing the coverage_config_x86_64.json file with the new coverage value.

* Deprecates `with_regions` `with_regions_mut` and `map_and_fold`.
* Update unit tests

Signed-off-by: defunct <defunct@defunct.io>

Resolves #133
@jiangliu jiangliu merged commit bc0f7c2 into rust-vmm:master Mar 11, 2021
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.

Deprecate GuestMemory stopgap "iterator" methods
3 participants