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

internal: project-model: when using rust-project.json, prefer the sysroot-defined rustc over discovery in $PATH #15560

Conversation

davidbarsky
Copy link
Contributor

At the moment, rust-analyzer discovers rustc via the $PATH even if the sysroot field is defined in a rust-project.json. However, this does not work for users who do not have rustup installed, resulting in any cfg-based inference in rust-analzyer not working correctly. In my (decently naive!) opinion, it makes more sense to rely on the sysroot field in the rust-project.json.

One might ask "why not add rustc to the $PATH?" That is a reasonable question, but that doesn't work for my use case:

  • The path to the sysroot in my employer's monorepo changes depending on which platform a user is on. For example, if they're on Linux, they'd want to use the sysroot defined at path a, whereas if they're on macOS, they'd want to use the sysroot at path b (I wrote the sysroot resolution functionality here, if you're curious).
  • The location of the sysroot can (and does!) change, especially as people figure out how to make Rust run successfully on non-Linux platforms (e.g., iOS, Android, etc.) in a monorepo. Updating people's $PATH company-wide is hard while updating a config inside a CLI is pretty easy.

Testing

I've created a rust-project.json using rust-project and was able to successfully load a project with and without the sysroot/sysroot_src fields—without those fields, rust-analyzer fell back to the $PATH based approach, as evidenced by [DEBUG project_model::rustc_cfg] using rustc from env rustc="rustc" showing up in the logs.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2023
@davidbarsky davidbarsky force-pushed the davidbarsky/use-sysroot-rustc-to-determine-cfgs branch from 839409a to 553152e Compare September 7, 2023 19:07
Copy link
Contributor Author

@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.

No rush on reviewing this, but I wanted to leave comments on the approach I took here since changes inside of project-model can be kinda tricky.

crates/project-model/src/rustc_cfg.rs Show resolved Hide resolved
use crate::{cfg_flag::CfgFlag, utf8_stdout, ManifestPath};
use crate::{cfg_flag::CfgFlag, utf8_stdout, ManifestPath, Sysroot};

pub(crate) enum RustcCfgConfig<'a> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name here is a little generic, but I'm struggling to think of anything better. In any case, I hope that the doc comment helps a little.

Copy link
Member

Choose a reason for hiding this comment

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

The entire module is slightly mislabeled by now I feel like, not sure what a better name for this would be though. Maybe builtin_cfg and the like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

builtin_cfg is a good option. While we're bikeshedding, what do you think something like target_cfg since the output consists of things like:

``` rust-analyzer on davidbarsky/use-sysroot-rustc-to-determine-cfgs [$] via 🦀 v1.71.0 ❯ rustc --print cfg -O panic="unwind" target_arch="aarch64" target_endian="little" target_env="" target_family="unix" target_feature="aes" target_feature="crc" target_feature="dit" target_feature="dotprod" target_feature="dpb" target_feature="dpb2" target_feature="fcma" target_feature="fhm" target_feature="flagm" target_feature="fp16" target_feature="frintts" target_feature="jsconv" target_feature="lor" target_feature="lse" target_feature="neon" target_feature="paca" target_feature="pacg" target_feature="pan" target_feature="pmuv3" target_feature="ras" target_feature="rcpc" target_feature="rcpc2" target_feature="rdm" target_feature="sb" target_feature="sha2" target_feature="sha3" target_feature="ssbs" target_feature="vh" target_has_atomic="128" target_has_atomic="16" target_has_atomic="32" target_has_atomic="64" target_has_atomic="8" target_has_atomic="ptr" target_os="macos" target_pointer_width="64" target_vendor="apple" unix ```

crates/project-model/src/rustc_cfg.rs Show resolved Hide resolved
crates/project-model/src/sysroot.rs Show resolved Hide resolved
crates/project-model/src/sysroot.rs Show resolved Hide resolved
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.

Sounds reasonable to me

crates/project-model/src/rustc_cfg.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Sep 8, 2023

@bors delegate+

@bors
Copy link
Collaborator

bors commented Sep 8, 2023

✌️ @davidbarsky, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2023
@davidbarsky
Copy link
Contributor Author

@bors r=@Veykril

@bors
Copy link
Collaborator

bors commented Sep 8, 2023

📌 Commit 0bf6ffa has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 8, 2023

⌛ Testing commit 0bf6ffa with merge c405509...

@bors
Copy link
Collaborator

bors commented Sep 8, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing c405509 to master...

@bors bors merged commit c405509 into rust-lang:master Sep 8, 2023
10 checks passed
@davidbarsky davidbarsky deleted the davidbarsky/use-sysroot-rustc-to-determine-cfgs branch September 8, 2023 20:53
@lnicola lnicola changed the title project-model: when using rust-project.json, prefer the sysroot-defined rustc over discovery in $PATH internal: project-model: when using rust-project.json, prefer the sysroot-defined rustc over discovery in $PATH Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants