Avoid panic when using partition_packages on Rust < 1.71

[clechasseur]

#59 added support for using the workspace default members when no parameter is specified. However, this only works on Rust 1.71+, so using clap-cargo on Rust < 1.71 results in a panic when calling partition_packages.

I suggest a simple change: avoid using workspace_default_members unless the flags actually do ask for default packages. This way, it would for example be possible to call a clap-cargo-powered tool with --workspace on Rust < 1.71 and it would work.

(A better change might be to check if workspace_default_members are available before using them, but alas this doesn't seem to be available in cargo_metadata currently.)

If this change is acceptable, I volunteer to work on a PR for this.

[epage]

Could you clarify what panic you are seeing?

I can see us cleaning up the code to only calculate things we need but I don't see how that would be causing a panic.

[clechasseur]

Minimal repro (available here):

Cargo.toml

[package]
name = "cargo-clap-cargo-test"
version = "0.1.0"
edition = "2021"

[dependencies]
cargo_metadata = "=0.18.1"
clap = { version = "=4.5.2", features = ["cargo", "derive"] }
clap-cargo = { version = "=0.14.0", features = ["clap", "cargo_metadata"] }

src/main.rs

use clap::{Args, Parser};
use clap_cargo::{Manifest, Workspace};

#[derive(Debug, Parser)]
#[command(name = "cargo", bin_name = "cargo")]
enum Cli {
    ClapCargoTest(ClapCargoTestArgs),
}

#[derive(Debug, Args)]
struct ClapCargoTestArgs {
    #[command(flatten)]
    manifest: Manifest,
    #[command(flatten)]
    workspace: Workspace,
}

fn main() {
    let Cli::ClapCargoTest(args) = Cli::parse();

    let metadata = args.manifest.metadata().exec().unwrap();
    let (selected, unselected) = args.workspace.partition_packages(&metadata);

    println!(
        "Selected packages: {:?}\nUnselected packages: {:?}",
        selected.iter().map(|&p| &p.name).collect::<Vec<_>>(),
        unselected.iter().map(|&p| &p.name).collect::<Vec<_>>(),
    );
}

Result (some parts snipped for brevity):

PS ...\clap-cargo-test> cargo install --path .
  Installing cargo-clap-cargo-test v0.1.0 (...)
  ...
   Installed package `cargo-clap-cargo-test v0.1.0 (...)

PS ...\clap-cargo-test> cargo +1.71.0 clap-cargo-test
Selected packages: ["cargo-clap-cargo-test"]
Unselected packages: ["anstream", "anstyle", ...]

PS ...\clap-cargo-test> cargo +1.70.0 clap-cargo-test
thread 'main' panicked at ...\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cargo_metadata-0.18.1\src\lib.rs:246:14:
WorkspaceDefaultMembers should only be dereferenced on Cargo versions >= 1.71
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The panic is in the cargo_metadata crate here:
https://github.com/oli-obk/cargo_metadata/blob/0.18.1/src/lib.rs#L246

pub struct WorkspaceDefaultMembers(Option<Vec<PackageId>>);

impl core::ops::Deref for WorkspaceDefaultMembers {
    type Target = [PackageId];

    fn deref(&self) -> &Self::Target {
        self.0
            .as_ref()
            .expect("WorkspaceDefaultMembers should only be dereferenced on Cargo versions >= 1.71")
            // ^^ THIS RIGHT HERE ^^
    }
}

It is caused by defererencing the workspace_default_members here:
https://github.com/crate-ci/clap-cargo/blob/v0.14.0/src/workspace.rs#L42-L43

pub fn partition_packages<'m>(
        &self,
        meta: &'m cargo_metadata::Metadata,
    ) -> (
        Vec<&'m cargo_metadata::Package>,
        Vec<&'m cargo_metadata::Package>,
    ) {
        let selection =
            Packages::from_flags(self.workspace || self.all, &self.exclude, &self.package);
        let workspace_members: std::collections::HashSet<_> =
            meta.workspace_members.iter().collect();
        let workspace_default_members: std::collections::HashSet<_> =
            meta.workspace_default_members.iter().collect();
                                       // ^ DEFERENCING HAPPENS HERE

To avoid it, my suggestion would be to move the dereferencing of workspace_default_members to the Packages::Default match arm. This way, it would still panic when used with default flags, but specifying a custom flag like --workspace would allow proper use on Rust < 1.71.

[epage]

If we delay accessing that field, we will just make it so we sometimes panic but o always. That would make it harder to detect this problem.

It looks like cargo-metadata does not provide a sufficient API for us to detect the user is running an older cargo.

Choices for moving forward

• Do nothing. I generally recommend people run the latest version of rustc for development and worry about their deployment / MSRV version for production builds / CI.
• Revert Add support for workspace default members #59 until some indeterminate period of time. We are already 5 versions away. Its hard to say what a sufficient window further away would be
• See if cargo-metadata would be willing to add support for detecting the presence of that field.

[clechasseur]

I guess there's another choice: determine the running Cargo's version to see if it's safe to access the field. However, that doesn't seem to be an ideal solution. (That could be implemented in the tool using clap-cargo too.)

FWIW, I've also volunteered to add something to detect this in cargo_metadata: oli-obk/cargo_metadata#254

Do nothing. I generally recommend people run the latest version of rustc for development and worry about their deployment / MSRV version for production builds / CI.

I indeed detected the issue in a CI workflow that validates a crate's MSRV.

[epage]

1.84 comes out next week. Our own MSRV is 1.74 so we can't even build on systems this is targeted to run on.

Whats the use case for runtime support for 1.71 with the latest clap-cargo?

[clechasseur]

I use clap-cargo in a cargo command designed as a companion to cargo-msrv (cargo-msrv-prep). Since the goal here is to determine a crate's MSRV, the tool needs to be able to run on older versions of cargo (although not necessarily to build on such versions). I assume that cargo-msrv has the same need, especially since the PR above was created by @foresterre, cargo-msrv's creator; they are also responsible for the fix to oli-obk/cargo_metadata#254 (the fix has been merged but no new version of cargo-metadata has been released yet).

(Digging any deeper into the use case would require a discussion about the importance of MSRV in Rust libraries and support of older Rust versions in general, so I'll let you decide whether it's a discussion that belongs in this issue or not.)

[epage]

(Digging any deeper into the use case would require a discussion about the importance of MSRV in Rust libraries and support of older Rust versions in general, so I'll let you decide whether it's a discussion that belongs in this issue or not.)

Let's not. I've participated in many and most MSRV conversations I come across are unproductive asks for free work from overburdended maintainers.

Our own MSRV policy is N-2 and I've deviated from my norm of strict updating of MSRV to let it slide all the way back to 1.74.

As for oli-obk/cargo_metadata#282, I find it sketchy to infer meaning from the presence of absence of a field.

[foresterre]

As for oli-obk/cargo_metadata#282, I find it sketchy to infer meaning from the presence of absence of a field.

I'm also not completely satisfied with it, but I find it much better than panicking on a dereference (requiring a panic handler to be set). Essentially, the field is missing for older cargo metadata versions (because it didn't exist in the first place), so it is a slightly morphed Option. I see that there can be more reasons for the lack of presence of the value: the option could also be interpreted to be None if no default workspace packages are set, or if it isn't a workspace in the first place.

The question is how you would invoke cargo metadata with a single modern version of the cargo_metadata crate on an older Cargo project, which doesn't have the fields which cargo metadata expected?

To me, the MSRV of cargo_metadata implies that a Rust compiler with at least that MSRV, should be able to compile the cargo_metadata crate.
To me that doesn't mean that cargo_metadata should only support deserializing the invoked cargo metadata command for its MSRV and up.