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

Windows environment variables are case preserving (but case-insensitive) #9630

Closed
ChrisDenton opened this issue Jun 27, 2021 · 3 comments · Fixed by #9646
Closed

Windows environment variables are case preserving (but case-insensitive) #9630

ChrisDenton opened this issue Jun 27, 2021 · 3 comments · Fixed by #9646
Labels
A-configuration Area: cargo config files and env vars A-environment-variables Area: environment variables C-bug Category: bug O-windows OS: Windows

Comments

@ChrisDenton
Copy link
Member

ChrisDenton commented Jun 27, 2021

I recently submitted a PR to stop Rust ASCII upper casing environment variables: rust-lang/rust#85270. This causes the following test to fail.

let target = rustc_host();
let env_key = format!(
"CARGO_TARGET_{}_LINKER",
target.to_lowercase().replace('-', "_")
);
let mut execs = p.cargo("build -v --target");
execs.arg(target).env(&env_key, "nonexistent-linker");
if cfg!(windows) {
// Windows env keys are case insensitive, so no warning, but it will
// fail due to the missing linker.
execs
.with_stderr_does_not_contain("warning:[..]")
.with_status(101);

I think it's because something relies on a new process upper casing key names? Which only happened before because Rust manually ASCII upper cased keys when starting a new process.


Hm, that seems to be the assumption here:

fn check_environment_key_case_mismatch(&self, key: &ConfigKey) {
if cfg!(windows) {
// In the case of windows the check for case mismatch in keys can be skipped
// as windows already converts its environment keys into the desired format.
return;
}

Whereas Windows itself does no such conversion.


cc @ehuss ? (only because you most recently touched the relevant test 🙂)

@ChrisDenton ChrisDenton added the C-bug Category: bug label Jun 27, 2021
@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jun 27, 2021

Here's a quick summary of the issue the linked PR seeks to address (using a few simple Rust programs you can try):

listenv.rs:

fn main() {
    for (key, _) in std::env::vars() {
        if key != key.to_ascii_uppercase() {
            println!("{}", key);
        }
    }
}

spawn.rs:

fn main() -> std::io::Result<()> {
    std::process::Command::new(r".\listenv.exe").spawn()?.wait()?;
    Ok(())
}

withenv.rs

fn main() ->  std::io::Result<()> {
    std::process::Command::new(r".\listenv.exe").env("hello", "world").spawn()?.wait()?;
    Ok(())
}

Compile all these with rustc:

Z:\test> rustc listenv.rs
Z:\test> rustc spawn.rs
Z:\test> rustc withenv.rs

If you run .\listenv.exe you should see environment keys with lower case names (e.g. "windir", "Path", "ProgramFiles", etc). The same goes for .\spawn.exe. However .\withenv.exe invokes Rust's ASCII upper casing behaviour and therefore all keys are upper cased (or were, if my PR gets merged).

Note that in any case, std::env::var(key) will work case-insensitively.

@ehuss
Copy link
Contributor

ehuss commented Jul 2, 2021

I posted #9646 to disable the test on Windows for now. Once that is merged, I'll update rust-lang/rust, and then re-approve your PR. Afterwards, I'll work on fixing Cargo to work with the preserved casing.

@ehuss ehuss added A-configuration Area: cargo config files and env vars A-environment-variables Area: environment variables O-windows OS: Windows labels Jul 2, 2021
@ChrisDenton
Copy link
Member Author

Thanks! I've added a comment to my PR with a (hopefully accurate) summary of the issue.

@bors bors closed this as completed in 6e31c13 Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-environment-variables Area: environment variables C-bug Category: bug O-windows OS: Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants