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

[metadata] Add workspace.dependencies and <dep>.workspace to cargo-metadata (Rust 1.64+) #11141

Open
poliorcetics opened this issue Sep 24, 2022 · 11 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-metadata S-propose-close Status: A team member has nominated this for closing, pending further input from the team

Comments

@poliorcetics
Copy link
Contributor

Problem

cargo metada does not show the workspace dependencies nor if a dependency is inherited from the workspace in its output.

Proposed Solution

Add the missing metadata to the output.

Notes

We should also remember to update oli-obk/cargo_metadata at the same time to make it immediately available to most people

@poliorcetics poliorcetics added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Sep 24, 2022
@epage
Copy link
Contributor

epage commented Sep 26, 2022

What is is you are wanting to do programmatically with the workspace. entries and packages references them?

@poliorcetics
Copy link
Contributor Author

poliorcetics commented Sep 26, 2022

I wanted to write a little tool to see if a dependency was inherited or not from the workspace, and if its features matched the ones defined in workspace.dependencies. Same for things like workspace.package infos (repository for example, in multi-crates repositories)

@poliorcetics
Copy link
Contributor Author

Another example, for my PR on cargo-edit, it would avoid reading the manifest twice, once through the metadata to resolve the version and one manually to find if package.version.workspace is true

@epage
Copy link
Contributor

epage commented Sep 29, 2022

The cost of reading the manifest multiple times feels relatively small to me in the context of cargo-set-version and cargo-release.

@poliorcetics
Copy link
Contributor Author

For this specific case yes, at least in terms of IOs. In terms of code, it's quite a few lines to check a boolean. It's also lots of duplicated code in each program that want to check this

@Muscraft
Copy link
Member

To give numbers to what @epage is saying:

When working on workspace inheritance we found a case where cargo was reading most manifests twice and making it only once dropped the time by about 10ms (150ms to 140ms) for one large workspace and on smaller workspaces, it was a few 1ms - 2 ms. This was on an M1 Mac with an SSD but I would still not be concerned about the added time.

Looking at the issue of duplicated code for any project that has to deal with this, I am not very concerned. There is always going to be some level of this across the ecosystem. I would be concerned that changing the cargo metadata output could be a breaking change and have winder implications than intended.

@epage
Copy link
Contributor

epage commented Sep 29, 2022

We can make cargo-metadata changes without them being breaking, it just comes with a complexity and performance penalty to all other cargo-metadata users. We have to weigh these out in adding more content to cargo-metadata. Ideally, we would add a query language on top of it so people can request what they want. Even with that, in this case, it requires cargo to do extra book keeping to track the extra information about workspace inheritance.

From there, the question is how much of a need is there for this to weigh the needs of these users against everything else.

@poliorcetics
Copy link
Contributor Author

Looking at the issue of duplicated code for any project that has to deal with this, I am not very concerned

That's what concerns me the most. To correctly check a workspace, the procedure is

  1. Ask cargo_metadata for the the workspace members
  2. Check root Cargo.toml for workspace.package.version = "version"
  3. For each workspace members, read the manifest, checking for package.version.workspace = true (in all the forms accepted by TOML, so obligatory direct dependency on either toml_edit or toml, and writing specific code for this)

The last step is easy but could also be a one-line check if it was present in the metadata, at very little cost for cargo I believe since it already must check for this when computing the version itself

From there, the question is how much of a need is there for this to weigh the needs of these users against everything else.

Yes, it could be I'm the only person needing this, in which case it's not very useful. I have seen at least one other tool (on Reddit r/rust I think ?) that moves workspace crates to [workspace.dependencies] and checks their version is inherited from the workspace (so it also needs to check for package.version.workspace = true, not just matching versions)

@epage epage added the S-propose-close Status: A team member has nominated this for closing, pending further input from the team label Oct 16, 2023
@epage
Copy link
Contributor

epage commented Oct 16, 2023

I'm going to propose to the cargo team that we close this. This adds a lot of bookkeeping on cargo's side while being one of several forms of normalized information where we don't track the unnormalized form. The workaround (read each manifest) is relatively trivial (its what cargo does in cargo add and cargo rm).

@heaths
Copy link

heaths commented Feb 1, 2024

Since dependencies are shown for each workspace member - as well as other bits of information like which features they are using or using the default features, etc. - wouldn't it be idiomatic to show whether that information was inherited from a workspace? I appreciate from #4018 that identifying the "current" package isn't always authoritative, but it seems every package in a workspace already has package-specific "attributes" to its dependency list. Identifying the "current" crate isn't too difficult nor worry me too much with duplicative effort, but parsing dependencies for all the current and possible future permutations as @poliorcetics eludes to is something it seems either cargo metadata or the cargo crate should do in a consolidated manner.

@epage
Copy link
Contributor

epage commented Feb 1, 2024

I don't see how this is a question of what is "idiomatic". cargo metadata today shows normalized output.

It puts a big compatibility burden on us to show a partially-normalized form.

There is also the question of how normalized

  • Whether a dependency is inherited or not
  • What fields were inherited
  • Other inherited information like package fields
  • When it comes to [lints], we might add overriding and so that gets back to "what fields are inherited" question that dependencies have.

For any of these questions, this would also require us adding all of this bookkeeping to cargo so we can present it. cargo metadata doesn't look directly at cargo-util-schema::maniest::Manifest and render it but is pulling the data from the core types in cargo that are used for all regular processing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-metadata S-propose-close Status: A team member has nominated this for closing, pending further input from the team
Projects
None yet
Development

No branches or pull requests

4 participants