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

Config env snapshot doesn't handle case-insensitive env vars correctly on windows #11814

Closed
sokra opened this issue Mar 9, 2023 · 3 comments · Fixed by #11824
Closed

Config env snapshot doesn't handle case-insensitive env vars correctly on windows #11814

sokra opened this issue Mar 9, 2023 · 3 comments · Fixed by #11824
Labels
A-environment-variables Area: environment variables C-bug Category: bug O-windows OS: Windows P-high Priority: High regression-from-stable-to-beta Regression in beta that previously worked in stable.

Comments

@sokra
Copy link

sokra commented Mar 9, 2023

Problem

Some recent changes introduce snapshotting of env variables:

/// Environment variables, separated to assist testing.
env: HashMap<String, String>,

e. g. this PR #11727 by @kylematsuda

That introduces a little bug on windows where env vars are case-insensitive. env::var_os would handle that correctly, but config.get_env_os does a HashMap lookup which is case-sensitive.

e. g. the mentioned PR breaks the lookup of cargo subcommands on windows (where the env var is called Path instead of PATH).

Steps

  1. On windows install a cargo subcommand into a different directory
  2. Add the directory to your PATH
  3. Run cargo
  4. Cargo says: no such command: <subcommand>

Workaround:

set PATH2=%PATH%
set PATH=
set PATH=%PATH2%
set PATH2=

This changes the casing of the env variable.

Possible Solution(s)

Wrap the keys in the env snapshot HashMap with some case-insenstive NewType struct.

Notes

No response

Version

nightly-2023-03-01
@sokra sokra added the C-bug Category: bug label Mar 9, 2023
@weihanglo weihanglo added O-windows OS: Windows A-environment-variables Area: environment variables labels Mar 9, 2023
@ehuss ehuss added P-high Priority: High regression-from-stable-to-beta Regression in beta that previously worked in stable. labels Mar 9, 2023
@kylematsuda
Copy link
Contributor

Hi, thanks for the detailed explanation. Sorry about this, I didn't know that env vars are case-insensitive on Windows until now.

The Config has another hashmap (upper_case_env) that contains the environment variables converted to upper case. @ehuss, do you think it might be an acceptable workaround to (1) convert the variable name to upper case, then (2) check in upper_case_env if the variable is not found in env (on Windows only)?

@ehuss
Copy link
Contributor

ehuss commented Mar 9, 2023

Hm, I don't think that will work perfectly since upper_case_env modifies - to _ also. I can't offhand think of way to do this without having two separate maps (one with - converted, and one without).

I believe Windows does case conversion a little differently from how Rust does it (and it depends on the version of Windows), but I think for our purposes, to_uppercase() should be ok.

@ChrisDenton
Copy link
Member

Btw, I wrote a detailed comment on a similar issue elsewhere.

bors added a commit that referenced this issue Mar 17, 2023
Handle case mismatches when looking up env vars in the Config snapshot

### What does this PR try to resolve?

Fixes #11814.

Windows environment variables are case-insensitive, which causes problems when looking them up in the `Config` env snapshot.

This PR adds another member (`case_insensitive_env`) in `Config` that maps upper-cased keys to their original values in the env (for example, `"PATH" => "Path"`). If lookup in `self.env` fails, this PR converts the key to upper case and looks it up in `self.case_insensitive_env` to obtain the correct key name if it exists (on Windows only).

### How should we test and review this PR?

Please see the new tests in `testsuite/config.rs` and `testsuite/cargo_command.rs`.

### Additional information

Currently, this uses `str::to_uppercase` to uppercase the keys. This requires key to be valid UTF-8, and may disagree with how the OS uppercases things (see the link in [this comment](#11814 (comment)) for details).
@bors bors closed this as completed in c9faf70 Mar 17, 2023
weihanglo pushed a commit to weihanglo/cargo that referenced this issue Mar 17, 2023
…=ehuss

Handle case mismatches when looking up env vars in the Config snapshot

### What does this PR try to resolve?

Fixes rust-lang#11814.

Windows environment variables are case-insensitive, which causes problems when looking them up in the `Config` env snapshot.

This PR adds another member (`case_insensitive_env`) in `Config` that maps upper-cased keys to their original values in the env (for example, `"PATH" => "Path"`). If lookup in `self.env` fails, this PR converts the key to upper case and looks it up in `self.case_insensitive_env` to obtain the correct key name if it exists (on Windows only).

### How should we test and review this PR?

Please see the new tests in `testsuite/config.rs` and `testsuite/cargo_command.rs`.

### Additional information

Currently, this uses `str::to_uppercase` to uppercase the keys. This requires key to be valid UTF-8, and may disagree with how the OS uppercases things (see the link in [this comment](rust-lang#11814 (comment)) for details).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-environment-variables Area: environment variables C-bug Category: bug O-windows OS: Windows P-high Priority: High regression-from-stable-to-beta Regression in beta that previously worked in stable.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants