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

fix: restore re-exports but using alloc #291

Conversation

mFragaBA
Copy link

@mFragaBA mFragaBA commented Mar 18, 2024

Describe your changes

On #290, with part of those changes we removed the re-exports of Vec, BTreeMap, BTreeSet, among others which breaks compilation of crates like miden-vm. Here we restore the re-exports (but using alloc), and in a future PR we should remove those as well (maybe for 0.9?)

Also, I tried marking those re-exports as deprecated but I couldn't make them appear (might be related to this issue)

Also: should I change the crate's version for the release in this PR or in a separate one?

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@mFragaBA mFragaBA force-pushed the mFragaBA/temporary-re-exports-for-deprecated-modules branch from 7c64a17 to 0a99d47 Compare March 18, 2024 17:08
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@mFragaBA mFragaBA changed the title restore re-exports but using alloc fix: restore re-exports but using alloc Mar 18, 2024
@@ -1,6 +1,7 @@
//! Utilities used in this crate which can also be generally useful downstream.

use alloc::string::String;
pub use alloc::{format, vec};
Copy link
Contributor

Choose a reason for hiding this comment

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

These macros were originally exported from the string and collections modules re-exported from winter-utils, so re-exporting them here isn't quite correct. See #292 for the patch that restores the original setup. I think @bobbinth is planning to merge both of those here shortly and cut new releases to fix downstream crates.

Copy link
Author

Choose a reason for hiding this comment

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

oh great news then! I thought it they were intentionally removed from winterfell so this was more of a workaround. I'll close this PR then. Thanks for the explanation @bitwalker!

Copy link
Author

Choose a reason for hiding this comment

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

btw, considering the deprecated modules will get removed eventually. Should we make a PR on miden-vm changing the usages of these re-exports for alloc::***, or do we just change the re-exports in miden-crypto?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is already done in 0xPolygonMiden/miden-vm#1277 - we just need to merge it an release it :)

@mFragaBA
Copy link
Author

As mentioned above, these changes are not needed after #292 got merged

@mFragaBA mFragaBA closed this Mar 18, 2024
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.

3 participants