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 a minimal-versions check #2015

Merged
merged 8 commits into from Apr 8, 2022
Merged

Add a minimal-versions check #2015

merged 8 commits into from Apr 8, 2022

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Mar 24, 2022

This adds a minimal-versions check to the tracing project. Adapted from tokio-rs/tokio. Adding this avoids breaking downstream dependencies from accidentally under-constraining minimal versions of dependencies when they depend on tracing.

I've currently just introduced the check. I will try to and do encourage others to add patches to fix this where possible since it can be a fair bit of work to chase down a version of all dependencies that passes minimal-versions and is msrv. I've also seen some really odd windows-specific issues (which are not being tested for here).

This is currently only testing tracing, tracing-core, and tracing-subscriber. Packages such as tracing-futures are proving to be a bit harder to deal with due to having features which enable very old dependencies.

Steps to test the build minimal versions locally:

cargo install cargo-hack
rustup default nightly
cargo hack --remove-dev-deps --workspace
cargo update -Z minimal-versions
cargo hack check --all-features --ignore-private

CC: tokio-rs/tokio#4513

@udoprog udoprog requested review from hawkw, davidbarsky and a team as code owners March 24, 2022 05:26
@udoprog
Copy link
Contributor Author

udoprog commented Mar 24, 2022

So a thing which this check doesn't accomplish is that each crate is not tested independently. In order to accomplish that they have to be removed from the workspace. Easiest way to accomplish this (that I am aware of) is to remove the Cargo.toml in the root.

Once that is done, we can see the following build failure in tracing-core:

info: running `cargo check --all-features` on tracing-core (1/1)
    Checking lazy_static v1.0.0
    Checking tracing-core v0.2.0 (D:\Repo\tracing\tracing-core)
error: cannot find macro `__lazy_static_internal` in this scope
  --> src\callsite.rs:81:5
   |
81 | /     lazy_static! {
82 | |         static ref REGISTRY: Registry = Registry {
83 | |             callsites: LinkedList::new(),
84 | |             dispatchers: RwLock::new(Vec::new()),
85 | |         };
86 | |     }
   | |_____^
   |
   = note: this error originates in the macro `lazy_static` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0425]: cannot find value `REGISTRY` in this scope
   --> src\callsite.rs:108:31
    |
108 |         let mut dispatchers = REGISTRY.dispatchers.write().unwrap();
    |                               ^^^^^^^^ not found in this scope

error[E0425]: cannot find value `REGISTRY` in this scope
   --> src\callsite.rs:109:26
    |
109 |         let callsites = &REGISTRY.callsites;
    |                          ^^^^^^^^ not found in this scope

error[E0425]: cannot find value `REGISTRY` in this scope
   --> src\callsite.rs:118:27
    |
118 |         let dispatchers = REGISTRY.dispatchers.read().unwrap();
    |                           ^^^^^^^^ not found in this scope

error[E0425]: cannot find value `REGISTRY` in this scope
   --> src\callsite.rs:120:9
    |
120 |         REGISTRY.callsites.push(registration);
    |         ^^^^^^^^ not found in this scope

error[E0425]: cannot find value `REGISTRY` in this scope
   --> src\callsite.rs:124:31
    |
124 |         let mut dispatchers = REGISTRY.dispatchers.write().unwrap();
    |                               ^^^^^^^^ not found in this scope

error[E0425]: cannot find value `REGISTRY` in this scope
   --> src\callsite.rs:125:26
    |
125 |         let callsites = &REGISTRY.callsites;
    |                          ^^^^^^^^ not found in this scope

@hawkw
Copy link
Member

hawkw commented Mar 24, 2022

So a thing which this check doesn't accomplish is that each crate is not tested independently. In order to accomplish that they have to be removed from the workspace. Easiest way to accomplish this (that I am aware of) is to remove the Cargo.toml in the root.

Hmm, it seems like --- hypothetically --- we could just have the CI job run rm Cargo.toml and then cd into each crate's directory? But, the current approach also seems fine for now, and we can figure out additional scripting later...

@hawkw
Copy link
Member

hawkw commented Mar 29, 2022

@udoprog just to clarify, is this ready to merge now?

@udoprog
Copy link
Contributor Author

udoprog commented Mar 29, 2022

If you're OK with the following limitations:

  • The MSRV checks do not properly check individual crates due to them being in a workspace. Deleting the Cargo.toml was suggested, but I haven't been able to try it.
  • I've only fixed 4 crates.

The branch also appears to be outdated now, I'll bump it when I get a minute.

@hawkw
Copy link
Member

hawkw commented Apr 7, 2022

@udoprog do you know if it's possible to use cargo-hack to handle running minimal-versions checks for individual crates? that might make this a bit easier...

@udoprog
Copy link
Contributor Author

udoprog commented Apr 7, 2022

I don't believe it is possible with cargo-hack (also note I do use it for removing dev-dependencies above)! I started poking around with a cargo extension that would create a crate using the built-in packaging mechanism and unpack it into a clean directory where it can run the specified command over it. But I've yet to make much progress with it.

As mentioned in the related Tokio issue hunting down and fixing min-version violations is fairly time consuming. If there was solid tooling to do it* it would be easier to motivate spending the effort on a personal level.

*: A tool would be something where you can just run a script to do all the right tests, and another script which will do other work necessary to just tell you the lowest versions of packages that have been automatically bisected to work within some MSRV constraint.

@taiki-e
Copy link
Member

taiki-e commented Apr 8, 2022

I agree with it is impossible for the current cargo-hack by itself to handle this correctly.

cargo-minimal-versions (PR 4 to be exact) should do most of the right things about this, but as far as running builds within the current workspace, it seems to be impossible to handle the issue related to optional dependencies. (There is also the problem that the current implementation of PR 4 does not work well when tools like rust-analyzer and cargo-watch are running in the background.)

(For library) The way to really seem to be right about this is to remove the path dependencies within the current workspace, then specify the library you want to build as a dependency of a temporary crate created outside the current workspace, and build that temporary crate. (Note that the temporary crate needs to be per feature, not per crate. Also, to catch linkage problems, subcommand needs to be build, the temporary crate needs to be a binary crate, and we need to use statements like use deps as _ to force the linkage.)

I plan to implement that approach in the future in cargo-minimal-versions, but I don't know ETA.
(cargo-minimal-versions also supports binary crate and test subcommand, so perhaps we might want to enable that approach only if the subcommand is check or build and the --lib flag is explicitly passed.)

@udoprog
Copy link
Contributor Author

udoprog commented Apr 8, 2022

I'm glad to see you're putting some mileage into the problem @taiki-e!

The second issue I briefly skimmed over here and I find the most time consuming is the lack of a tool that helps you discover the versions of specific dependencies which are actually build-compatible with a specific msrv. In my mind that would require something like bisecting each dependency from the current specification up until the latest available version and find the latest (or earliest) version of it that still allows the project to build.

The way I currently do it is to:

  • Look at the build error (which might be in some transitive dependency) and go to the project and see when it was fixed. Usually this is some missing API that was introduced in a later version which is either used by the project directly or some dependency.
  • Follow the dependency backwards and find the version of the crate that we depend on which causes the build error to resolve.
  • If no such version exists, file a bug with the project and ask them very nicely to do a patch release which is msrv compatible where they constrain dependencies.

@hawkw
Copy link
Member

hawkw commented Apr 8, 2022

Given that this PR does fix some existing minimal versions issues, I'd like to go ahead and merge this now, even if it isn't exhaustive. Would it make sense to merge this branch as-is, and continue working on minimal-version checking as the tooling improves?

@udoprog
Copy link
Contributor Author

udoprog commented Apr 8, 2022

Go for it!

@hawkw hawkw enabled auto-merge (squash) April 8, 2022 17:56
@hawkw hawkw merged commit 5888369 into tokio-rs:master Apr 8, 2022
hawkw pushed a commit that referenced this pull request Apr 8, 2022
This adds a minimal-versions check to the tracing project. Adapted from
`tokio-rs/tokio`. Adding this avoids breaking downstream dependencies
from accidentally under-constraining minimal versions of dependencies
when they depend on tracing.

I've currently just introduced the check. I will try to and do encourage
others to add patches to fix this where possible since it can be a fair
bit of work to chase down a version of all dependencies that passes
minimal-versions and is msrv. I've also seen some really odd
windows-specific issues (which are not being tested for here).

This is currently only testing `tracing`, `tracing-core`, and
`tracing-subscriber`. Packages such as `tracing-futures` are proving to
be a bit harder to deal with due to having features which enable very
old dependencies.

Steps to test the build minimal versions locally:

```sh
cargo install cargo-hack
rustup default nightly
cargo hack --remove-dev-deps --workspace
cargo update -Z minimal-versions
cargo hack check --all-features --ignore-private
```

CC: tokio-rs/tokio#4513
@hawkw hawkw mentioned this pull request Apr 8, 2022
@udoprog udoprog deleted the minimal-versions branch April 9, 2022 03:19
hawkw pushed a commit that referenced this pull request Apr 9, 2022
This adds a minimal-versions check to the tracing project. Adapted from
`tokio-rs/tokio`. Adding this avoids breaking downstream dependencies
from accidentally under-constraining minimal versions of dependencies
when they depend on tracing.

I've currently just introduced the check. I will try to and do encourage
others to add patches to fix this where possible since it can be a fair
bit of work to chase down a version of all dependencies that passes
minimal-versions and is msrv. I've also seen some really odd
windows-specific issues (which are not being tested for here).

This is currently only testing `tracing`, `tracing-core`, and
`tracing-subscriber`. Packages such as `tracing-futures` are proving to
be a bit harder to deal with due to having features which enable very
old dependencies.

Steps to test the build minimal versions locally:

```sh
cargo install cargo-hack
rustup default nightly
cargo hack --remove-dev-deps --workspace
cargo update -Z minimal-versions
cargo hack check --all-features --ignore-private
```

CC: tokio-rs/tokio#4513
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.

None yet

5 participants