Add minimal_versions CI job
#53
Conversation
| - name: Default features | ||
| run: cargo test --workspace | ||
| - name: No-default features | ||
| run: cargo test --workspace --no-default-features |
There was a problem hiding this comment.
I think I'd rather these be check than test
Like with MSRV, I'm assuming 99% of the time, a minimal version issue arises from an incompatibility with the API rather than a behavior change.
You have more experience with this though, so let me know if you've been seeing significant issues with relying on new behavior with the same APIs
There was a problem hiding this comment.
Another benefit to only doing check (rather than test or check --all-targets) is it means we don't have to block this on dev dependencies (granted, I maintain the only dev-dependency in this crate)
There was a problem hiding this comment.
Like with MSRV, I'm assuming 99% of the time, a minimal version issue arises from an incompatibility with the API rather than a behavior change.
Yeah, but there is still that 1% when your minimal crate version is some bugfix, that is tested inside the crate.
You have more experience with this though, so let me know if you've been seeing significant issues with relying on new behavior with the same APIs
In my experience this helped 1 or 2 times.
But I think, that if we can automate something to avoid potential problems, we should do this all in. Tradeoff here is maybe somewhat slower CI, but all jobs run in parallel and additional wait time is just waiting in the queue for jobs to start.
There was a problem hiding this comment.
But I think, that if we can automate something to avoid potential problems, we should do this all in. Tradeoff here is maybe somewhat slower CI, but all jobs run in parallel and additional wait time is just waiting in the queue for jobs to start.
So the impact here is less but in clap, we were doing the "all in" approach before and the CI took hours to run. Some jobs alone took 30+ minutes to run. Another problem is that parallelism is limited; Github limits the number of concurrent runners and I believe its global to the org the repo is in. I also feel we should be considerate of the resources Github is giving to us for free.
We then switched to a two-tier CI system where we checked fewer things on to give a green light for the PR but then bors would run the full set on merge. This still took somewhere between 30-60 minutes to run. We eventually dropped the "all in" approach and CI gives us nearly the same coverage for a fraction of the time, focusing on the likely problems
| @@ -140,3 +140,31 @@ jobs: | |||
| with: | |||
| token: ${{ secrets.GITHUB_TOKEN }} | |||
| args: --workspace --all-features --all-targets -- -D warnings | |||
| minimal_versions: | |||
There was a problem hiding this comment.
For PRs to be blocked on this, you need to update the ci job to need this
| toolchain: ${{ matrix.rust }} | ||
| override: true | ||
| - name: Downgrade crates to minimal supported versions | ||
| run: cargo +nightly update -Z minimal-versions |
There was a problem hiding this comment.
I've heard a good number of horror stories from dependencies not testing this, breaking people who do want this.
I'm uncomfortable committing to this unless all dependencies being used by this job either are also testing this or explicitly document as a contract that they are zero-dependency crates.
There was a problem hiding this comment.
@epage but why? You will hit this either way from downstream crates, sooner or later, like this time has happened. It's only a matter of catching regressions earlier.
There was a problem hiding this comment.
Keep in mind I'm going off of vague memory of comments I've seen on reddit. I remember reading of some people (like burntsushi?) trying to use this but core Rust crate authors were refusing to verify it, making it a minefield for dependents to verify and it not break them.
In breaking this down, nothing breaks until I upgrade my minimal versions, unlike MSRV. The downside is when I do upgrade, it becomes a chore of chasing through my dependency chain to find all of the issues. This becomes an impediment to doing what should be a basic operation.
Without my dependencies also being on board, I'm not seeing how the benefits outweigh the costs.
There was a problem hiding this comment.
I'm uncomfortable committing to this unless all dependencies being used by this job either are also testing this or explicitly document as a contract that they are zero-dependency crates.
In case some of your transitive dependencies have too low minimal version, you can just specify them in Cargo.toml, while not using in the crate by yourself, so I don't see where the horror stories can come from.
There was a problem hiding this comment.
Having to micro-manage this might not sound too big of a deal to you but it can be a deal to others, especially when they are managing a significant number of crates.
As I said elsewhere, we can move forward with the dependency updates as we work through how to handle the workflows. We can decouple the fix from the effort of either trying to get buy-in from dependencies or finding a compromise for how to handle CI.
| serde = { version = "1.0.113", features = ["derive"] } | ||
| serde_json = "1.0" | ||
| once_cell = "1.2.0" | ||
| log = "0.4" | ||
| log = "0.4.4" |
There was a problem hiding this comment.
If you want to break this out into its own PR, I'm willing to merge it without waiting on the other parts
| needs: smoke | ||
| strategy: | ||
| matrix: | ||
| os: [ "ubuntu-latest", "windows-latest", "macos-latest" ] |
There was a problem hiding this comment.
Like with check vs test, do we need all platforms or just one?
There was a problem hiding this comment.
Yeah, but there is still that 1% when your minimal crate version is some bugfix, that is tested inside the crate.
Theoretically in this 1% maybe itself 1% of cases where this bugfix is OS-dependent
Revealed from clap-rs/clap#3172
Actual minimal versions of
logis0.4.4, not0.4and1.0.113forserde.To avoid similar issues, I've also added minimal_versions CI job, which downgrades all crates to minimal compatible versions and runs tests on them. Although it's nightly feature for cargo, we use it throughout multiple projects and never had any issues with it.