• Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • core
    • None

      StringParameterValue.buildEnvVars says

      env.put(name.toUpperCase(),value);

      Say the user specified 'includedTests' in the job configuration and used an Ant script:

      <property env="env."/>
      <junit ... includes="${env.includedTests}"/>

      First of all, this will not work because only ${env.INCLUDEDTESTS} will be defined. It is nice that name.html says this will happen, but you would have to click the help button to see this unexpected behavior.

      Second, if you happen to run the Hudson server in Turkey, even that won't work because the environment variable will be ${env.İNCLUDEDTESTS} due to the locale-sensitive upcasing!

      At a minimum, all usages of String.toUpper/LowerCase() in Hudson should be reviewed to see if passing Locale.ENGLISH would be more appropriate.

      In the case of *ParameterValue.buildEnvVars, it should be reconsidered whether automatic upcasing is even desirable; surely it is clearer to use an uppercase variable name in the job config if that is what you want to see during the build. (For compatibility, you could set both the upcased string and the original string, and remove the note about upcasing from the help text.)

          [JENKINS-5391] Dubious use of String.toUpperCase()

          Code changed in hudson
          User: : kohsuke
          Path:
          trunk/hudson/main/core/src/main/java/hudson/EnvVars.java
          trunk/hudson/main/core/src/main/java/hudson/model/BooleanParameterValue.java
          trunk/hudson/main/core/src/main/java/hudson/model/JobParameterValue.java
          trunk/hudson/main/core/src/main/java/hudson/model/ParameterValue.java
          trunk/hudson/main/core/src/main/java/hudson/model/RunParameterValue.java
          trunk/hudson/main/core/src/main/java/hudson/model/StringParameterValue.java
          trunk/www/changelog.html
          http://jenkins-ci.org/commit/26640
          Log:
          [FIXED JENKINS-5391] in 1.344. we should be case insensitive but case preserving.

          SCM/JIRA link daemon added a comment - Code changed in hudson User: : kohsuke Path: trunk/hudson/main/core/src/main/java/hudson/EnvVars.java trunk/hudson/main/core/src/main/java/hudson/model/BooleanParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/JobParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/ParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/RunParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/StringParameterValue.java trunk/www/changelog.html http://jenkins-ci.org/commit/26640 Log: [FIXED JENKINS-5391] in 1.344. we should be case insensitive but case preserving.

          Jesse Glick added a comment -

          Note that there remain a number of suspect usages of String.toUpper/LowerCase in Hudson code; for example, in LogRecorderManager:

          @QueryParameter String level;
          // ...
          lv = Level.parse(level.toUpperCase());

          will throw an exception if ?level=info is passed to a server running in Turkish (or Azeri) locale.

          (I have lobbied to get these two methods @Deprecated since nearly all calls turn out to be incorrect on inspection, and the remainder can more clearly be expressed as to*Case(Locale.getDefault()); no one seems to be listening so far.)

          Jesse Glick added a comment - Note that there remain a number of suspect usages of String.toUpper/LowerCase in Hudson code; for example, in LogRecorderManager: @QueryParameter String level; // ... lv = Level.parse(level.toUpperCase()); will throw an exception if ?level=info is passed to a server running in Turkish (or Azeri) locale. (I have lobbied to get these two methods @Deprecated since nearly all calls turn out to be incorrect on inspection, and the remainder can more clearly be expressed as to*Case(Locale.getDefault()); no one seems to be listening so far.)

          Code changed in hudson
          User: : kohsuke
          Path:
          trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorder.java
          trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorderManager.java
          trunk/hudson/main/core/src/main/java/hudson/model/FingerprintMap.java
          trunk/hudson/main/core/src/main/java/hudson/model/Hudson.java
          trunk/hudson/main/core/src/main/java/hudson/model/MultiStageTimeSeries.java
          trunk/hudson/main/core/src/main/java/hudson/node_monitors/DiskSpaceMonitorDescriptor.java
          trunk/hudson/main/core/src/main/java/hudson/tools/JDKInstaller.java
          trunk/hudson/main/core/src/main/java/hudson/util/FormFieldValidator.java
          trunk/hudson/main/core/src/main/java/hudson/util/FormValidation.java
          trunk/hudson/main/core/src/main/java/hudson/util/TableNestChecker.java
          trunk/hudson/main/core/src/main/java/hudson/util/VersionNumber.java
          trunk/hudson/main/remoting/src/main/java/hudson/remoting/Launcher.java
          trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/HudsonPageCreator.java
          trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/recipes/PresetData.java
          trunk/hudson/main/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java
          http://jenkins-ci.org/commit/26650
          Log:
          JENKINS-5391 Fixed the infamous Turkish bug.

          SCM/JIRA link daemon added a comment - Code changed in hudson User: : kohsuke Path: trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorder.java trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorderManager.java trunk/hudson/main/core/src/main/java/hudson/model/FingerprintMap.java trunk/hudson/main/core/src/main/java/hudson/model/Hudson.java trunk/hudson/main/core/src/main/java/hudson/model/MultiStageTimeSeries.java trunk/hudson/main/core/src/main/java/hudson/node_monitors/DiskSpaceMonitorDescriptor.java trunk/hudson/main/core/src/main/java/hudson/tools/JDKInstaller.java trunk/hudson/main/core/src/main/java/hudson/util/FormFieldValidator.java trunk/hudson/main/core/src/main/java/hudson/util/FormValidation.java trunk/hudson/main/core/src/main/java/hudson/util/TableNestChecker.java trunk/hudson/main/core/src/main/java/hudson/util/VersionNumber.java trunk/hudson/main/remoting/src/main/java/hudson/remoting/Launcher.java trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/HudsonPageCreator.java trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/recipes/PresetData.java trunk/hudson/main/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java http://jenkins-ci.org/commit/26650 Log: JENKINS-5391 Fixed the infamous Turkish bug.

          mcrooney added a comment -

          This actually breaks any existing parameterized builds which have different casing, so the changelog should probably have a strongly emphasized warning that upgrading has the possibility of breaking parameterized builds.

          For example, say we have parameters "foo" and "bar", and are referring to them in our jobs via the environment variables "FOO" and "BAR" (because we have to because of the forced upper casing). After upgrading, these environment variables no longer exist and the jobs no longer run. I can of course change the casing to match either way but, it would have been nice to know before upgrading that I might have broken jobs on my hands so I can fix them before they all blow up

          mcrooney added a comment - This actually breaks any existing parameterized builds which have different casing, so the changelog should probably have a strongly emphasized warning that upgrading has the possibility of breaking parameterized builds. For example, say we have parameters "foo" and "bar", and are referring to them in our jobs via the environment variables "FOO" and "BAR" (because we have to because of the forced upper casing). After upgrading, these environment variables no longer exist and the jobs no longer run. I can of course change the casing to match either way but, it would have been nice to know before upgrading that I might have broken jobs on my hands so I can fix them before they all blow up

          Jesse Glick added a comment -

          Hence my suggestion that "for compatibility, you could set both the upcased string and the original string".

          Given the new usage of EnvVars (and that its expand method should be case-insensitive already), probably this would be accomplished where ProcessBuilder.environment() is populated from EnvVars - in Launcher and Proc and perhaps elsewhere.

          Jesse Glick added a comment - Hence my suggestion that "for compatibility, you could set both the upcased string and the original string". Given the new usage of EnvVars (and that its expand method should be case-insensitive already), probably this would be accomplished where ProcessBuilder.environment() is populated from EnvVars - in Launcher and Proc and perhaps elsewhere.

          mcrooney added a comment -

          Still seems to be broken on 1.345, I have a failing parameterized build and doing an 'env' in a build step shows only the lowercase versions defined.

          mcrooney added a comment - Still seems to be broken on 1.345, I have a failing parameterized build and doing an 'env' in a build step shows only the lowercase versions defined.

          vkodocha added a comment -

          I've created the issue 5843 which is basically a duplicate of this here.

          I've seen that the backwards compatibility should be solved since version 1.345. And I also looked at the code of the current version. The code looks fine but it does not work for me.

          I have tested this with a clean install of Hudson 1.351, created a new job with one parameter with a lowercase name, and one build step to show all environment vars. I tested this on Mac OS X 10.6 and on a Vista machine with the same result for both platforms. Only the lowercase variable is set, the uppercase variant is just missing.

          On windows it seams not to be working as the environments vars seams to be ignoring case. Meaning a set hello=5 and then a set HELLO=10 will result in a variable named hello=10 to be set. But on mac this is not the case so I don't know why it is not working.
          (I don't have a problem with it not woking under Windows as I use Mac but I just wanted to verify that it's not a mac specific problem).

          vkodocha added a comment - I've created the issue 5843 which is basically a duplicate of this here. I've seen that the backwards compatibility should be solved since version 1.345. And I also looked at the code of the current version. The code looks fine but it does not work for me. I have tested this with a clean install of Hudson 1.351, created a new job with one parameter with a lowercase name, and one build step to show all environment vars. I tested this on Mac OS X 10.6 and on a Vista machine with the same result for both platforms. Only the lowercase variable is set, the uppercase variant is just missing. On windows it seams not to be working as the environments vars seams to be ignoring case. Meaning a set hello=5 and then a set HELLO=10 will result in a variable named hello=10 to be set. But on mac this is not the case so I don't know why it is not working. (I don't have a problem with it not woking under Windows as I use Mac but I just wanted to verify that it's not a mac specific problem).

          Alex Earl added a comment -

          Is this still an issue?

          Alex Earl added a comment - Is this still an issue?

          Daniel Beck added a comment -

          In a different way, yes:
          https://github.com/jenkinsci/jenkins/blob/d8af38b7e3367f3988ceb1d9e15b2fed4efd7971/core/src/main/java/hudson/model/StringParameterValue.java#L55...L57

          It simply defines both.

          Which is probably hilarious on the case of parameter names such as "User" or "Home".

          Related insanity is JENKINS-16255.

          Daniel Beck added a comment - In a different way, yes: https://github.com/jenkinsci/jenkins/blob/d8af38b7e3367f3988ceb1d9e15b2fed4efd7971/core/src/main/java/hudson/model/StringParameterValue.java#L55...L57 It simply defines both. Which is probably hilarious on the case of parameter names such as "User" or "Home". Related insanity is JENKINS-16255 .

            Unassigned Unassigned
            jglick Jesse Glick
            Votes:
            2 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: