Skip to content

Conversation

@randombit
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the chore label Jan 6, 2026
@randombit randombit force-pushed the jack/fix-cargo-build-of-ic-secp256r1 branch from 98f0be8 to f52c765 Compare January 6, 2026 17:50
@randombit
Copy link
Contributor Author

 $ cargo test
   Compiling ic-secp256r1 v0.1.0 (/home/jack/work/dfinity/packages/ic-secp256r1)
error[E0599]: no method named `to_bytes` found for struct `ecdsa::der::Signature` in the current scope
   --> packages/ic-secp256r1/src/lib.rs:421:22
    |
421 |         sig.to_der().to_bytes().to_vec()
    |         ---          ^^^^^^^^
    |         |
    |         method `to_bytes` is available on `&ecdsa::Signature<NistP256>`
    |

I have no idea why this is happening -- as far as I can tell cargo and bazel should be using the same version of p256 with the same set of feature flags.

This particular utility function was introduced in #7886, and bisecting confirms that the commit that merges #7886 is the one that causes the regression [I had initially assumed the culprit was going to be some bazel config update]. Since the system tests only work with bazel, surely I only tested it with bazel and not cargo. And apparently CI doesn't check that the packages subdir crates build with cargo. :/

@randombit randombit marked this pull request as ready for review January 6, 2026 20:06
@randombit randombit requested a review from a team as a code owner January 6, 2026 20:06
@fspreiss
Copy link
Contributor

fspreiss commented Jan 7, 2026

 $ cargo test
   Compiling ic-secp256r1 v0.1.0 (/home/jack/work/dfinity/packages/ic-secp256r1)
error[E0599]: no method named `to_bytes` found for struct `ecdsa::der::Signature` in the current scope
   --> packages/ic-secp256r1/src/lib.rs:421:22
    |
421 |         sig.to_der().to_bytes().to_vec()
    |         ---          ^^^^^^^^
    |         |
    |         method `to_bytes` is available on `&ecdsa::Signature<NistP256>`
    |

I have no idea why this is happening -- as far as I can tell cargo and bazel should be using the same version of p256 with the same set of feature flags.

This particular utility function was introduced in #7886, and bisecting confirms that the commit that merges #7886 is the one that causes the regression [I had initially assumed the culprit was going to be some bazel config update]. Since the system tests only work with bazel, surely I only tested it with bazel and not cargo.

Its #7806, right?

I had a quick look and also don't understand why this is happening. Really weird. Maybe some subtle difference in dependency resolution??

And apparently CI doesn't check that the packages subdir crates build with cargo. :/

@basvandijk, could it be that CI doesn't check that the packages subdir crates build with Cargo?

@basvandijk
Copy link
Collaborator

@basvandijk, could it be that CI doesn't check that the packages subdir crates build with Cargo?

The cargo-linux job builds the whole cargo workspace by running cargo build --release --locked in the root of the repo. I also see our cargo workspace references the packages under packages/. So those packages should be build.

@randombit randombit requested a review from a team as a code owner January 7, 2026 17:22
@github-actions github-actions bot added the @idx label Jan 7, 2026
@randombit
Copy link
Contributor Author

For historical record - this was discussed on Slack. CI does build with cargo but it builds as part of the workspace, and feature unification across the workspace causes p256 to be built with the alloc feature, which is what as required by this util function. So this PR adds alloc as an explicit crate feature to p256 to slightly reduce weird surprises in the future.

@randombit randombit enabled auto-merge January 7, 2026 17:53
@randombit randombit added this pull request to the merge queue Jan 8, 2026
Merged via the queue into master with commit 930b984 Jan 8, 2026
38 checks passed
@randombit randombit deleted the jack/fix-cargo-build-of-ic-secp256r1 branch January 8, 2026 17:59
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