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

Refactor CI msrv & minver checks #2238

Merged
merged 7 commits into from
Jul 27, 2022
Merged

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jul 20, 2022

Motivation

Test msrv and minimal-versions compliance in the currently best-known fashion.

Solution

  • Use cargo-hack and cargo-minimal-versions
  • Also I replaced uses of actions-rs (abandoned) while I was passing by

@CAD97 CAD97 requested review from hawkw, davidbarsky and a team as code owners July 20, 2022 19:53
@CAD97 CAD97 force-pushed the actions-rs-rip branch 2 times, most recently from 6824022 to 452e45d Compare July 20, 2022 20:25
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

one question about this change, just to make sure I understand the motivation here.

as a side note, i'm not super excited about how all these files were re-formatted, i'm now concerned that any subsequent modifications i make will reformat them differently...perhaps we ought to check in an .editorconfig for yaml files or something, to ensure different people's editors don't reformat them as often. but, it's not a huge deal.

Comment on lines 89 to 116
minimal-versions:
# Check for minimal-versions errors where a dependency is too
# underconstrained to build on the minimal supported version of all
# dependencies in the dependency graph.
name: cargo check (-Zminimal-versions)
needs: check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions-rs/toolchain@v1
- uses: dtolnay/rust-toolchain@stable
with:
toolchain: nightly
profile: minimal
override: true
- name: install cargo-hack
uses: taiki-e/install-action@cargo-hack
- name: "check --all-features -Z minimal-versions"
run: |
# Remove dev-dependencies from Cargo.toml to prevent the next `cargo update`
# from determining minimal versions based on dev-dependencies.
cargo hack --remove-dev-deps --workspace
# Update Cargo.lock to minimal version dependencies.
cargo update -Z minimal-versions
cargo hack check \
--package tracing \
--package tracing-core \
--package tracing-subscriber \
--all-features --ignore-private
Copy link
Member

Choose a reason for hiding this comment

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

what happened to the minimal-versions job? it seems to have disappeared --- is this because we think the +MSRV -Zminimal-versions run is sufficient to ensure minver compatibility? do we no longer need to check it on stable as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the theory is that checking +MSRV -Zminimal-versions is sufficient, and that standard backwards-compatibility guarantees imply that +stable -Zminimal-versions will also work. This isn't guaranteed, but we can have fair confidence since we're testing stable with normal maximal version resolution.

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 21, 2022

Unfortunately the change here I think cannot be covered in an editorconfig; it's changing

# from
---
key:
- list
- of
- values

# to
---
key:
  - list
  - of
  - values

and short of switching to flow syntax everywhere, there doesn't seem to be a standard1 "use this option" config file. Interestingly, in a quick search, nearly every example uses the second form except for the official YAML spec.

Either way, we probably should normalize to one option or the other; most of our YAML is using shallow lists but not all. I went ahead and cleared that change out in a rebase to avoid making that call here.

Footnotes

  1. We could add a vscode workspace configuration for @ext:redhat.vscode-yaml to disable the formatter, which would cover the 90% case probably. I checked; it doesn't appear to have a setting for "shallow" list indentation.

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 21, 2022

What this doesn't check is that --no-default-features also checks on +MSRV minimal-versions... doing a full --feature-powerset1 is probably overkill but it seems valuable to at least check without optional dependencies to remove their version constraints on the kept dependencies.

[ci run I'm looking at]

That would necessitate another CI matrix split like in the existing feature powerset job. Given the cargo-msrv job is nearly as long as a build and can't really benefit from a cache the way the actual builds can, perhaps this would be a good idea anyway... the longest single-crate feature powerset check is a minute, whereas this job is nearly three. Caching got the tests down to under four minutes on linux.

Footnotes

  1. Is just powersetting optional-deps and not other features a) possible in cargo-hack and b) sufficient?

@hawkw
Copy link
Member

hawkw commented Jul 21, 2022

Okay, the YAML change isn't really a big deal, and I'm fine with whatever style we end up using as long as my vscode install doesn't automatically reformat everything as soon as I open a config. :)

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 22, 2022

I added the matrix jobs, it's your call whether to keep them or not.

previous ci ran in 12:30; this ci ran in 14:00. A cursory overview suggests that each matrix job has about 20s of overhead compared to running that check in a shared job.

(As far as CI time optimization goes, though, the big win would be caching dependency compilation on the test runners.)

@hawkw
Copy link
Member

hawkw commented Jul 27, 2022

I added the matrix jobs, it's your call whether to keep them or not.

previous ci ran in 12:30; this ci ran in 14:00. A cursory overview suggests that each matrix job has about 20s of overhead compared to running that check in a shared job.

(As far as CI time optimization goes, though, the big win would be caching dependency compilation on the test runners.)

Hmm, the extra 1:30 or so of runtime is kind of a bummer, but I'm going to move forward with this change regardless, since it cleans up a bunch of stuff that I'd like to get fixed up. Let's try adding caching in a subsequent branch maybe?

@CAD97
Copy link
Contributor Author

CAD97 commented Jul 27, 2022

Sounds good to me.

@hawkw hawkw enabled auto-merge (squash) July 27, 2022 23:12
@hawkw hawkw merged commit bfd0e55 into tokio-rs:master Jul 27, 2022
@CAD97 CAD97 deleted the actions-rs-rip branch July 27, 2022 23:44
davidbarsky added a commit that referenced this pull request Nov 7, 2023
davidbarsky added a commit that referenced this pull request Nov 7, 2023
davidbarsky added a commit that referenced this pull request Nov 7, 2023
hawkw pushed a commit that referenced this pull request Nov 7, 2023
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request Feb 14, 2024
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
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