SetPropertiesFromEnv should be case-insensitive on Windows

[dabrahams]

I did

buildbot.steps.slave.SetPropertiesFromEnv(
    name='get-slave-env', variables=['VS90COMNTOOLS','ProgramFiles(x86)','ProgramFiles','LLVM_LIT_TOOLS_DIR']))

These are the actual names of the environment variables as listed by set at the slave's command-prompt. However, BB didn't set them. Looking at the logs, apparently it sees these variables as all-uppercase.

[dabrahams]

sorry, should have said: "BB didn't set *all of* them." It set the ones that were all-uppercase in my code.

[dustin]

Can you add the log here, too, so I can see what you're seeing?

[tom.prince]

What is the output of python -c 'import os; print os.environ' on the slave. That is what is the slave sees and passes to the master. I think you may just want to use all uppercase in your code.

[dabrahams]

I eventually worked around it that way, but IMO this is still a bug in BuildBot. It should recognize that the slave is a Windows machine and upcase the request for variable values inside of SetPropertiesFromEnv?.

[tom.prince]

I am inclined to follow pythons lead, and leave this up to the user. In addition, I wonder if windows actually supports case-sensitivity in variable names, and cmd.exe just coerces everything to uppercase. (i.e. if putenv allows you to create environment variables with lowercase names)

[dabrahams]

Replying to tom.prince:

I am inclined to follow pythons lead, and leave this up to the user.

Leave what to the user? Figuring out that the system is squashing everything to uppercase and making the necessary adjustments? In case you think otherwise, you're not currently offering any flexibility in exchange for a serious usability impediment.

In addition, I wonder if windows actually supports case-sensitivity in variable names,

It does, in the sense that if you ask it to list the environment variables, it will show them to you with the case you originally specified them with. It does not, in the sense that you can't have two separate variables called "Foo" and "foo".

and cmd.exe just coerces everything to uppercase.

cmd.exe does not coerce.

[dustin]

Replying to dabrahams:

In addition, I wonder if windows actually supports case-sensitivity in variable names,

It does, in the sense that if you ask it to list the environment variables, it will show them to you with the case you originally specified them with. It does not, in the sense that you can't have two separate variables called "Foo" and "foo".

This is called case-preserving but case-insensitive, and it's the way NTFS filenames work, for example.

and cmd.exe just coerces everything to uppercase.

cmd.exe does not coerce.

We don't actually know what is going on - the questions in comment 2 and comment 3 are unanswered. Also, we need to know how you've installed Buildbot - msys? cygwin? native Python? Does it behave the same way for all three?

In general, Buildbot is not going to try to un-hork windows horked-ness. That way lies madness. I'm happy to add hints to the documentation, but they should explain, rather than direct, and should be a comprehensive description of what's going wrong.

[dabrahams]

Replying to dustin:

Replying to dabrahams:

In addition, I wonder if windows actually supports case-sensitivity in variable names,

It does, in the sense that if you ask it to list the environment variables, it will show them to you with the case you originally specified them with. It does not, in the sense that you can't have two separate variables called "Foo" and "foo".

This is called case-preserving but case-insensitive, and it's the way NTFS filenames work, for example.

Right. And HFS+, by default.

and cmd.exe just coerces everything to uppercase.

cmd.exe does not coerce.

We don't actually know what is going on - the questions

I don't mean to be blunt or uncooperative, but IMO the answers to these questions are irrelevant. Still, I'll try to provide them anyway.

in comment 2

Do you want me to undo my workarounds first? And do you want the log on the slave or master or...?

and comment 3 are unanswered.

The output has all-uppercase variable names. But again, that's irrelevant IMO. I can do os.environ['TMP'] or os.environ['Tmp'] in python and I'll get the same result. That is, Python has to pick some representation for the case of variable names, and it chooses ALL_UPPERCASE for that, but when I ask for a particular variable name, it does the right thing regardless of case. BB does not behave similarly, and it should.

Also, we need to know how you've installed Buildbot - msys? cygwin? native Python? Does it behave the same way for all three?

Native Python, but again IMO the answer to whether it behaves the same way is irrelevant, and would take an inordinate effort to discover for all three.

In general, Buildbot is not going to try to un-hork windows horked-ness. That way lies madness.

If you think the idea case-insensitive-but-case-preserving environment variables is essentially horked, then I lose (along with everyone else who needs to run tests on windows among other platforms) and I'll stop wasting everyone's time now. Is that your position?

I'm happy to add hints to the documentation, but they should explain, rather than direct, and should be a comprehensive description of what's going wrong.

No, that way lies madness. This should "just work." There's no reason for it not to, and there's no reason to burden users with an explanation of why it doesn't, and how to work around the problem.

[dustin]

First, your tone is accusatory and aggressive, and such a tone is not welcome in the Buildbot community. As maintainer, I will try to look past that and solve the bug, but please moderate your approach in this and any other interactions with Buildbot.

So let's focus on the issue at hand, which I still do not completely understand. Hopefully you can fill in any gaps.

SetPropertiesFromEnviron, if given a mixed-case variable name, will fail to set the corresponding property. If given the same name, but uppercased, then it will succeed. So, for example, 'ProgramFiles(x86)' will fail, but 'PROGRAMFILES(X86)' will succeed.

On a Windows system - at least a native install (yours) and in my testing with an msys install - os.environ is a case-insensitive but non-case-preserving dictionary (it folds to uppercase). That is:

>>> os.environ['fOo'] = 'BAR'
>>> os.environ['foo']
BAR
>>> os.environ['FOO']
BAR
>>> [ k for k in os.environ.keys() if k.upper() == 'FOO' ]
['FOO']

The environment dumped by Buildbot for a buildstep (e.g.,  http://buildbot.buildbot.net/builders/os-winxp/builds/85/steps/test%20slave/logs/stdio) is built from os.environ, so the variables there will always appear in uppercase.

That environment is transmitted to the buildmaster when the slave connects, and appears on the master as a normal (case-sensitive, case-preserving) Python dictionary. So on the master, slave_environ['fOO'] will fail, but slave_environ['FOO'] will succeed. Which matches dabrahams' observations in comment 0.

Internally, then, Buildbot is consistent - it displays environment variables in uppercase in build logs, and expects to see variables treated similarly in SetPropertiesFromEnv.

The difficulty of the suggestion to make this "just work" on windows is that the implementation of SetPropertiesFromEnv is entirely master-side, and doesn't have a good way of knowing the slave's OS type.

So, we have

• internal consistency,
• high difficulty to make the "right" fix, and
• an easy workaround for users (just use uppercase in SetPropertiesFromEnv).

I'd like to find a low-impact solution. Off the top of my head, we could

1. Recommend in the documentation that users spell environment variables as they appear in the build logs, noting that on Windows that is generally in uppercase
2. Add an argument to SetPropertiesFromEnviron to do the folding, e.g.,

   SetPropertiesFromEnv(variables=['FoO'], uppercase=True)
   

3. Fall back to the uppercased version of the variable if the name as given does not exist in the dictionary (with documentation of this behavior).

I'm most in favor of the third option. What do you think?

[dabrahams]

First, I apologize for the escalation in tone. I assure you, neither aggression nor accusation was intended, but perception is everything, so again, I apologize. Also, I understand that making it work properly is high-impact compared with the scale of the problem.

I evaluate the options as follows: #1 helps, but makes it difficult to write portable test recipes and requires some intricate documentation, making SetPropertiesFromEnv harder to use for everyone, #2 doesn't help at all (if you know to pass the uppercase=True argument, you would also know to upcase the variable name), and #3 looks like a bit of a hack that introduces a (admittedly small) maintenance hassle for you, still needs to be documented (and thus decreases usability) and could theoretically cause problems in *nix-only environments. That said, those theoretical problems look very unlikely, so maybe #3 is the least of all evils. I'd say #1 or something like it runs a close second.

[Dustin J. Mitchell]

SetPropertiesFromEnv? folds to uppercase for Windows slaves

Windows environment variables are case-insensitive but in some displays are case-preserving. The step now detects when the slave is win32, and behaves accordingly on the master side by folding keys to uppercase. Fixes #1892.

Changeset: 8a5076796e2519f543618a4736c57ca34903a0a3

[Dustin J. Mitchell]

clearer docs for previous commit

Refs #1892.

Changeset: b52246900bd0dfc48e1f8d5a8f71e0c86e85da53