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

Bump msrv and ensure tests work with msrv #590

Closed
wants to merge 3 commits into from

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented Dec 11, 2024

What changes are proposed in this pull request?

cargo msrv doesn't seem to be able to check tests, so we add a new workflow to make sure cargo test at least works with our rust-version.

Incidentally, we need to bump to 1.81.0 for a test.

How was this change tested?

Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

left a question but LGTM

Comment on lines +48 to +54
- name: Install minimal stable and cargo msrv
uses: actions-rs/toolchain@v1
with:
profile: default
toolchain: stable
override: true
- uses: Swatinem/rust-cache@v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI @roeap will need to make his changes over here too :)

.github/workflows/build.yml Show resolved Hide resolved
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.44%. Comparing base (be1453f) to head (54a967c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #590      +/-   ##
==========================================
- Coverage   83.46%   83.44%   -0.03%     
==========================================
  Files          74       74              
  Lines       16858    16858              
  Branches    16858    16858              
==========================================
- Hits        14071    14067       -4     
- Misses       2129     2131       +2     
- Partials      658      660       +2     

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

@zachschuermann
Copy link
Collaborator

also heads up looks like new job failed to get the version for toolchain:

error: error: invalid value '$(cargo msrv show --path kernel/ --output-format minimal)' for '<toolchain>...': invalid toolchain name: '$(cargo msrv show --path kernel/ --output-format minimal)'

@nicklan
Copy link
Collaborator Author

nicklan commented Dec 11, 2024

also heads up looks like new job failed to get the version for toolchain:

error: error: invalid value '$(cargo msrv show --path kernel/ --output-format minimal)' for '<toolchain>...': invalid toolchain name: '$(cargo msrv show --path kernel/ --output-format minimal)'

Yeah. I'll request when it's ready. I expect to fight with actions a bit before I get this working

@zachschuermann
Copy link
Collaborator

also heads up looks like new job failed to get the version for toolchain:

error: error: invalid value '$(cargo msrv show --path kernel/ --output-format minimal)' for '<toolchain>...': invalid toolchain name: '$(cargo msrv show --path kernel/ --output-format minimal)'

Yeah. I'll request when it's ready. I expect to fight with actions a bit before I get this working

haha sounds good - sorry i jumped the gun a bit

Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

restamp

@nicklan nicklan added the breaking-change Change that will require a version bump label Dec 11, 2024
@nicklan
Copy link
Collaborator Author

nicklan commented Dec 13, 2024

closing in favor of #596

@nicklan nicklan closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that will require a version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants