Skip to content

Add ManifestPath to CrateData; update dependency view to use ManifestPath #14931

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

Conversation

davidbarsky
Copy link
Contributor

Hi! I've got a few changes:

  • I've added an optional ManifestPath to CrateGraph and project_json.rs's Crate/CrateData structs. ManifestPath is optional inside of CrateGraph because single file workspaces don't have a manifest file by definition. I've made ManifestPath optional inside of project_json.rs for backwards compatibility reasons, but I'd be happy to make it non-optional after a period of time.
  • I've also made the dependency explorer to use the manifest path from the crate graph instead of trying to figure out the location of the the crate root by searching for a Cargo.toml. Since this is is sourced from Cargo itself, I think this is an overall improvement. It also allows for non-Cargo build systems to make use of this feature.

If you're okay with these changes, please let me know before landing it! I'd like to update the documentation for rust-project.json with these details (as well as update the rust-project definition in the VS Code extension).

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2023
@Veykril
Copy link
Member

Veykril commented Jun 10, 2023

What is the use case for this? Just simplifying the dependency explorer's cargo toml lookup? The manifest path is a project structure specific thing, so I don't think we should put that into the internal crate graph as that is project/workspace agnostic.

We should instead just look through the loaded workspaces for the cargo target whose root module is the same root as a given crate's, basically what GlobalStateSnapshot::cargo_target_for_crate_root does roughly. For rust-project.json we still would need to add that field I suppose since there is no other place to put that info (and then do the mapping like we'd do for cargo)

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2023
@davidbarsky
Copy link
Contributor Author

What is the use case for this? Just simplifying the dependency explorer's cargo toml lookup?

Yeah—there's a few user-facing features being added that rely on knowing the manifest path, and exposing Cargo's understanding of the world is preferable to trying to figure it on the fly.

The manifest path is a project structure specific thing, so I don't think we should put that into the internal crate graph as that is project/workspace agnostic.

Fair enough!

We should instead just look through the loaded workspaces for the cargo target whose root module is the same root as a given crate's, basically what GlobalStateSnapshot::cargo_target_for_crate_root does roughly.

Oh, that's perfect—I didn't know that existed. Sorry for missing it!

For rust-project.json we still would need to add that field I suppose since there is no other place to put that info (and then do the mapping like we'd do for cargo).

Can do—I'll add it as an optional field and update the documentation.

@Veykril
Copy link
Member

Veykril commented Aug 15, 2023

@davidbarsky what's the status on this? (doing a small PR cleanup right now)

@davidbarsky
Copy link
Contributor Author

@davidbarsky what's the status on this? (doing a small PR cleanup right now)

Oh, whoops! sorry about missing this. I'll close this PR and create a new one instead of rebasing and moving to ManifestPath.

@davidbarsky davidbarsky closed this Sep 5, 2023
@davidbarsky davidbarsky deleted the davidbarsky/add-manifest-path-to-crate-graph branch November 21, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants