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

Update semver-compatible dependencies #241

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Update semver-compatible dependencies #241

merged 1 commit into from
Apr 5, 2024

Conversation

djc
Copy link
Member

@djc djc commented Apr 1, 2024

To replace #240.

@djc djc requested review from ctz and cpu April 1, 2024 20:52
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.19%. Comparing base (439331b) to head (a2423a0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #241   +/-   ##
=======================================
  Coverage   97.19%   97.19%           
=======================================
  Files          19       19           
  Lines        4065     4065           
=======================================
  Hits         3951     3951           
  Misses        114      114           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc
Copy link
Member Author

djc commented Apr 1, 2024

So this is failing our MSRV checks because aws-lc-sys pulls in bindgen (as an optional build-dependency) and bindgen pulls in regex and regex now has an MSRV of 1.65. It's a bit tricky because AIUI the bindgen dependency is optional for aws-lc-sys on platforms for which they have pregenerated bindings, which seems to be all the tier 1 ones at this point?

We could avoid updating regex in our Cargo.lock to prove to ourselves that the library otherwise still works on 1.60 but downstream users (using the aws-lc-rs provider) will probably still think we no longer support 1.60?

@ctz
Copy link
Member

ctz commented Apr 2, 2024

I wonder if we should take 1.65? It'd mean we can use let-else, for example. The downside of that is debian stable is on 1.63.

@djc
Copy link
Member Author

djc commented Apr 2, 2024

It also feels silly because it looks like regex is not really a core dependency for bindgen, it's used to provide what looks like potentially optional API (which it seems aws-lc-rs doesn't actually use)...

@cpu
Copy link
Member

cpu commented Apr 2, 2024

The downside of that is debian stable is on 1.63.

I wonder how long we'd have to wait for Debian to get 1.65 🤔

@djc
Copy link
Member Author

djc commented Apr 2, 2024

The downside of that is debian stable is on 1.63.

I wonder how long we'd have to wait for Debian to get 1.65 🤔

I think for the last bump they went from 1.48 to 1.63. I don't think we'll want to wait that long...

@cpu
Copy link
Member

cpu commented Apr 5, 2024

cpu force-pushed the update-deps branch from 492824e to ec2bf76

I force pushed an update to pull in aws-lc-rs 1.6.4 since the 1.6.3 release with the bindgen issue was yanked.

error: package rustix v0.38.32 cannot be built because it requires rustc 1.63 or newer, while the currently active rustc version is 1.61.0

It looks like we might still need to consider an MSRV bump, but to a much more reasonable 1.63. That's in Debian stable, and we were talking about taking it already over in rustls/rustls#1885 for hashbrown. I think we should do it. Agree? This was confusion from forgetting to update the locked aws-lc-sys dep too.

@ctz
Copy link
Member

ctz commented Apr 5, 2024

I think we should do it. Agree?

Yep!

@cpu
Copy link
Member

cpu commented Apr 5, 2024

Ah, should have tried 1.63 as MSRV first:

error: package regex-automata v0.4.6 cannot be built because it requires rustc 1.65 or newer, while the currently active rustc version is 1.63.0

It looks like a bindgen build dep is still bringing in regex 1.10.4. I see the MSRV build fail w/ 1.63. This was confusion from forgetting to update the locked aws-lc-sys dep too.

@cpu
Copy link
Member

cpu commented Apr 5, 2024

I'm confused why rustls/rustls#1888 builds green with an MSRV of 1.61 given the above. I'm missing some detail 🤔

@cpu
Copy link
Member

cpu commented Apr 5, 2024

I'm missing some detail 🤔

It looks like Rustls is using aws-lc-sys 0.14.1 while webpki is using 0.14.0

rustls dep tree
rustls v0.23.4 (/home/daniel/Code/Rust/rustls/rustls)
├── aws-lc-rs v1.6.4
│   ├── aws-lc-sys v0.14.1
│   │   ├── libc v0.2.153
│   │   └── paste v1.0.14 (proc-macro)
│   │   [build-dependencies]
│   │   ├── cmake v0.1.50
│   │   │   └── cc v1.0.86
│   │   ├── dunce v1.0.4
│   │   └── fs_extra v1.3.0
│   ├── mirai-annotations v1.12.0
│   ├── paste v1.0.14 (proc-macro)
│   └── zeroize v1.7.0
<snipped>

vs

rustls-webpki dep tree
rustls-webpki v0.102.2 (/home/daniel/Code/Rust/webpki)
├── aws-lc-rs v1.6.4
│   ├── aws-lc-sys v0.14.0
│   │   ├── libc v0.2.153
│   │   └── paste v1.0.14 (proc-macro)
│   │   [build-dependencies]
│   │   ├── bindgen v0.69.4
│   │   │   ├── bitflags v2.5.0
│   │   │   ├── cexpr v0.6.0
│   │   │   │   └── nom v7.1.3
│   │   │   │       ├── memchr v2.7.2
│   │   │   │       └── minimal-lexical v0.2.1
│   │   │   ├── clang-sys v1.7.0
│   │   │   │   ├── glob v0.3.1
│   │   │   │   ├── libc v0.2.153
│   │   │   │   └── libloading v0.8.3
│   │   │   │       └── cfg-if v1.0.0
│   │   │   │   [build-dependencies]
│   │   │   │   └── glob v0.3.1
│   │   │   ├── itertools v0.12.1
│   │   │   │   └── either v1.10.0
│   │   │   ├── lazy_static v1.4.0
│   │   │   ├── lazycell v1.3.0
│   │   │   ├── log v0.4.21
│   │   │   ├── prettyplease v0.2.17
│   │   │   │   ├── proc-macro2 v1.0.79
│   │   │   │   │   └── unicode-ident v1.0.12
│   │   │   │   └── syn v2.0.57
│   │   │   │       ├── proc-macro2 v1.0.79 (*)
│   │   │   │       ├── quote v1.0.35
│   │   │   │       │   └── proc-macro2 v1.0.79 (*)
│   │   │   │       └── unicode-ident v1.0.12
│   │   │   ├── proc-macro2 v1.0.79 (*)
│   │   │   ├── quote v1.0.35 (*)
│   │   │   ├── regex v1.10.4
│   │   │   │   ├── regex-automata v0.4.6
│   │   │   │   │   └── regex-syntax v0.8.3
│   │   │   │   └── regex-syntax v0.8.3
│   │   │   ├── rustc-hash v1.1.0
│   │   │   ├── shlex v1.3.0
│   │   │   ├── syn v2.0.57 (*)
│   │   │   └── which v4.4.2
│   │   │       ├── either v1.10.0
│   │   │       ├── home v0.5.9
│   │   │       └── rustix v0.38.32
│   │   │           ├── bitflags v2.5.0
│   │   │           └── linux-raw-sys v0.4.13
│   │   ├── cmake v0.1.50
│   │   │   └── cc v1.0.90
│   │   ├── dunce v1.0.4
│   │   └── fs_extra v1.3.0
│   ├── mirai-annotations v1.12.0
│   ├── paste v1.0.14 (proc-macro)
│   └── zeroize v1.7.0

@djc
Copy link
Member Author

djc commented Apr 5, 2024

Maybe because aws-lc-rs has fixed their issue in 1.6.3 in 1.6.4?

@ctz
Copy link
Member

ctz commented Apr 5, 2024

I can repro that locally, but then if I do:

$ cargo update
    Updating crates.io index
    Updating aws-lc-sys v0.14.0 -> v0.14.1
    Updating syn v2.0.57 -> v2.0.58

That fixes it (because aws-lc-rs -> aws-lc-sys -> build-dependencies drops bindgen).

@cpu
Copy link
Member

cpu commented Apr 5, 2024

Maybe because aws-lc-rs has fixed their issue in 1.6.3 in 1.6.4?

I'm using aws-lc-rs 1.6.4 in both, but aws-lc-sys didn't get bumped in the lockfile here where it did in the main repo.

@cpu
Copy link
Member

cpu commented Apr 5, 2024

Ok, all fixed up. No MSRV change required.

Sorry for the confusion!

@cpu
Copy link
Member

cpu commented Apr 5, 2024

@ctz @djc Can one of you re-review since I pushed changes to this branch?

@ctz ctz enabled auto-merge April 5, 2024 16:04
@ctz ctz added this pull request to the merge queue Apr 5, 2024
@cpu
Copy link
Member

cpu commented Apr 5, 2024

slow

I cancelled this job that looked stuck and started it again.

Merged via the queue into main with commit 97f0364 Apr 5, 2024
58 checks passed
@ctz ctz deleted the update-deps branch April 5, 2024 16:41
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.

None yet

3 participants