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

chore: check minimal versions on CI #596

Merged
merged 1 commit into from
Nov 12, 2024
Merged

chore: check minimal versions on CI #596

merged 1 commit into from
Nov 12, 2024

Conversation

hds
Copy link
Collaborator

@hds hds commented Nov 5, 2024

As reported in issue #592, in our release of console-api 0.8.1, we
specified that we required a tonic version of 0.12 in teh Cargo.toml
file. However, since we had generated Rust code with tonic-build
0.12.3, we had some code that was only present in tonic from 0.12.3
as well.

This error was fixed in #593.

However, this showed a gap in our CI testing, we don't test against
minimum versions of our dependencies to catch this kind of error.

This change adds a CI job to check the minimal versions of our direct
dependencies. We perform a cargo update with the -Z direct-minimal-versions flag and then perform cargo check. This would
have caught the previous error and will catch any new ones of a similar
type.

One downside to this approach is that we must explicitly give a minimum
version for our direct dependencies that is equal to or greater than the
minimum version that any of of transitive dependencies specify for that
same crate. However this check is probably worth the annoyance.

@hds hds requested a review from a team as a code owner November 5, 2024 11:33
@hds hds force-pushed the hds/check-minimal-versions branch 3 times, most recently from a254c47 to 22534d2 Compare November 5, 2024 11:55
As reported in issue #592, in our release of `console-api` 0.8.1, we
specified that we required a tonic version of 0.12 in teh Cargo.toml
file. However, since we had generated Rust code with `tonic-build`
0.12.3, we had some code that was only present in `tonic` from 0.12.3
as well.

This error was fixed in #593.

However, this showed a gap in our CI testing, we don't test against
minimum versions of our dependencies to catch this kind of error.

This change adds a CI job to check the minimal versions of our direct
dependencies. We perform a cargo update with the `-Z
direct-minimal-versions` flag and then perform cargo check. This would
have caught the previous error and will catch any new ones of a similar
type.

One downside to this approach is that we must explicitly give a minimum
version for our direct dependencies that is equal to or greater than the
minimum version that any of of transitive dependencies specify for that
same crate. However this check is probably worth the annoyance.
Copy link
Collaborator

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -25,6 +25,8 @@ env:
RUSTUP_MAX_RETRIES: 10
# Don't emit giant backtraces in the CI logs.
RUST_BACKTRACE: short
# Pin a nightly version for minimal versions
rust_nightly: nightly-2024-05-05
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I ask why we need to pin the nightly version here? Is it to avoid breaking changes from Cargo's resolver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nightly versions may not always work. So we're pinning this to avoid flaky tests depending on the most recent nightly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Thank you!

run: |
# Remove dev-dependencies from Cargo.toml to prevent the next `cargo update`
# from determining minimal versions based on dev-dependencies.
cargo hack --remove-dev-deps --workspace
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is for rust-lang/cargo#5657 (comment)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also just because we don't need to have exact versions for dev dependencies (since it doesn't affect crates that depend on us).

Copy link
Collaborator

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks! 👍

@hds hds merged commit e4e535b into main Nov 12, 2024
19 checks passed
@hds hds deleted the hds/check-minimal-versions branch November 12, 2024 14:58
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.

2 participants