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

Set IS_RUST_ANALYZER=1 for build commands #15115

Closed
wants to merge 1 commit into from
Closed

Conversation

vgel
Copy link

@vgel vgel commented Jun 22, 2023

We have some builds scripts that do heavy work (e.g., running wasm-pack) that isn't necessary for RA's analysis, but nonetheless slows down IDE startup because RA runs the build script on server restart^1. We want to add something essentially like this to our build script:

// build.rs
if !is_rust_analyzer() {
    // do slow stuff that RA doesn't need
}

But right now there isn't a great way to detect RA in a build script. Based on current main, our build script actually looks like this:

let is_rust_analyzer = option_env!("RA_RUSTC_WRAPPER").is_some() || option_env!("IS_RUST_ANALYZER").is_some();
if !is_rust_analyzer {
    do_wasm_pack();
}

Where RA_RUSTC_WRAPPER is set for some invocations, but for full coverage you need to manually add IS_RUST_ANALYZER to cargoExtraEnv in your editor's rust-analyzer config. This PR simply unconditionally sets that environment variable.

Bikeshedding welcome on the variable name, I just picked what seemed reasonable. There's also the question of documentation, tests, etc.—this is my first RA PR so (if people want to see this merged), I'd appreciate guidance on how to do that.

^1 : We do emit the appropriate cargo:rerun-if-changed directives, but that doesn't help when switching branches causes genuine changes in those directories.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2023
@Veykril
Copy link
Member

Veykril commented Jul 3, 2023

I'm curious but are there problems with just setting cargo.extraEnv for you here?

@vgel
Copy link
Author

vgel commented Jul 6, 2023

I'm curious but are there problems with just setting cargo.extraEnv for you here?

Yeah, that's what we're doing now, but (AFAIK) there's no way to automatically configure that setting for all contributors on repo clone, they need to manually configure it. it seems like the sort of thing that naturally belongs in rust-analyzer itself instead of each project needing to reinvent the wheel and nag everyone to fix their editor configuration.

@bjorn3
Copy link
Member

bjorn3 commented Jul 6, 2023

This would cause alternating between a rust-analyzer invoked cargo check and a user build to rebuild the build script and everything that depends on it ever time, right?

Yeah, that's what we're doing now, but (AFAIK) there's no way to automatically configure that setting for all contributors on repo clone, they need to manually configure it.

You can set all rust-analyzer configurations in a .vscode/settings.json file you check in.

@vgel
Copy link
Author

vgel commented Jul 10, 2023

This would cause alternating between a rust-analyzer invoked cargo check and a user build to rebuild the build script and everything that depends on it ever time, right?

I'm not familiar enough with how rust-analyzer interacts with the workspace to know—does rust-analyzer produce intermediates that other cargo commands can use? In that case, at the risk of making the change more involved, maybe this could be an opt-in behavior from the build script in some way? Or, instead of an environment variable, the build script could signal "skip running me entirely if you're rust-analyzer and cache nothing so I'll be re-run correctly by cargo"?

Yeah, that's what we're doing now, but (AFAIK) there's no way to automatically configure that setting for all contributors on repo clone, they need to manually configure it.

You can set all rust-analyzer configurations in a .vscode/settings.json file you check in.

Only if the entire team uses VSCode :-) We're split between {Neo,}vim, Emacs, vscode, Jetbrains... plus we sometimes roll with person-specific workspace settings, for example setting the rust-analyzer target to wasm32 when working on the wasm parts of our code, and I think that approach would overwrite those personal settings?

@bjorn3
Copy link
Member

bjorn3 commented Jul 10, 2023

Rust-analyzer uses plain cargo check with the same target dir as user initiated builds for producing diagnostics. You can set the target dir that is used using a rust-analyzer config, but that has the same issue of not being able to set it on a per project basis for non-vscode editors.

@Veykril
Copy link
Member

Veykril commented Aug 15, 2023

Coming back to this finally (sorry for the silence), I am actually wondering now how cargo handles env changes here. Does it automatically track any env vars that are queried via the corresponding compile time macros?

That aside, I do think this is the wrong way to tackle this. The ideal solution I can see is that we finally get a minimal rust-analyzer.toml working that you can check out then that supports setting the extra env stuff for the project (so that this works in a client diagnostic way).

@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 Aug 15, 2023
@bjorn3
Copy link
Member

bjorn3 commented Aug 15, 2023

For env!() rustc records a dependency in the dep-info file which causes cargo to rebuild as necessary. For proc macros there is no tracking at all. And for env::var() in build scripts, the build script has to explicitly tell cargo the fact that it depends on the env var.

@Veykril
Copy link
Member

Veykril commented Aug 29, 2023

I'll close this, I think the rust-analyzer.toml approach is the correct one here (and someone is looking into that soon), see #13529 to follow that

@Veykril Veykril closed this Aug 29, 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.

None yet

4 participants