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

Fail CI if lock file isn't updated. #1042

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

nihohit
Copy link
Contributor

@nihohit nihohit commented Jan 29, 2024

No description provided.

@nihohit nihohit requested a review from jaymell January 29, 2024 10:15
@@ -6,7 +6,7 @@ test:
@echo "===================================================================="
@echo "Testing Connection Type TCP without features"
@echo "===================================================================="
@RUSTFLAGS="-D warnings" REDISRS_SERVER_TYPE=tcp RUST_BACKTRACE=1 cargo test -p redis --no-default-features -- --nocapture --test-threads=1
@RUSTFLAGS="-D warnings" REDISRS_SERVER_TYPE=tcp RUST_BACKTRACE=1 cargo test --locked -p redis --no-default-features -- --nocapture --test-threads=1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand what --locked does in the cargo test context. Should we consider doing this in the other runs as well? If we don't, are we still going to potentially hit the same compatibility issues with older Rust versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/cargo/commands/cargo-build.html#manifest-options

--frozen
--locked
Either of these flags requires that the Cargo.lock file is up-to-date. If the lock file is missing, or it needs to be updated, Cargo will exit with an error. The --frozen flag also prevents Cargo from attempting to access the network to determine if it is out-of-date.
These may be used in environments where you want to assert that the Cargo.lock file is up-to-date (such as a CI build) or want to avoid network access.

It does the same thing in cargo test as in cargo build - fail the build if the lock file is out of date.
I thought it makes sense to put it in the tests, since that will run on the oldest Rust version. We can move it to a separate step, if you prefer it out of the makefile.

Should we consider doing this in the other runs as well?

Not sure which runs are you referring to. In the makefile we hit 8 out of 9 CI workflows. Would you like this also in the lint workflow? That workflow is only tested on the most recent Rust, AFAIK.

are we still going to potentially hit the same compatibility issues with older Rust versions?

I don't think so, since this makefile is called from the workflows with the older rust version matrix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I mean the specific test invocations within the Makefile. The first one -- "Testing Connection Type TCP without features" -- doesn't test any of the features. I was thinking that we're more likely to have issues w/ the subsequent invocations that explicitly use --all-features and bring in all the additional dependencies. But maybe it doesn't matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, that's a good point. Maybe we'll just start with cargo build --locked --all-features?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me 👍

@nihohit
Copy link
Contributor Author

nihohit commented Feb 1, 2024

@jaymell round

@jaymell
Copy link
Contributor

jaymell commented Feb 1, 2024

Again, this could just be flawed understanding, but if we don't run each iteration of the tests in the Makefile with --locked, are we not still likely to encounter an issue related to unlocked dependency versions bumping MSRV?

EDIT: To be clear, what I'm asking is, should we add --locked to each test iteration in the Makefile?

@nihohit
Copy link
Contributor Author

nihohit commented Feb 1, 2024

I'm not sure that it's possible, but adding the flag doesn't cost us money, so why not?

@nihohit nihohit merged commit 4c9924e into redis-rs:main Mar 7, 2024
10 checks passed
@nihohit nihohit deleted the lock-upstream branch March 19, 2024 04:46
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