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

build.rs "rerun-if-env-changed" goes out of sync with the actual use site of the env var. #70517

Closed
eddyb opened this issue Mar 28, 2020 · 4 comments · Fixed by #71858
Closed
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 28, 2020

This is still in rustc_codegen_llvm's build script:

println!("cargo:rerun-if-env-changed=CFG_PREFIX");

But the use site has been moved here:

let install_prefix = option_env!("CFG_PREFIX").expect("CFG_PREFIX");

This means that rustc_codegen_ssa won't be rebuilt when that env var changes. For the record, I haven't seen a bug from this, was just curious where CFG_PREFIX may be used and spotted the mismatch.

cc @bjorn3

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-codegen Area: Code generation labels Mar 28, 2020
@eddyb
Copy link
Member Author

eddyb commented Mar 31, 2020

Found another one: CFG_COMPILER_HOST_TRIPLE is used in src/librustc_session/config.rs but it's still mentioned by src/librustc_middle/build.rs.

cc @Mark-Simulacrum Could we automate ensuring this? e.g. by searching for env! in tidy.

@Mark-Simulacrum
Copy link
Member

Yeah, we can try I guess. Not sure :)

Maybe the right solution is that Cargo would, other than some whitelist, remove unknown variables from rustc's environment -- and the whitelist can be expanded via the build script env-tracking.

@eddyb
Copy link
Member Author

eddyb commented Mar 31, 2020

@Mark-Simulacrum hmm I wonder if abusing the .d file format (from --emit=dep-info) or encoding this extra info in a comment, would be a good idea.

@Mark-Simulacrum
Copy link
Member

You mean such that if you use env! we automatically tell Cargo to rebuild on changes? (That's #40364).

But yeah, I think that would be good. Ideally this would be "well-integrated" vs. the current state of everyone having to remember to do it.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 25, 2020
Print environment variables accessed by rustc as special comments into depinfo files

So cargo (and perhaps others tools) can use them for linting (at least) or for actually rebuilding crates on env var changes.

---
I've recently observed one more forgotten environment variable in a build script rust-lang@8a77d1c and thought it would be nice to provide the list of accessed variables to cargo automatically as a part of depinfo.

Unsurprisingly, I wasn't the first who had this idea - cc rust-lang#70517 rust-lang#40364 rust-lang#44074.

Also, there are dozens of uses of `(option_)env!` in rustc repo and, like, half of them are not registered in build scripts.

---
Description:
- depinfo files are extended with special comments containing info about environment variables accessed during compilation.
- Comment format for environment variables with successfully retrieved value: `# env-dep:KEY=VALUE`.
- Comment format for environment variables without successfully retrieved value: `# env-dep:KEY` (can happen with `option_env!`).
- `KEY` and `VALUE` are minimally escaped (`\n`, `\r`, `\\`) so they don't break makefile comments and can be unescaped by anything that can unescape standard `escape_default` and friends.

FCP report: rust-lang#71858 (comment)

Closes rust-lang#70517
Closes rust-lang#40364
Closes rust-lang#44074
A new issue in the cargo repo will be needed to track the cargo side of this feature.

r? @ehuss
@bors bors closed this as completed in 1033351 Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
3 participants