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

ci: add minimal versions check #1039

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

joshka
Copy link

@joshka joshka commented Mar 31, 2024

Related to #1038

@tarcieri
Copy link
Member

Note that as we're working on prereleases we tend to disable these checks as they're not particularly compatible with things like [patch.crates-io] directives that pull in crates from git

@joshka
Copy link
Author

joshka commented Apr 1, 2024

Note that as we're working on prereleases we tend to disable these checks as they're not particularly compatible with things like [patch.crates-io] directives that pull in crates from git

Makes sense. That said, would you prefer to have a failing check to ignore or a missing check to forget for this?

I've updated the workflow to point at the the standard shared action - feel free to close this though if the idea is incompatible with your general use case. I see that the call to minimal versions is commented out in at least one other repo (with a note to re-enable upon release), perhaps this is an approach that's somewhere in the middle.

@tarcieri
Copy link
Member

tarcieri commented Apr 1, 2024

It will definitely require some tuning as cargo hack is, in cases like this, incredible overkill and generating nearly 10,000 feature combinations (though it seems it is finding some valid build failures, so it's separately something we should generally add)

@joshka
Copy link
Author

joshka commented Apr 1, 2024

It will definitely require some tuning as cargo hack is, in cases like this, incredible overkill and generating nearly 10,000 feature combinations (though it seems it is finding some valid build failures, so it's separately something we should generally add)

Adding an initial cargo test that runs against all features and fails fast might fix that problem, but so too might the -Zdirect-minimal-versions option instead of -Zminimal-versions. The former fails immediately without even attempting to run cargo check. If there's multiple failures, it's indeterminate which specific dependency will cause the process to halt. This is two runs both from within the k256 folder:

❯ cargo +nightly update -Zdirect-minimal-versions
    Updating crates.io index
error: failed to select a version for `proptest`.
    ... required by package `k256 v0.14.0-pre (/Users/joshka/local/elliptic-curves/k256)`
versions that meet the requirements `^1.4` are: 1.4.0

all possible versions conflict with previously selected packages.

  previously selected package `proptest v1.0.0`
    ... which satisfies dependency `proptest = "^1"` of package `bign256 v0.14.0-pre (/Users/joshka/local/elliptic-curves/bign256)`

failed to select a version for `proptest` which could resolve this conflict
❯ cargo +nightly update -Zdirect-minimal-versions
    Updating crates.io index
error: failed to select a version for `criterion`.
    ... required by package `p521 v0.14.0-pre (/Users/joshka/local/elliptic-curves/p521)`
versions that meet the requirements `^0.5.1` are: 0.5.1

all possible versions conflict with previously selected packages.

  previously selected package `criterion v0.5.0`
    ... which satisfies dependency `criterion = "^0.5"` of package `bign256 v0.14.0-pre (/Users/joshka/local/elliptic-curves/bign256)`

failed to select a version for `criterion` which could resolve this conflict

Edit: the cargo test --all-features approach is better here as you get the failing code which is helpful for working out which dependency version is necessary to make the build pass.

@tarcieri
Copy link
Member

tarcieri commented Apr 1, 2024

Yeah, those are the sorts of failures we get during prereleases which lead us to shut these checks off until we're off prerelease versions.

Here's an example of the checks disabled:

https://github.com/RustCrypto/traits/blob/master/.github/workflows/elliptic-curve.yml#L60-L62

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