Skip to content

crypto-common: use core::error::Error trait; MSRV 1.81 #1660

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

Merged
merged 2 commits into from
Sep 28, 2024

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Sep 5, 2024

The core::error::Error trait is now stable, meaning we no longer need to gate impls for it on a std feature.

The `core::error::Error` trait is now stable, meaning we no longer need
to gate impls for it on a `std` feature.
@tarcieri tarcieri requested a review from newpavlov September 5, 2024 17:26
@tarcieri
Copy link
Member Author

tarcieri commented Sep 5, 2024

Note: only did crypto-common for now since 1.81 is definitely an aggressive MSRV bump. I think we should make this change in all of the crates in this repo, but I thought I'd start here for discussion before attempting to do them all.

@tarcieri
Copy link
Member Author

tarcieri commented Sep 5, 2024

Also, perhaps we should consider removing the std feature entirely from these crates unless it's actually used for things like filesystem access that truly need std.

Right now crypto-common has the following in its [features] section of Cargo.toml:

std = ["getrandom?/std", "rand_core?/std"]

I didn't remove it to keep the change minimal for now, but it's something we could do potentially. I don't think the transitive activation of the std features of those dependencies really helps when using them with crypto-common, especially since we already have a getrandom feature which can be used to access rand_core::OsRng.

@newpavlov
Copy link
Member

newpavlov commented Sep 5, 2024

I would really like to migrate to core::error::Error, but MSRV 1.81 is too aggressive in my opinion, so I think we should wait until the next release cycle.

we should consider removing the std feature entirely from these crates unless it's actually used for things like filesystem access that truly need std

I agree.

@tarcieri
Copy link
Member Author

tarcieri commented Sep 5, 2024

Okay, will leave this as-is and we can take a look again when 1.82 is released (October 17th)

@sorairolake
Copy link

If this is merged, will you make similar changes to crates in other repositories owned by RustCrypto?

@newpavlov
Copy link
Member

Yes, we will apply similar changes (including MSRV bump) to our downstream crates as well. Since RustCrypto/hybrid-array#85 has already landed, we are likely to move forward with this change.

@tarcieri tarcieri changed the title [WIP] crypto-common: use core::error::Error trait; MSRV 1.81 crypto-common: use core::error::Error trait; MSRV 1.81 Sep 20, 2024
@tarcieri tarcieri marked this pull request as ready for review September 20, 2024 13:46
@tarcieri
Copy link
Member Author

I removed draft. I'll hold off on merging until I'm ready to update the downstream crates accordingly (busy today / this weekend)

@tarcieri tarcieri merged commit 66ca151 into master Sep 28, 2024
10 checks passed
@tarcieri tarcieri deleted the use-core-error-trait branch September 28, 2024 17:21
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
@tarcieri tarcieri mentioned this pull request Sep 28, 2024
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
tarcieri added a commit that referenced this pull request Sep 28, 2024
The MSRVs for dependent crates did not get bumped before #1660 was
merged.

This bumps them accordingly.
tarcieri added a commit that referenced this pull request Sep 30, 2024
As of #1660, the `crypto-common` crate stopped linking `std` entirely by
switching to `core::error::Error`.

That PR didn't remove the `std` feature however, which was retained to
transitively activate the `std` features of `getrandom` and `rand_core`
optioinally in the event their corresponding features are enabled.

This PR goes ahead and removes it entirely, which also unblocks being
able to remove the `std` feature from other crates like `aead`,
`cipher`, `digest`, and `universal-hash`.
tarcieri added a commit that referenced this pull request Sep 30, 2024
As of #1660, the `crypto-common` crate stopped linking `std` entirely by
switching to `core::error::Error`.

That PR didn't remove the `std` feature however, which was retained to
transitively activate the `std` features of `getrandom` and `rand_core`
optioinally in the event their corresponding features are enabled.

This PR goes ahead and removes it entirely, which also unblocks being
able to remove the `std` feature from other crates like `aead`,
`cipher`, `digest`, and `universal-hash`.
tarcieri added a commit that referenced this pull request Sep 30, 2024
As of #1660, the `crypto-common` crate stopped linking `std` entirely by
switching to `core::error::Error`.

That PR didn't remove the `std` feature however, which was retained to
transitively activate the `std` features of `getrandom` and `rand_core`
optioinally in the event their corresponding features are enabled.

This PR goes ahead and removes it entirely, which also unblocks being
able to remove the `std` feature from other crates like `aead`,
`cipher`, `digest`, and `universal-hash`.
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