chore: handle deprecations in winterfell 0.8.3 release #290
Conversation
|
/cc @bobbinth |
There was a problem hiding this comment.
Thank you! Looks great! I left one question inline and depending on the answer we can either merge as is, or merge after a small change.
NOTE: I couldn't create a branch in this repo, so had to open the PR from a fork, may want to add me as a contributor, but I don't know how often I'll be pushing code here, so I'm fine working off a fork.
Added you to the repo!
src/utils/mod.rs
Outdated
| pub use winter_utils::{ | ||
| boxed, string, uninit_vector, Box, ByteReader, ByteWriter, Deserializable, | ||
| DeserializationError, Serializable, SliceReader, | ||
| uninit_vector, Box, ByteReader, ByteWriter, Deserializable, DeserializationError, Serializable, | ||
| SliceReader, | ||
| }; |
There was a problem hiding this comment.
Since we are removing utils::boxed, utils::string, and a few other items from the public interface, would this not be a breaking change?
If so, I think this PR should probably go against next. Or if we keep the re-exports and add deprecation warnings, then this could go against main.
There was a problem hiding this comment.
Ah, good point, I'll add those back with deprecation warnings (I actually can't recall if the deprecation warnings from winter-utils propagate or not, I think they do, but I'll try to confirm quick).
There was a problem hiding this comment.
I've added those back, and it looks like by re-exporting them compiling miden-crypto will produce compilation warnings for referencing those modules, which is fine. I did look to see if I could silence them when building miden-crypto, but produce new deprecation warnings if the re-exported modules are referenced, but unfortunately that doesn't appear to be possible (or at least my little test couldn't trigger the deprecation warning in that configuration).
Anyway, should be good to go now, but just wanted to note that one detail.
There was a problem hiding this comment.
Thank you! Yes, this should work fine - but I'm not seeing any new commits on this branch.
There was a problem hiding this comment.
Actually, looks like I accidentally pushed to origin rather than my fork, but it obviously succeeded since you added me as a contributor 😅. I pushed to the right origin this time, should be good to go now.
|
|
@bobbinth I don't think there is anyway to both keep those modules in, and preserve the deprecation warnings, since it causes clippy to complain (hence the failing status check). Probably the thing to do here is to temporarily allow the clippy checks to fail until the follow up release from Alternatively, you can disable the deprecation warnings for those modules, but I don't think downstream users of |
There was a problem hiding this comment.
All looks good! Thank you again for fixing these up!
Probably the thing to do here is to temporarily allow the clippy checks to fail until the follow up release from
nextthat removes them from the public API.
Yes - I think this is the right thing to do. I'll merge as is and then we can address it more properly in the next minor release (which I want to make as soon as we finish #285.
|
|
This PR handles the deprecations in the winterfell 0.8.3 release, and makes the same kind of changes made in facebook/winterfell#262 and in the corresponding commits of 0xPolygonMiden/miden-vm#1277 related to the upgrade.
I'm opening this against
mainand not next since I believe we'll want to do a patch release for this, but I can also open this againstnext, I have both staged locally, just let me know.NOTE: I couldn't create a branch in this repo, so had to open the PR from a fork, may want to add me as a contributor, but I don't know how often I'll be pushing code here, so I'm fine working off a fork.