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 #479: Use workspace inheritance for dependencies #556

Closed
wants to merge 11 commits into from

Conversation

Aphoh
Copy link
Contributor

@Aphoh Aphoh commented Dec 22, 2022

Description

closes: #479


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.
  • Wrote unit tests (no new code)
  • Updated relevant documentation in the code (no docs to change)
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md (does this require a mention in the changelog? Not super relevant to consumers of the library)
  • Re-reviewed Files changed in the GitHub PR explorer

@Aphoh Aphoh requested review from a team as code owners December 22, 2022 23:13
@Aphoh Aphoh requested review from Pratyush, mmagician and weikengchen and removed request for a team December 22, 2022 23:13
ec/Cargo.toml Outdated
@@ -14,25 +14,25 @@ edition = "2021"
rust-version = "1.57"
Copy link
Member

Choose a reason for hiding this comment

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

@Pratyush just a quick check, this means that our minimal support version is 1.64 right?

Copy link
Member

Choose a reason for hiding this comment

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

Yup

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rust-version = "1.57"
rust-version = "1.64"

@mmagician mmagician added this to the next patch release milestone Dec 28, 2022
If we clone algebra inside curves, it will look at the curves Cargo.toml
for workspace dependencies. Cloning it outside fixes this.
@Aphoh
Copy link
Contributor Author

Aphoh commented Dec 29, 2022

Merged master and pushed a fix that should work for the failing CI item.

@weikengchen
Copy link
Member

added that we may put it in "the version" immediately after 0.4.0 due to the need to increase MSVP.

@Aphoh
Copy link
Contributor Author

Aphoh commented Dec 31, 2022

This failing CI item works for me when I run it locally with nektos/act... I'm not sure why it now says it can't find one of the curves' sub-directories. Any ideas?

Copy link
Member

@Pratyush Pratyush 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 great PR! Left a couple of comments that should be addressed, but otherwise this looks great!

poly-benches/Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@mmagician
Copy link
Member

We should finally incorporate this (and bump rust version) in the next release.

@mmagician mmagician mentioned this pull request Sep 18, 2023
6 tasks
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.

Use workspace inheritance to reduce duplication in crate Cargo.tomls
4 participants