Add cargo metadata to msrv check due to valuable

[arlyon]

🚨 note: CI fail is expected here!

Motivation

Hello! I noticed my CI is failing when trying to include tracing on rust version 1.49.0. This is the advertised MSRV but is breaking because tarpaulin, used for calculating coverage, fails when calling cargo metadata.

Even when the valuable is not enabled, it shows up in Cargo.lock. This is fine even on rust version 1.49.0 as it is ignored when running cargo build, but cargo metadata fails:

❯ cargo metadata  
warning: please specify `--format-version` flag explicitly to avoid compatibility problems
error: failed to download `valuable v0.1.0`

Caused by:
  unable to get packages from source

Caused by:
  failed to parse manifest at `/home/arlyon/.cargo/registry/src/gitpro.ttaallkk.top-1ecc6299db9ec823/valuable-0.1.0/Cargo.toml`

Caused by:
  feature `resolver` is required

  this Cargo does not support nightly features, but if you
  switch to nightly channel you can add
  `cargo-features = ["resolver"]` to enable this feature

Running the same command on rust version 1.51.0, valuable's stated MSRV, works as expected.

Solution

This PR is just to highlight the issue. The proposed solution is to consider cargo metadata as a command that should be supported by the MSRV, and then bump all the crates that depend on valuable to MSRV 1.51.0.

In my case (arlyon/async-stripe#160), I will probably end up downgrading to the latest version without valuable for now, and keep a note if we decide to bump MSRV later down the line.

Thanks for all the work you do! 😍

Alex

More context here: #1913 (comment)

[hawkw]

Hmm, I'm really not sure what the best solution for this is. Our MSRV policy doesn't consider the MSRVs of optional dependencies that are not enabled by default; when an optional dependency is enabled, we consider the MSRV to be the higher of the MSRVs of the tracing crate and the external crate. My understanding is that many other crates follow a similar practice.

The cargo metadata failure is certainly unfortunate. To me, it doesn't seem correct, off the top of my head. The dependency on valuable is an opt-in feature, and if you haven't intentionally enabled it, I don't think it should be breaking your build --- especially given that it is also behind the --cfg tracing_unstable cfg. If you're not building with RUSTFLAGS="--cfg tracing_unstable, it shouldn't be in your dependency tree, as I understand it.

Before we decide what to do here, I think we should make sure this is intended behavior for cargo metadata, and not a bug. If the cargo maintainers think this is correct behavior, we may have to seriously reconsider our MSRV policy.

[arlyon]

Thanks for the detailed insight. I'll bring this up with the cargo team and see what they think. I agree that it's odd that a package that isn't built shows up in the metadata, but I'm sure it'd be a breaking change on their end to start excluding data. Maybe the solution is a flag on metadata that tarpaulin can use to exclude unbuilt packages?

The cargo metadata failure is certainly unfortunate. To me, it doesn't seem correct, off the top of my head. The dependency on valuable is an opt-in feature, and if you haven't intentionally enabled it, I don't think it should be breaking your build --- especially given that it is also behind the --cfg tracing_unstable cfg. If you're not building with RUSTFLAGS="--cfg tracing_unstable, it shouldn't be in your dependency tree, as I understand it.

You're right, it's not technically in the tree, and cargo tree does not include it. It took a while to unwrap what was going on!

I'll report back :)

[arlyon]

Opened an issue here: rust-lang/cargo#10384

[bryangarza]

@arlyon can we close this? Or is there any action remaining on the tracing side for now?

[arlyon]

Hi

I think the outcome was that supporting cargo metadata was not grounds enough to bump msrv so I believe this can be closed. Thanks.

[bryangarza]

Sounds good, thanks! :)