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

minimal-versions: run tests using stable Rust #33

Merged
merged 3 commits into from
Nov 11, 2023

Conversation

tarcieri
Copy link
Member

Adds a configurable toolchain parameter to the reusable workflow for minimal-versions which allows a different toolchain than nightly to be used for the purposes of running tests.

nightly is only needed for the -Z minimal-versions resolution of Cargo.lock. We can avoid nightly breakages of which we've hit quite a few lately by reducing the scope of what we use nightly for to just that version resolution.

Additionally, this moves the resolution of Cargo.lock to earlier in the workflow so it applies to all steps. Previously benchmarks were being tested before the -Z minimal-versions resolution took place.

Adds a configurable `toolchain` parameter to the reusable workflow for
minimal-versions which allows a different toolchain than `nightly` to be
used for the purposes of running tests.

`nightly` is only needed for the `-Z minimal-versions` resolution of
Cargo.lock. We can avoid nightly breakages of which we've hit quite a
few lately by reducing the scope of what we use `nightly` for to *just*
that version resolution.

Additionally, this moves the resolution of Cargo.lock to earlier in the
workflow so it applies to all steps. Previously benchmarks were being
tested before the `-Z minimal-versions` resolution took place.
@tarcieri
Copy link
Member Author

FWIW I think using cargo hack in this workflow is overkill and means it can't be used with crates that have a large number of features due to the combinatorial explosion.

I think we could get by with just testing a few different feature combinations like --no-default-features, default features, and --all-features

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

FWIW I think using cargo hack in this workflow is overkill and means it can't be used with crates that have a large number of features due to the combinatorial explosion.

It may be worth to make the test command a configurable parameter as well, with the cargo hack test being the default.

.github/workflows/minimal-versions.yml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member Author

tarcieri commented Nov 3, 2023

It may be worth to make the test command a configurable parameter as well, with the cargo hack test being the default.

I feel like cargo hack requires a lot of maintenance and planning, where the purpose of this workflow is merely to ensure that the library works with minimal dependencies. Honestly I feel like cargo hack is major overkill here, and even cargo build or even cargo check would be sufficient.

I've been dealing with many, many subprojects that have excessively long minimal-versions workflows, and in many cases simply abandoned the shared action for a more minimalist check, which seems to defeat the point. The default should be easy-to-use and cover all cases. In many of these cases minimal-versions is the last job to complete, by far, and just clogging up CI workflows.

@newpavlov
Copy link
Member

--each-feature being the default may address your concerns. I think we should test all features, but you are right that testing all combinations in a minimal versions job is probably a tad excessive.

@tarcieri tarcieri merged commit 371d627 into master Nov 11, 2023
1 check passed
@tarcieri tarcieri deleted the minimal-versions/run-tests-using-stable branch November 11, 2023 23:25
@tarcieri
Copy link
Member Author

Ran into another seemingly nightly-related failure, so hopefully this fixes it: https://github.com/RustCrypto/traits/actions/runs/6837021752/job/18592552010

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