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

Re-add rust-analyzer.cargo.sysrootQueryMetadata #18511

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

darichey
Copy link
Contributor

@darichey darichey commented Nov 14, 2024

#17795 removed this option, turning it on for everyone. This commit adds it back, still on by default.

At work, users are effectively offline, so the initial cargo metadata will always fail. The --no-deps retry will succeed, but the experience with this path is very poor, because a ton of std stuff fails to resolve (some still works, I can't tell the pattern). Something like this is probably happening in #17759.
Screenshot 2024-11-14 at 12 32 23 PM

We want this config option so we can turn it off by default for our users and always use the stitched sysroot library.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2024
@darichey
Copy link
Contributor Author

The --no-deps retry will succeed, but the experience with this path is very poor, because a ton of std stuff fails to resolve

In fact, this experience is so poor, I wonder if it's even worth supporting?

@darichey
Copy link
Contributor Author

If we'd like to get rid of the stitched sysroot stuff, a longer term solution could be teaching rust-analyzer to use something other than cargo metadata for this. At work, the sysroot builds with Buck, so presumably we could use it to get the info we need.

Copy link
Contributor

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

I'm a plus one on this, but I'd prefer someone else merge it. I can, need be.

crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
@darichey darichey force-pushed the sysroot-query-metadata branch 2 times, most recently from 3b06f26 to 52c4534 Compare November 18, 2024 22:26
@darichey darichey force-pushed the sysroot-query-metadata branch from 52c4534 to 3c98b98 Compare November 18, 2024 22:29
@lnicola lnicola enabled auto-merge November 19, 2024 17:06
@lnicola lnicola disabled auto-merge November 26, 2024 05:35
@lnicola lnicola enabled auto-merge November 26, 2024 05:35
@lnicola
Copy link
Member

lnicola commented Nov 26, 2024

r? @davidbarsky

Copy link
Contributor

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

approved! didn't want to be the only one to approve it for conflict of interest reasons.

@lnicola lnicola added this pull request to the merge queue Nov 26, 2024
@lnicola
Copy link
Member

lnicola commented Nov 26, 2024

I know, but I wasn't able to merge it because of your pending "changes requested" review.

@davidbarsky
Copy link
Contributor

I know, but I wasn't able to merge it because of your pending "changes requested" review.

Wild, okay! I thought it was overridable.

@lnicola
Copy link
Member

lnicola commented Nov 26, 2024

I know, check out my struggles in the history above 😕.

@davidbarsky
Copy link
Contributor

I know, check out my struggles in the history above 😕.

I didn't see those in email and I didn't stroll up. I'm sorry about that.

Merged via the queue into rust-lang:master with commit 8d5e91c Nov 26, 2024
9 checks passed
@lnicola lnicola mentioned this pull request Nov 27, 2024
@Veykril
Copy link
Member

Veykril commented Dec 2, 2024

At work, users are effectively offline, so the initial cargo metadata will always fail. The --no-deps retry will succeed, but the experience with this path is very poor, because a ton of std stuff fails to resolve (some still works, I can't tell the pattern).

Hmm, that seems very odd to me? The no-deps retry should yield the same result as the stitched workspace

@davidbarsky
Copy link
Contributor

I'll let @darichey expand, but he tells me "it doesn't".

(I've noticed that weirdness as well, for what it's worth.)

@Veykril
Copy link
Member

Veykril commented Dec 2, 2024

In fact, I can't reproduce this. If I run r-a offline I still get resolved standard library stuff. I'd really prefer to get rid of this config as I'd like to drop the stitched workspace hack in the near future

@darichey
Copy link
Contributor Author

darichey commented Dec 2, 2024

Maybe it's specific to json workspace project loading? I'll try to get a simple repro today

@Veykril
Copy link
Member

Veykril commented Dec 2, 2024

Perhaps, I only tried a basic cargo setup

@Veykril
Copy link
Member

Veykril commented Dec 3, 2024

Opened #18590 to track this, please drop any findings for this there

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