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 missing dependency in solana-curve25519's Cargo.toml #4301

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

hycinth22
Copy link

@hycinth22 hycinth22 commented Jan 6, 2025

Problem

Currently, solana-curve25519 indirectly depends on crate subtle through curve25519-dalek.

here, solana-curve25519 uses into_option provided by crate subtle (provided in subtle >=2.6)


but dependency curve25519-dalek only require subtle version >=2.3. (https://docs.rs/crate/curve25519-dalek/4.1.3/source/Cargo.toml)


Currently, we specify subtle version 2.6.1 in agave workspace Cargo.toml.
That's why the compilation of validator can succeed.

subtle = "2.6.1"

But there is no one in solana-curve25519's Cargo.toml


For users who only use the crate solana-curve25519,
this will cause Cargo to resolve the lower version of subtle and report into_option cannot be found.

Summary of Changes

Fixes # add it in solana-curve25519's Cargo.toml to fix potential compile error

@mergify mergify bot requested a review from a team January 6, 2025 14:30
Copy link

@joncinque joncinque 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 your contribution! I confirm that this is an issue. Just a little nit to make it clear what's going on for future readers -- let me know what you think

curves/curve25519/Cargo.toml Show resolved Hide resolved
@joncinque joncinque added the CI Pull Request is ready to enter CI label Jan 6, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 6, 2025
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

CI will also fail due to uncommitted Cargo.lock changes -- can you run ./scripts/cargo-for-all-lock-files.sh tree and commit those changes too?

curves/curve25519/Cargo.toml Outdated Show resolved Hide resolved
@hycinth22
Copy link
Author

updated 😀 thanks for reviewing

@joncinque joncinque added the CI Pull Request is ready to enter CI label Jan 7, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 7, 2025
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

We're almost there, I swear!

curves/curve25519/Cargo.toml Show resolved Hide resolved
@hycinth22
Copy link
Author

hope everything is fixed.

BTW, is there a way for me to check these rules next time I submit a PR?

@joncinque joncinque added the CI Pull Request is ready to enter CI label Jan 7, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 7, 2025
@joncinque
Copy link

BTW, is there a way for me to check these rules next time I submit a PR?

To be honest, it's a bit annoying to do locally, which is why I didn't bring it up. But you can run ./ci/test-checks.sh and ./ci/test-sanity.sh if you have the required tools installed

Copy link

@joncinque joncinque 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 your contribution!

@joncinque joncinque merged commit a75585e into anza-xyz:master Jan 7, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants