• 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()

          Jesse Glick created issue -

          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.
          SCM/JIRA link daemon made changes -
          Resolution New: Fixed [ 1 ]
          Status Original: Open [ 1 ] New: Resolved [ 5 ]

          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.
          mcrooney made changes -
          Resolution Original: Fixed [ 1 ]
          Status Original: Resolved [ 5 ] New: Reopened [ 4 ]
          vkodocha made changes -
          Link New: This issue is blocking JENKINS-5843 [ JENKINS-5843 ]

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

              Created:
              Updated: