-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add minimal_versions
CI job
#53
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working to get the ecosystem onto minimal version testing!
As I note below, I am hesitant without my dependencies also committing to it.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like with check
vs test
, do we need all platforms or just one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
log
is0.4.4
, not0.4
and1.0.113
forserde
.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.