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

Remove borsh from msrv builder #263

Merged
merged 1 commit into from Mar 12, 2024
Merged

Remove borsh from msrv builder #263

merged 1 commit into from Mar 12, 2024

Conversation

bluss
Copy link
Owner

@bluss bluss commented Mar 11, 2024

Is not supported at msrv anyway (msrv applies to defaults)

Is not supported at msrv anyway (msrv applies to defaults)
Copy link
Collaborator

@tbu- tbu- left a comment

Choose a reason for hiding this comment

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

LGTM.

As the compilation output shows, we'd probably have to commit a Cargo.lock to make sure that the crate continues to compile on the MSRV.

@bluss
Copy link
Owner Author

bluss commented Mar 11, 2024

I see, so serde still fails too.

I think I like specific cargo upgrade --precise calls - for the msrv builder -- instead of using a lockfile?

@bluss bluss changed the title Remove borsch from msrv builder Remove borsh from msrv builder Mar 11, 2024
@tbu-
Copy link
Collaborator

tbu- commented Mar 11, 2024

I think I like specific cargo upgrade --precise calls - for the msrv builder -- instead of using a lockfile?

That works, it doesn't pin dependencies of dependencies though. What do we want to test? Using a lockfile has the advantage of pinning everything, we see when arrayvec breaks something.

The Cargo FAQ explicitly mentions this use case for a Cargo.lock, from https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control:

Having this snapshot of dependencies can also help when projects need to be verified against consistent versions of dependencies, like when

  • Verifying a minimum-supported Rust version (MSRV) that is less than the latest version of a dependency supports

@bluss bluss merged commit f243d43 into master Mar 12, 2024
4 of 5 checks passed
@bluss bluss deleted the msrv branch March 12, 2024 20:49
@bluss
Copy link
Owner Author

bluss commented Mar 12, 2024

Sounds good, if you want to. I merged here since it was at least a step in the right direction

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

2 participants