-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add support for loading rustc private crates #6524
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
ce72d23 to
0b08993
Compare
0b08993 to
8716087
Compare
crates/project_model/src/lib.rs
Outdated
| cfg_options.insert_atom("test".into()); | ||
| cfg_options.insert_atom("debug_assertions".into()); | ||
|
|
||
| let mut rustc_pkg_crates = FxHashMap::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, this match arm becomes more complex since it must load rustc_workspace (if provided) and cargo. I tried to clean it up a bit by pulling out the logic around adding a pkg, tgt crate root to a separate function, but there is still quite a bit of repitition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll look into fixing this after the PR is meged. I bet the code could be refactored somewhat more radically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, don't feel confident enough with the codebase to go ahead and do that, but I can definitely see some possible refactorings.
matklad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, modulo dbg!. It would help a bit if you could revert package-lock.json changes, I'll land them separately.
We have one integration test for project loading, but for rustc crates, I think it's better to stick with manual testing.
|
cc @RalfJung I think you wanted something like this for miri, no? I vaguely recall that I promised you to look into something, but I don't remember what it was 😅 |
|
Wow I think that was a long time ago.^^ I haven't been doing a lot of Miri stuff lately so I didn't really run into RA problems with Miri. But there are a lot of |
|
If they come from |
|
So there is no |
|
We have |
|
Can we just stick the source in |
|
I believe the sources are shipped in However, there's no |
cc41b48 to
89ce6b6
Compare
|
bors r+ Thanks @xldenis |
The root |
|
There's a |
|
Oh right possibly I only added |
This PR presents a solution to the problem of making
rustc_privatecrates visible torust-analyzer.Currently developers add dependencies to those crates behind a
cfg(NOT_A_TARGET)target to preventcargofrom building them.This solution is unsatisfactory since it requires modifying
Cargo.tomland causes problems for collaboration or CI.The proposed solution suggested by @matklad is to allow users to give RA a path where the
rustcsources could be found and then load that like a normal workspace.This PR implements this solution by adding a
rustcSourceconfiguration item and adding the cargo metadata to the crate graph if it is provided.I have no idea how this should be tested, or if this code is generally tested at all. I've locally run the extension with these changes and it correctly loads the relevant crates on a
rustc_privateproject of mine.