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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

name: Minimal versions
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

rust: [ "1.54.0" ] # MSRV
continue-on-error: ${{ matrix.rust != 'stable' }}
runs-on: ${{ matrix.os }}
steps:
- name: Checkout repository
uses: actions/checkout@v2
- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: nightly
- uses: actions-rs/toolchain@v1
with:
profile: minimal
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.

Copy link

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 •
edited
Loading

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.

- uses: Swatinem/rust-cache@v1
- name: Default features
run: cargo test --workspace
- name: No-default features
run: cargo test --workspace --no-default-features
Comment on lines +167 to +170
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

4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ pre-release-replacements = [
]

[dependencies]
serde = { version = "1.0", features = ["derive"] }
serde = { version = "1.0.113", features = ["derive"] }
serde_json = "1.0"
once_cell = "1.2.0"
log = "0.4"
log = "0.4.4"
Comment on lines +33 to +36
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


[dev-dependencies]
assert_fs = "1.0"
Expand Down