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

[WIP] Fix: Cargo fails to detect environment variable #13596

Closed
wants to merge 3 commits into from

Conversation

LuuuXXX
Copy link
Contributor

@LuuuXXX LuuuXXX commented Mar 16, 2024

What does this PR try to resolve?

Fixes: #13280

Cargo fails to detect and rebuild while using environment variable reset environment variable which is set in config.toml.

examples:
.cargo/config.toml:

[env]
Z = "0"

src/main.rs:

fn main() {
    println!("{:?}", option_env!("Z"));
}

run these commands:

cargo run     # first build, output None
Z=1 cargo run # cargo does know the environment variable is set and rebuilds it, output Some("1")

How should we test and review this PR?

checkout and run test:

cargo test --package cargo --test testsuite -- cargo_env_config::env_reset

Additional information

Fingerprint code is not able to detect environment variables cause they were removed from Cargo's dep-info. See #13280 (comment).

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2024
@LuuuXXX LuuuXXX changed the title Issue 13280 Fix: Cargo fails to detect environment variable Mar 16, 2024
@ehuss
Copy link
Contributor

ehuss commented Mar 16, 2024

I would expect this detection to be in the fingerprint code. There is code there already for managing changes in environment variables. Can you say why the current fingerprinting code is not catching this case?

@weihanglo
Copy link
Member

weihanglo commented Mar 16, 2024

@ehuss
Environment variables were removed from Cargo's dep-info because they were in rustc ProcessBuilder. See #13280 (comment).

@weihanglo
Copy link
Member

And agree with Eric on this being fixed in fingerprint.

@rustbot rustbot added the A-rebuild-detection Area: rebuild detection and fingerprinting label Mar 16, 2024
@LuuuXXX
Copy link
Contributor Author

LuuuXXX commented Mar 16, 2024

Thanks to @weihanglo for the additional explanation, it has been added to Additional information.

@LuuuXXX
Copy link
Contributor Author

LuuuXXX commented Mar 18, 2024

@rustbot ready

let dirty_reason = match dirty_reason {
Some(dr) => dr,
None => {
let Some(dr) = env_config_modified(bcx.gctx) else {
Copy link
Member

Choose a reason for hiding this comment

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

While this does fix the case described in #13280, the actual problem is not addressed: Dep-info from Cargo doesn't track # env-var: from rustc's dep-info files.

It's simple to reproduce delete the .cargo/config.toml from the case in #13280, then run

cargo r --config 'env.Z="1"'
cargo r --config 'env.Z="2"'
cargo r --config 'env.Z="3"'

Only Z=1 would be captured, and Cargo never recompile after the first invocation. Hence, the current patch just solves half of it and buries the real bug deeper.

Copy link
Member

Choose a reason for hiding this comment

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

Wonder some alternatives may be like:

  • Have a set of environment variables Cargo internally sets for rustc, and removes them from Cargo's dep info here. This might need to take cargo::rustc-env build script directive into account, and I feel it will be pretty error-prone as we might miss something when adding more envs.
  • Leverage the unstable --env-set rustc flag from the env sandboxing RFC. However, it is still unstable so Cargo can't use it.

@LuuuXXX LuuuXXX changed the title Fix: Cargo fails to detect environment variable [WIP] Fix: Cargo fails to detect environment variable Mar 18, 2024
@LuuuXXX LuuuXXX marked this pull request as draft March 25, 2024 06:24
@weihanglo
Copy link
Member

@LuuuXXX are you still interested in moving this forward?

@weihanglo
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024
@LuuuXXX
Copy link
Contributor Author

LuuuXXX commented Jul 16, 2024

Sorry for missing message,I apologize for not having the time to work on it in the coming month,a few things are tripping me up.

@bors
Copy link
Contributor

bors commented Oct 8, 2024

☔ The latest upstream changes (presumably #14137) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
Copy link
Member

Going to close due to inactivity. We are still interested in some solutions here. Feel free to reopen if you have a chance to work on it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
5 participants