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

Add minimal_versions CI job #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add minimal_versions CI job #53

wants to merge 1 commit into from

Conversation

ilslv
Copy link
Contributor

@ilslv ilslv commented Dec 14, 2021

Revealed from clap-rs/clap#3172

Actual minimal versions of log is 0.4.4, not 0.4 and 1.0.113 for serde.
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.

Copy link
Contributor

@epage epage left a 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.

Comment on lines 167 to 170
- name: Default features
run: cargo test --workspace
- name: No-default features
run: cargo test --workspace --no-default-features
Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@epage

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.

Copy link
Contributor

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:
Copy link
Contributor

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
Copy link
Contributor

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.

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@ilslv ilslv Dec 15, 2021

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.

Copy link
Contributor

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.

Comment on lines 33 to 36
serde = { version = "1.0.113", features = ["derive"] }
serde_json = "1.0"
once_cell = "1.2.0"
log = "0.4"
log = "0.4.4"
Copy link
Contributor

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" ]
Copy link
Contributor

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?

Copy link
Contributor Author

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

@ilslv ilslv mentioned this pull request Dec 15, 2021
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.

3 participants