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

feat: update to rust edition 2021 #135

Merged
merged 7 commits into from
Mar 10, 2022
Merged

feat: update to rust edition 2021 #135

merged 7 commits into from
Mar 10, 2022

Conversation

dignifiedquire
Copy link
Member

bumps MSRV to 1.56.0

@newpavlov
Copy link
Member

It's also worth to add rust-version = "1.56" field as well.

@dignifiedquire
Copy link
Member Author

I wonder why nightly CI is unhappy now

@paolobarbolini
Copy link

Wih this MSRV bump it should now be possible to relax the zeroize version requirement

@tarcieri
Copy link
Member

I wonder why nightly CI is unhappy now

The nightly feature of the subtle crate is using a feature which has been stabilized in 1.56:

error[E0557]: feature has been removed
  --> /Users/bascule/.cargo/registry/src/github.com-1ecc6299db9ec823/subtle-2.4.0/src/lib.rs:12:42
   |
12 | #![cfg_attr(feature = "nightly", feature(external_doc))]
   |                                          ^^^^^^^^^^^^ feature has been removed
   |
   = note: use #[doc = include_str!("filename")] instead, which handles macro invocations

Note that the nightly feature of subtle doesn't provide much value anymore, so it might be good not to activate it, particularly since it doesn't compile on recent nightlies anymore.

@dignifiedquire dignifiedquire force-pushed the edition-2021 branch 2 times, most recently from fc93a5a to a736919 Compare January 17, 2022 13:58
@tarcieri
Copy link
Member

Wow that's really weird... almost feels like a nightly bug?

@dignifiedquire maybe try bumping num-bigint-dig to the 2021 edition first?

@poliorcetics
Copy link
Contributor

CI gets the following error:

 error[E0599]: no function or associated item named `deserialize` found for struct `Vec<_, _>` in the current scope
Error:     --> /cargo/registry/src/github.com-1ecc6299db9ec823/num-bigint-dig-0.7.0/src/biguint.rs:2582:35
     |
2582 |         let data: Vec<u32> = Vec::deserialize(deserializer)?;
     |                                   ^^^^^^^^^^^ function or associated item not found in `Vec<_, _>`

I'm not managing to reproduce it on num-bigint, tag v0.7.0, with all the features used by cg +nightly build --features=i128,nightly,prime,rand,serde,u64_digit,zeroize but compiling this PR with cargo +nightly build --all-features does have the error.

Nightly version: cargo 1.60.0-nightly (358e79fe5 2022-01-04)

@poliorcetics
Copy link
Contributor

Taking this PR and changing the edition to 2018 works, this is very probably a rustc bug due to num-bigint(-dig) not setting an edition in Cargo.toml (so defauting to 2015). This probably cannot be merged without num-bigint moving to 2018 or 2021 edition first.

tarcieri added a commit to tarcieri/num-bigint that referenced this pull request Feb 1, 2022
Upgrades the edition, with changes primarily performed by
`cargo fix --edition`.

In order to keep the changes to a minimum, this doesn't include
automated idiom fixes (i.e. `cargo fix --edition-idioms`).

This is needed to unblock the edition upgrade for the RSA crate:
RustCrypto/RSA#135
@tarcieri
Copy link
Member

tarcieri commented Feb 1, 2022

I opened a PR to bump the edition for num-bigint-dig to 2021: dignifiedquire/num-bigint#34

@poliorcetics
Copy link
Contributor

Asked for a 0.8 update for num-bigint-dig so it can be updated in this PR and complete it

@paolobarbolini
Copy link

paolobarbolini commented Feb 18, 2022

num-bigint-dig 0.8 is out 🥳!
https://crates.io/crates/num-bigint-dig

@dignifiedquire
Copy link
Member Author

unfortunately this did not fix the rustc bug :'(

@paolobarbolini
Copy link

paolobarbolini commented Feb 18, 2022

@paolobarbolini
Copy link

paolobarbolini commented Feb 18, 2022

I can see it was enabled previously 👀. Must have been caused by resolver = 2

https://github.com/RustCrypto/RSA/runs/5036633905?check_suite_focus=true#step:4:274

@paolobarbolini
Copy link

Confirmed by running cargo check --no-default-features --features i128,u64_digit,prime,zeroize,serde and cargo tree --edges normal --format "{p} {f}" --no-default-features --features i128,u64_digit,prime,zeroize,serde on num-bigint-dig

@newpavlov
Copy link
Member

It could be worth to add the minimal versions job to CI.

minimal-versions:
uses: RustCrypto/actions/.github/workflows/minimal-versions.yml@master
with:
working-directory: ${{ github.workflow }}
Copy link
Member

@newpavlov newpavlov Mar 1, 2022

Choose a reason for hiding this comment

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

Argh, unfortunately, you can not use the shared workflow, since it targets multi-crate repos. You can use the following config instead:

  minimal-versions:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: RustCrypto/actions/cargo-cache@master
      - uses: actions-rs/toolchain@v1
        with:
          toolchain: nightly
          override: true
          profile: minimal
      - uses: RustCrypto/actions/cargo-hack-install@master
      - run: cargo update -Z minimal-versions
      - run: cargo hack test --release --feature-powerset

Also while we are at it, it could be useful to add cargo cache to other RSA jobs to reduce CI run time. I can submit a separate PR, if you do not want to do it right now.

@poliorcetics
Copy link
Contributor

CI improvement should be delayed to another MR, this is blocking a new release of RSA which in turn blocks the whole ecosystem that uses both RSA and another crate that has made the jump to a recent version of hashes.

I can take the commits except the last one and put them in another MR to move forward if needed.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

I tried @newpavlov's minimal-versions config. It looks like num-bigint-dig v0.8.1 is not configured with the correct versions for that to work: https://github.com/RustCrypto/RSA/runs/5497433460?check_suite_focus=true

So I agree it would make sense to get this PR merged first then circle back on making num-bigint-dig minimal-versions correct and then following up on minimal-versions CI in a separate PR.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member

I am fine with leaving the CI changes for later, but I think the minimal versions build should be fixed before we release rsa v0.6.

@tarcieri tarcieri merged commit 190a8ec into master Mar 10, 2022
@tarcieri tarcieri deleted the edition-2021 branch March 10, 2022 18:25
@tarcieri
Copy link
Member

FYI for anyone particularly blocked here: I just cut a v0.6.0-pre release with these changes

https://crates.io/crates/rsa/0.6.0-pre

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.

5 participants