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 dependencies which do not bump minimum rustc #3359

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

shssoichiro
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (3c3a26f) to head (025a405).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3359      +/-   ##
==========================================
+ Coverage   88.52%   88.54%   +0.01%     
==========================================
  Files          89       89              
  Lines       28323    28355      +32     
==========================================
+ Hits        25074    25107      +33     
+ Misses       3249     3248       -1     

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

Cargo.lock Outdated
@@ -251,15 +260,15 @@ dependencies = [

[[package]]
name = "bumpalo"
version = "3.14.0"
version = "3.15.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the WASM build. Bumpalo 3.15.0 changed the MSRV to 1.73.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would be really happy if more Rust maintainers would consider a compiler version change as breaking... anyway I guess I'll add a CI step for wasm, looks like we're missing them.

Copy link
Contributor

Choose a reason for hiding this comment

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

MSRV bump being considered semver breaking or not seems to be a very contentious issue..
It would be a lot better if cargo update honored the MSRV, I think a lot of issues would be inherently solved by that

@FreezyLemon
Copy link
Contributor

BTW I recommend using cargo-hack to validate MSRV (and other things):
cargo +1.70 hack --each-feature check

Might be worth adding something to CI, although a job like that would take very long to complete...

@shssoichiro
Copy link
Collaborator Author

We do have CI jobs for validating minimum rust version. Just not one for wasm currently. hack might be useful for local testing before pushing up to CI.

@shssoichiro shssoichiro force-pushed the update-deps branch 2 times, most recently from 0be80a3 to 29f9e4f Compare March 12, 2024 01:03
@shssoichiro
Copy link
Collaborator Author

It looks like adding wasm tests in CI here may be a bit more complicated than in v_frame. But I'm also not an expert at Github actions. Anyway, for now I'll create an issue to follow up on adding wasm tests to CI, so we don't forget it.

Copy link
Contributor

@FreezyLemon FreezyLemon left a comment

Choose a reason for hiding this comment

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

Everything seems to build fine on 1.70 now.

@FreezyLemon
Copy link
Contributor

On second thought, maybe add a comment in the Cargo.toml that clap is held back for MSRV reasons?

@lu-zero
Copy link
Collaborator

lu-zero commented Aug 14, 2024

It should be freshened and landed... Somehow the notification got lost.

@lu-zero
Copy link
Collaborator

lu-zero commented Aug 14, 2024

Meh, let's bump to 1.74 and be done with that :)

@shssoichiro shssoichiro merged commit 2c51344 into xiph:master Aug 14, 2024
24 of 25 checks passed
@FreezyLemon FreezyLemon mentioned this pull request Sep 14, 2024
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