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

Add support for specifying Natvis flags for Cargo to pass through to rustc #10192

Closed
wants to merge 19 commits into from

Conversation

ridwanabdillahi
Copy link

@ridwanabdillahi ridwanabdillahi commented Dec 11, 2021

Implemented this RFC to add support for new toml syntax to specify the set of Natvis files to add for a crate.

Added new toml section [debugger-visualizations] and new key debugger-visualizations.natvis which accepts a list of file paths and passes them down to rustc.

Updated the fingerprinting logic to ensure a rebuild for a crate when the set of Natvis files have been updated.

Also added auto discovery of Natvis files under the pre-determined directory of the package root, dbgvis/natvis which can be overridden when the debugger-visualizations.natvis key is set.

RFC: rust-lang/rfcs#3191
Rust PR: rust-lang/rust#91779

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@alexcrichton
Copy link
Member

I personally know very little about natvis (basically nothing other than that it's related to debugging on Windows), but from a Cargo UI perspective one suggestion I might have is that we could possibly make this more user-friendly a bit with some conventions. For example if you have src/lib.rs then you don't need to otherwise inform Cargo that you've got a library in your project. We could preseumably do something similar for natvis files. If the interface is that Cargo passes a bunch of -C natvis=... files to rustc then Cargo could have something where the convention is that $PROJECT_ROOT/natvis/*.natvis are all passe to rustc (of course there's bikeshedding to happen on the path itself). The manual configuration in Cargo.toml would still be supported but would be optional if you're otherwise following the Cargo conventions. Are there conventions for *.natvis files in other languages/ecosystems/projects we could draw inspiration from?

Otherwise I notice that the natvis files are a global configuration option but should they instead be per-compile-target for Cargo? For example if natvis files were available should Cargo pass them to rustc for the binaries, libraries, etc? Or only the library?

(some of this depends on the precise rustc interface which I don't know much about, sorry only skimmed the RFC, so that may inform the design here as well)

@ridwanabdillahi
Copy link
Author

We could preseumably do something similar for natvis files. If the interface is that Cargo passes a bunch of -C natvis=... files to rustc then Cargo could have something where the convention is that $PROJECT_ROOT/natvis/*.natvis are all passe to rustc (of course there's bikeshedding to happen on the path

In the RFC under the future goals section, I mention this capability for Cargo to discover Natvis files that live under a specific directory and automatically pass them onto rustc. The convention stated in the RFC is $PROJECT_ROOT/dbgvis/natvis/*.natvis. I added support for this behavior in this PR and the toml syntax would override the auto discovery behavior of Cargo.

Otherwise I notice that the natvis files are a global configuration option but should they instead be per-compile-target for Cargo?

I believe being a global configuration option would be the right thing to do here. In the Rust implementation, the natvis for each crate is stored in the crate metadata. Thus when building a binary with multiple dependencies, the Natvis files can be added to the linker invocation of the final binary being generated.

@alexcrichton
Copy link
Member

Oh sorry I think I missed that part! If there's already inference that sounds great.

I don't really know enough about natvis to know how it all works, so if you say passing it to all crates within a package is the way to do it then I will believe you.

@bors
Copy link
Contributor

bors commented Jan 10, 2022

☔ The latest upstream changes (presumably #10217) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 22, 2022

☔ The latest upstream changes (presumably #9992) made this pull request unmergeable. Please resolve the merge conflicts.

@ridwanabdillahi
Copy link
Author

The latest changes proposed in RFC 3191 remove the need for any Cargo changes.

@ridwanabdillahi ridwanabdillahi deleted the natvis branch March 3, 2022 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants