Compatibility with workspace inheritance

[max-sixty]

Related to #295, but I think not the same.

Currently running cargo msrv verify on https://github.com/prql/prql within prql-compiler raises an error:

❯ cargo msrv verify
Fetching index
Unable to find key 'package.rust-version' (or 'package.metadata.msrv') in '/Users/maximilian/workspace/prql/prql-compiler/Cargo.toml'

I think that's because we use workspace inheritance, taking the rust version from the workspace.

Is that something that could be supported?

Thanks!

[foresterre]

Yes, absolutely! I hope to take on proper workspace support in the next version.

[gautamprikshit1]

Any guidance/help on this one??

[foresterre]

Any guidance/help on this one??

As in: you want to help with implementing this, or how you can workaround it right now? :)

[gautamprikshit1]

I want to implement it

[foresterre]

Awesome!

Before implementing anything though, we'll need to think about what the proper workspace support will entail. For this, I wanted to first look in detail at how Cargo handles workspaces (on a user and implementation level), and then what other cargo plugins currently do. Probably we'll want to mimic Cargo where we can.

I'll try to find some time this week to think this through and write it down.
If you have opinions or would like to help with the prior research, feel welcome to write about it :). If you rather only implement ideas, that's also fine!

[max-sixty]

Unless I'm missing some complexity, this issue would just be inheriting whatever is specified at the workspace level in each crate.

Then #295 might be slightly more involved — i.e. does cargo-msrv check every crate individually, etc.

[foresterre]

I think two things need to happen.

1. Find out whether the current crate is part of a workspace
   a. (Then we also have the manifest of the workspace)
2. Set inherited values from the workspace

Considering (1), the Cargo book states:

When inside a subdirectory within the workspace, Cargo will automatically search the parent directories for a Cargo.toml file with a [workspace] definition to determine which workspace to use. The package.workspace manifest key can be used in member crates to point at a workspace’s root to override this automatic search. The manual setting can be useful if the member is not inside a subdirectory of the workspace root.

In other words: paths to workspace member crates are always children, so e.g., this would be invalid:

[workspace]
members = [ 
  "../member_one_level_higher",
]

This makes the problem much simpler; if members could have been anywhere relative to the workspace, well, you might've needed to seach your complete file system to find a workspace from a member crate (since the workspace points to its children, but not the reverse).

Now we can simply recursively enumerate parent Cargo manifests, and check whether there is a workspace, and whether we're a member of it:

---

In pseudo code:

let current_crate = ...;

while let Some(path) = path.pop_dir():
  if let Some(manifest) = path.join('Cargo.toml'):
    if let Some(ws) = workspace_section(manifest):
      if ws.is_member(current_crate):
        return Some(true) // we're part of this workspace, we'll need to inherit workspace values
      else:
        return Some(false) // we're not part of the workspace, no need to inherit anything

return None

The second part (2), is simply replacing the inherited values, in the respective Context.

[foresterre]

@gautamprikshit1 Do you still want to give it a shot?

[not-my-profile]

I really don't see any reason why cargo-msrv should reimplement the Cargo config resolution, when it can just obtain the resolved configuration via cargo metadata. In fact this crate does already depend on cargo_metadata for the dependency graph resolver.

The only obstacle is that the cargo_metadata crate apparently uses a wrong type for the rust_version field, but I have just submitted a PR there to remedy that: oli-obk/cargo_metadata#244.

Once that type is fixed in cargo_metadata, I am happy to submit a PR here to use cargo_metadata for the config resolution of cargo msrv verify, to resolve this issue :)

[foresterre]

Once that type is fixed in cargo_metadata, I am happy to submit a PR here to use cargo_metadata for the config resolution of cargo msrv verify, to resolve this issue :)

That would be wonderful 😄. Thanks!

[spenserblack]

I'm just getting back into Rust after a long hiatus, so please forgive me if this is silly. But doesn't cargo package convert the manifest into a more backwards-compatible format, including inheritance? With issues like this cargo msrv is actually returning the MSRV for development, but couldn't the MSRV for the end user be obtained by checking against target/package/FOO-X.Y.Z?

[CGMossa]

@not-my-profile hej! I see that oli-obk/cargo_metadata#244 has been merged. Do you still remember how you would fix the workspace issue with that now done?

Update: Oh! I see that you made that PR yourself.. Anyways, if you're still active, maybe I can help?

[foresterre]

I think to solve this issue, we need:

1. Fix build(deps): bump cargo_metadata from 0.15.4 to 0.18.1 #802 (it fails on the exact fix referenced here before, where the cargo msrv list prints the MSRV of dependencies, and used to use the VersionReq, but now it is a Version instead. Change type of Package.rust_version from VersionReq to Version oli-obk/cargo_metadata#244)
2. Use cargo metadata to infer the workspace variables, and set them as appropriate (this can possibly be done incrementally)

Feel free to submit a PR; I won't have time to implement this myself in the next week or so, but am happy to help or review a PR.

[Jefffrey]

Hey, was just wondering on the status of this issue now? If it's not on anyone's radar at the moment, I can take a shot at implementing a PR to push this over the finish line 🙂

[foresterre]

I don't think anyone is working on thus right now :)

[max-sixty]

I think we can close, thanks to #882

[eitsupi]

Hello.
I have tried the new cargo-msrv and it seems that cargo-msrv msrv find supports workspaces root (great!) but not verify.
Should I open another issue?

[foresterre]

Hello. I have tried the new cargo-msrv and it seems that cargo-msrv msrv find supports workspaces root (great!) but not verify. Should I open another issue?

Yikes, that should've worked.
Please do open an issue for that 🙏.