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

feat: Add config setting which allows adding additional include paths to the VFS. #18880

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

PrototypeNM1
Copy link
Contributor

@PrototypeNM1 PrototypeNM1 commented Jan 8, 2025

The motivation for this is resolving unresolved symbols for Firefox. We build Rust code with cargo but generate some code via our primary build system mach. Additionally, we bundle the windows crate in an out-of-source and out-of-build location. Essentially the same idea as discussed here, Firefox issue tracked here.

I investigated instead populating the vfs via rustc's --emit=dep-info as cargo already emits .d files but at this time cargo does not relay .d file changes as it hazards the unspecified cargo directory layout becoming defacto specified.

This change therefore instead adds a config option to manually specify extra includes for the VFS as suggested in the above Zulip conversation and in this bug (similarly motivated by Firefox's needs).

I don't believe this change ready to land but it's in a place where it's sufficient to anchor a conversation on what's necessary beyond a minimal implementation, and I've verified this works with both Firefox's XPCOM generated functions and windows includes.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2025
@PrototypeNM1 PrototypeNM1 force-pushed the extra-includes branch 2 times, most recently from 50aa72e to a1a3055 Compare January 8, 2025 03:12
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether adding this to all local roots will work (I fear it might break the sourceroot setup), but generally this seems fine to me to temporarily paper over the issues people have until we have it properly fixed (or a proper way to add these things on a finer basis).

@Veykril
Copy link
Member

Veykril commented Jan 8, 2025

You'll need to run the tests in the rust-analyzer crate (that will regenerate some files) cargo test -p rust-analyzer as well as move the config so its in lexicographic order (that one should be done first as it affects the generated file).

@PrototypeNM1
Copy link
Contributor Author

PrototypeNM1 commented Jan 8, 2025

Is there an issue I can link to for tracking the proper fix you alluded to?

@Veykril
Copy link
Member

Veykril commented Jan 9, 2025

#9173

@Veykril Veykril added this pull request to the merge queue Jan 9, 2025
Merged via the queue into rust-lang:master with commit 2e234e3 Jan 9, 2025
9 checks passed
@lnicola lnicola changed the title Add config setting which allows adding additional include paths to the VFS. feat: Add config setting which allows adding additional include paths to the VFS. Jan 9, 2025
@PrototypeNM1 PrototypeNM1 deleted the extra-includes branch January 9, 2025 21:48
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.

3 participants