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

verify sub command can't be used on root of workspace (find can be used) #1023

Open
eitsupi opened this issue Oct 9, 2024 · 7 comments
Open

Comments

@eitsupi
Copy link

eitsupi commented Oct 9, 2024

I have tried the new cargo-msrv and it seems that cargo-msrv msrv find supports workspaces root (great!) but not verify.
It seems that verify can't get the version from workspace.package.rust-version.

Originally posted by @eitsupi in #590 (comment)

@foresterre
Copy link
Owner

Thanks! I'll have a look

@foresterre
Copy link
Owner

Could you describe what you did and what you expected to happen?

I tried to reproduce it, but here it seems to work as expected:

asciicast

@eitsupi
Copy link
Author

eitsupi commented Oct 9, 2024

Thanks for your quick response!

My intention was to use the verify command at the root of the workspace rather than in a subdirectory. (To verify workspace.package.rust-version, which was found by the find command)
It works fine for subdirectories (individual crates) as in your example.

@foresterre
Copy link
Owner

Are you in a virtual workspace or does your crate also have a root package?

If it is a virtual workspace, what kind of behaviour did you expect from cargo-msrv (or any cargo plugin which reads from the workspace.package table)? 😄

As far as I am aware, the keys in the workspace.package table are solely to share their values between workspace members, and aren't meant to pin the value for the workspace as a whole. Note that inheriting packages need to explicitely say it, if they want to inherit the key from the workspace.

What I can imagine however, is that the behaviour you're looking for is that running cargo msrv verify --workspace, verifies all packages in the workspace.

The difficulty with doing this by default, i.e. when running cargo msrv verify from a workspace root, is that you'll have to disambiguate between "running verify against a root package" or "running verify against all workspace members plus the root package"

@eitsupi
Copy link
Author

eitsupi commented Oct 10, 2024

If it is a virtual workspace, what kind of behaviour did you expect from cargo-msrv (or any cargo plugin which reads from the workspace.package table)? 😄

As far as I am aware, the keys in the workspace.package table are solely to share their values between workspace members, and aren't meant to pin the value for the workspace as a whole. Note that inheriting packages need to explicitely say it, if they want to inherit the key from the workspace.

Thank you for clarifying this.
I mistakenly thought it represented the MSRV of the entire workspace because the find command worked.

Perhaps find should also fail in the virtual workspace root? 🤔

There is no doubt that what I am ultimately looking for is a command to display the MSRV of all the crates at once.

@max-sixty
Copy link

The difficulty with doing this by default, i.e. when running cargo msrv verify from a workspace root, is that you'll have to disambiguate between "running verify against a root package" or "running verify against all workspace members plus the root package"

I would have imagined that it would inherit the behavior of cargo — use the default workspace members — which is the root if one exists, or all members if one doesn't exist or --workspace is passed.

(along with @eitsupi, I'm a dev of prql. I'm separately a maintainer of insta, where we do exactly that)

@foresterre
Copy link
Owner

The difficulty with doing this by default, i.e. when running cargo msrv verify from a workspace root, is that you'll have to disambiguate between "running verify against a root package" or "running verify against all workspace members plus the root package"

I would have imagined that it would inherit the behavior of cargo — use the default workspace members — which is the root if one exists, or all members if one doesn't exist or --workspace is passed.

(along with @eitsupi, I'm a dev of prql. I'm separately a maintainer of insta, where we do exactly that)

Thanks for the feedback!

I never realized that the following is the cargo default for a virtual workspace:

or all members if one doesn't exist

I agree that inheriting cargo's behavior is most logical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants