[Buildbot-commits] [Buildbot] #1892: SetPropertiesFromEnv should be case-insensitive on Windows

[Buildbot]

#1892: SetPropertiesFromEnv should be case-insensitive on Windows
------------------------+------------------------
Reporter:  dabrahams    |       Owner:
    Type:  defect       |      Status:  new
Priority:  major        |   Milestone:  undecided
 Version:  0.8.3p1      |  Resolution:
Keywords:  windows git  |
------------------------+------------------------

Comment (by 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
  1. Add an argument to !SetPropertiesFromEnviron to do the folding, e.g.,
     {{{
     SetPropertiesFromEnv(variables=['FoO'], uppercase=True)
     }}}
  1. 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?

-- 
Ticket URL: <http://trac.buildbot.net/ticket/1892#comment:9>
Buildbot <http://buildbot.net/>
Buildbot: build/test automation