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 std::env::vars() uppercases environment variables #85242

Closed
sophiajt opened this issue May 13, 2021 · 3 comments · Fixed by #85270
Closed

Windows std::env::vars() uppercases environment variables #85242

sophiajt opened this issue May 13, 2021 · 3 comments · Fixed by #85270
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@sophiajt
Copy link
Contributor

I tried this code:

    for v in std::env::vars() {
        println!("{:?}", v);
    }

I expected to see this happen: a case-sensitive list of environment variables and values, matching the output of Environment.GetEnvironmentVariables from C# and .NET and the output of Get-ChildItem Env: from PowerShell.

It seems we should align the std::env::vars() output to match the modern APIs for Windows that Microsoft provides.

Instead, this happened: std::env::vars() returns all environment variable names in all uppercase.

Rust via std::env::vars():

SYSTEMDRIVE = C:
<snip>

C# via Environment.GetEnvironmentVariables:

SystemDrive = C:
<snip>

Meta

rustc --version --verbose:

>rustc --version --verbose
rustc 1.51.0-nightly (04caa632d 2021-01-30)
binary: rustc
commit-hash: 04caa632dd10c2bf64b69524c7f9c4c30a436877
commit-date: 2021-01-30
host: x86_64-pc-windows-msvc
release: 1.51.0-nightly
LLVM version: 11.0.1

@ehuss
Copy link
Contributor

ehuss commented May 13, 2021

I don't think this is related to reading environment variables, but is related to process spawning. If you are spawning a process from a Rust program, then they will be uppercased. But if the Rust program is spawned from something else (like PowerShell), then the env keys should be case-preserved.

The issue is here. This was originally added in #17249 to fix #16937. I personally don't think that issue is all that important that all environment keys should be forced to be uppercase. However, if there is a desire to fix both issues, then there would need to be a case-insensitive map (maybe just keep a separate set of uppercased keys?).

@sophiajt
Copy link
Contributor Author

@ehuss thanks for the pointer. For our case, Nushell should be able to spawn processes and keep the environment as expected rather than uppercasing. This allows Nushell to work just as PowerShell spawning a program.

As best as I can tell, it's the lookup of a single variable that should be case-insensitive and that listing of variables should maintain the case they had, though I'm far from an expert here.

@jonas-schievink jonas-schievink added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 13, 2021
@ChrisDenton
Copy link
Contributor

ChrisDenton commented May 13, 2021

Hm, the current code does look wrong. Windows environment variables are compared case-insensitive but when set they keep the same case. The simplest improvement here would be remove the make_ascii_uppercase and instead implement Ord, and PartialEq in a way that ignores case differences.

However, note that Windows uses Unicode casing, not just ASCII. There is a Windows function for comparing but using it for Ord would mean lots of conversions from OsString to the Windows string type (and back again). Calling Rust's Unicode functions with a WTF-8 OsString looks tricky too (though maybe there's something I'm missing). I'm uncertain how to approach a proper fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
4 participants