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

Drop rust-analyzer.cargo.sysrootQueryMetadata and support for stitched sysroot workspace #18590

Open
Veykril opened this issue Dec 3, 2024 · 8 comments
Labels
C-cleanup Category: issues documenting cleanup

Comments

@Veykril
Copy link
Member

Veykril commented Dec 3, 2024

#18511 added the config back, ultimately we should get rid of the entire stitched concept as we won't need it anymore for newer sysroots.

@Veykril Veykril added the C-cleanup Category: issues documenting cleanup label Dec 3, 2024
@Veykril
Copy link
Member Author

Veykril commented Dec 3, 2024

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

Originally posted by @darichey in #18511 (comment)

The json part might be it, or rather your environment. I assume your env might not support running cargo as is for the sysroot discovery?

@lnicola
Copy link
Member

lnicola commented Dec 3, 2024

CC #17759 (comment).

@davidbarsky
Copy link
Contributor

Yeah, the default development environment we operate in has an outbound firewall to the entire internet and requires deliberate, per-program actions to opt-out. Additionally, cargo is not preinstalled on devservers, Buck is assumed to be the default.

(We do have a buckified sysroot, though...)

@Veykril
Copy link
Member Author

Veykril commented Dec 3, 2024

Additionally, cargo is not preinstalled on devservers, Buck is assumed to be the default.

That is the issue then, although we should fall back to the stitched one in that case I feel like. I guess my recovery logic fails there (when the --no-deps call still doesnt yield results).

That does raise a good point though, we can't assume cargo exists for people using the json format (I do wanna get rid of the current stitched system either way so there is something to be designed here that replaces that). So probably replacing the stitched stuff with something that the rust-project.json format can supply, effectively move the stitching to the non-cargo build system which feels proper as it then replaces what cargo does in this aspect as well.

@darichey
Copy link
Contributor

darichey commented Dec 3, 2024

Additionally, cargo is not preinstalled on devservers, Buck is assumed to be the default.

That is the issue then

I don't think this is it! The problem only occurred for people who do have cargo installed!

I got distracted by other things, let me get a repro :)

@Veykril
Copy link
Member Author

Veykril commented Dec 3, 2024

That is even more perplexing 😬 though i still think the approach for replacing the stitched one stands nevertheless (which would solve it either way here, or well, should)

@darichey
Copy link
Contributor

darichey commented Dec 3, 2024

@Veykril I can repro with only this patch to force --no-deps when loading the sysroot: https://gist.github.com/darichey/f3173b82a2eaec75473419d0cc661f93.

@davidbarsky Pointed out that the "use X instead of cargo for sysroot metadata" setting should be independent of rust-project.json/discoverCommand because most users of Buck/Bazel will still want to use Cargo for the sysroot. I'm happy to work on this if you agree.

@Veykril
Copy link
Member Author

Veykril commented Dec 3, 2024

Yes that sounds fine to me. So always try using cargo for cargo workspaces, but for rust-project.json the json either specifies the sysroot layout manually (it stitches it itself, or sets it empty if it doesn't want a sysroot) or it doesn't in which case we shell out to cargo. That sounds reasonable to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: issues documenting cleanup
Projects
None yet
Development

No branches or pull requests

4 participants