canonicalize Windows environment variables to uppercase

[stevengj]

This PR mostly fixes #29334 by canonicalizing Windows environment variables (which are case-insensitive case-preserving) to uppercase when they are iterated, copied, etcetera.

Otherwise it is easy to write buggy code for Windows by following our setenv documentation, e.g.

env = copy(ENV)
env["PATH"] = ....   # incorrect if PATH was capitalized as Path!
setenv(`some command`, env)

For example, I found code "in the wild" that does problematic things like this in Conda.jl, BinDeps.jl, Documenter.jl, the REPL, and Base.runtests — all of these examples don't take into account the case-insensitivity of Windows environment variables, and all should be fixed by this PR.

It still doesn't throw if the user explicitly writes something like merge!(ENV, Dict("key" => "1", "KEY" => "2")), but that could be addressed in a separate PR … that seems like a much less pressing problem to me.

[vtjnash]

Aside: I noticed the design here has a resource leak here for GetEnvironmentStringsW in some cases. We should probably eagerly copy the environment to an array so that we can call FreeEnvironmentStrings correctly

[stevengj]

CI failures seem unrelated.

[stevengj]

Hooray, green CI!

[stevengj]

I think this qualifies as a minor change — almost any code that this could break was buggy anyway.

For example, the following code would now break on Windows:

ENV["foo"]="bar"
env = copy(ENV)
env["foo"] # now fails because the key is called "FOO"

but this code was buggy — it would fail anyway if there were already an environment variable "Foo" or "FOO" etc., since mutating an existing environment variable preserves the old case.

In practice, I looked over the existing packages that call copy(ENV) and it doesn't look like any of them will be broken by this change, and in fact nearly all of them are buggy without this change — they invariably assume that all environment variables are uppercase.

[stevengj]

@vtjnash, can we leave fixing the potential resource leak (for when you don't finish the iterator) for another PR, since that is an unrelated issue? Or do you want me to fix it here?

[vtjnash]

Later PR

[stevengj]

@StefanKarpinski, do you have any opinion on this? It's a slightly tricky case for compatibility vs correctness.

[StefanKarpinski]

I think it seems like a good idea and you have gone to admirable lengths to make it correct (i.e. using Windows own case tables), not just almost correct (using our own case tables). Seems good for a minor release to me. Should have a NEWS entry and maybe a mention in the ENV docs.

[stevengj]

Added NEWS and documentation as suggested, thanks.

[stevengj]

CI failures are an unrelated Travis glitch (and everything was green before the latest commit, which just updated documentation).

I think this should be good to merge.

[vtjnash]

Tried to kick them (travis failed to start the jobs), although still gtg whenever you want to merge from my perspective.

[StefanKarpinski]

(On Windows, system environment variables are case-insensitive, and ENV correspondingly converts all keys to uppercase for display, iteration, and copying. Portable code should not rely on the existence of lower-case environment variables or on the ability to distinguish variables by case.)

Since this is what we're recommending, wouldn't it make sense to just convert ENV keys to uppercase on Windows on insertion and lookup as well? Or is that effectively what happens already? In that case shouldn't we delete the "on the existence of lower-case environment variables" part since it's impossible to rely on that on Windows?

[stevengj]

Since this is what we're recommending, wouldn't it make sense to just convert ENV keys to uppercase on Windows on insertion and lookup as well? Or is that effectively what happens already?

ENV[key] access is already case-insensitive. However, I don't think we should explicitly convert keys to uppercase in getenv/setenv, because:

• ENV access is case-preserving, so even we convert all keys to uppercase, ENV["FOO"]="bar" could still result in a lower-case "foo" variable internally if "foo" was already defined. (Unless you propose that every setenv call first deletes any existing environment variable, which I don't think we should do.) After this PR, the internal preserved case is mostly hidden from view, however — it appears as if the variables were all uppercase.

• If for some obscure reason you need to set a lowercase environment variable (e.g. a subprocess checks the case for some reason), I'd hate to lose that ability entirely. Right now, you can still set the internal preserved case if absolutely necessary by delete!(ENV, key)[key] = foo.

In that case shouldn't we delete the "on the existence of lower-case environment variables" part since it's impossible to rely on that on Windows?

I'm not sure what you mean? It already says that portable code should not rely on lower case being preserved.

[StefanKarpinski]

I guess my issue is about what the meaning of that sentence fragment is. Does it mean that the programmer should be aware that ENV["key"] = val may not result in the lowercase key key existing since it may, if KEY is already a key`, for example, end up being a different case? If so, perhaps that should be spelled out explicitly? I'm really wondering, given the changes here, under what circumstances would "rely[ing] on lower case being preserved" still be a problem?

[stevengj]

Stefan, relying on lower case being preserved also was and is a problem if you make a copy of ENV, as described above: #30593 (comment)

[stevengj]

How about rephrasing it as:

(On Windows, system environment variables are case-insensitive, and ENV correspondingly converts all keys to uppercase for display, iteration, and copying. Portable code should not rely on the ability to distinguish variables by case, and should beware that setting an ostensibly lowercase variable may result in an uppercase ENV key.)

[StefanKarpinski]

That does clarify it.