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

canonicalize Windows environment variables to uppercase #30593

Merged
merged 6 commits into from Jan 8, 2019

Conversation

@stevengj
Copy link
Member

commented Jan 4, 2019

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.

@stevengj stevengj added the windows label Jan 4, 2019

@stevengj stevengj requested a review from tkelman Jan 4, 2019

base/env.jl Outdated Show resolved Hide resolved
@vtjnash

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

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 added 2 commits Jan 4, 2019
@stevengj

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

CI failures seem unrelated.

@stevengj

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

Hooray, green CI!

@stevengj

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Later PR

@stevengj

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

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

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

Added NEWS and documentation as suggested, thanks.

@vtjnash
vtjnash approved these changes Jan 8, 2019
@stevengj

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

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

@stevengj stevengj merged commit 6f4eb43 into JuliaLang:master Jan 8, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
julia freebsd ci Build done
Details

@stevengj stevengj deleted the stevengj:windowsenv branch Jan 8, 2019

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

(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

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

That does clarify it.

@stevengj stevengj referenced this pull request Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.