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

Let ark-ff specify the same num-bigint version as the rest of the repos #317

Merged
merged 1 commit into from Sep 24, 2021

Conversation

weikengchen
Copy link
Member

@weikengchen weikengchen commented Sep 22, 2021

Description

This is related to #316 but is not expected to be a solution.

There is a pending "unstable feature" in Rust 1.56 beta that requires num-bigint to be bumped up to 0.4.2 or higher to be able to compile.

My local test seems to show that the compiler would always retrieve the 0.4.2 version (since arkwork-rs never uses Cargo.lock). So this seems to be an issue for downstream applications to handle.

Nevertheless, we can improve the current code: only the algebra and r1cs-std repos use num-bigint, and ark-ff is the only one that is specifying 0.4.0, while the rest are all using 0.4.

This PR changes what ark-ff specifies in the Cargo.toml to 0.4, thereby keeping it consistent with the rest.

This does not closes #316. We will see whether more changes will be needed due to potential Rust changes.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Re-reviewed Files changed in the Github PR explorer

N/A:

  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md

Copy link
Member

@vlopes11 vlopes11 left a comment

Choose a reason for hiding this comment

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

LGTM. Generally speaking its a good thing to use only major/minor, but we commit that num-bigint follows semver good practices.

I'll in parallel check their history and will come back to you in case historically they make breaking changes as patches

Copy link
Member

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍🏼

@weikengchen
Copy link
Member Author

Some additional notes before I merged:

  1. About semver (major/minor/patch), I agree that keeping major/minor should be sufficient for our cases.

  2. There seem to be no breaking changes between 0.4.0 and 0.4.2 (https://docs.rs/crate/num-bigint/0.4.2/source/RELEASES.md). The main commit that is related to the issue is rust: use explicitily Integer::div_ceil rust-num/num-bigint#219.

@weikengchen weikengchen merged commit d7fa00e into master Sep 24, 2021
@weikengchen weikengchen deleted the num-bigint-version branch September 24, 2021 16:42
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.

CI Breakage on num-bigint 0.4.0
3 participants