Merge pull request #13 from synopsys-arc-oss/JENKINS_20560_FIX

oleg-nenashev · oleg-nenashev · commit 6c81ebbc162d · 2013-11-24T23:16:51.000-08:00
[FIXED JENKINS-20560] - Custom-tools should not override global paths in...
diff --git a/src/main/java/com/cloudbees/jenkins/plugins/customtools/CustomToolInstallWrapper.java b/src/main/java/com/cloudbees/jenkins/plugins/customtools/CustomToolInstallWrapper.java
@@ -214,19 +214,27 @@ public Proc launch(ProcStarter starter) throws IOException {
                     vars = new EnvVars();
                 }
                  
-                // HACK: Avoids issue with invalid separators in EnvVars::override in case of different master/slave
-                String overridenPaths = vars.get("PATH");
-                overridenPaths += paths.toListString();
-                vars.override("PATH", overridenPaths);
+                // Inject paths
+                final String injectedPaths = paths.toListString();              
+                if (injectedPaths != null) {              
+                    vars.override("PATH+", injectedPaths);
+                }
+                               
+                // Inject additional variables
                 vars.putAll(homes);
                 vars.putAll(versions);
-                
-                // Inject additional variables
                 for (EnvVariablesInjector injector : additionalVarInjectors) {
                     injector.Inject(vars);
                 }
+                           
+                // Override paths to prevent JENKINS-20560              
+                if (vars.containsKey("PATH")) {
+                    final String overallPaths=vars.get("PATH");
+                    vars.remove("PATH");
+                    vars.put("PATH+", overallPaths);
+                }
                 
-                return getInner().launch(starter.envs(Util.mapToEnv(vars)));
+                return getInner().launch(starter.envs(vars));
             }
                         
             private EnvVars toEnvVars(String[] envs) {
diff --git a/src/main/java/com/synopsys/arc/jenkinsci/plugins/customtools/PathsList.java b/src/main/java/com/synopsys/arc/jenkinsci/plugins/customtools/PathsList.java
@@ -68,32 +68,38 @@ public boolean add(String path) {
     /**
      * Adds PathsList and overrides null variables.
      * @param pathsList PathsList to be added
-     * @return True if 
+     * @return True if the paths list has been modified after the tool installation
      */
+    //TODO: Is it a bug?
     public boolean add(PathsList pathsList) {
         if (pathSeparator == null) {
             pathSeparator = pathsList.pathSeparator;
         }
         if (separator == null) {
             separator = pathsList.separator;
         }
+        // Add homeDir as well (legacy behavior)
         if (pathsList.homeDir != null) {
             this.paths.add(pathsList.homeDir);
         }
                
         return this.paths.addAll(pathsList.paths);
     }
     
+    /**
+     * Gets the list of installed tools
+     * @return A list with valid delimiters or null if paths is empty
+     */
     public String toListString() {
-        StringBuilder builder = new StringBuilder(paths.size()*2);
-        for ( String path : paths) {
-            builder.append(pathSeparator);
-            builder.append(path);
-        } 
+        if (paths.isEmpty()) {
+            return null;
+        }
         
-        // Add homeDir as well (legacy behavior)
-        builder.append(pathSeparator);
-           
+        StringBuilder builder = new StringBuilder();
+        for ( String path : paths) {         
+            builder.append(path);
+            builder.append(pathSeparator);
+        }            
         return builder.toString();
     }
 }
diff --git a/src/test/java/com/cloudbees/jenkins/plugins/customtools/CustomToolInstallWrapperTest.java b/src/test/java/com/cloudbees/jenkins/plugins/customtools/CustomToolInstallWrapperTest.java
@@ -58,6 +58,8 @@ public void testNestedWrapperReverse() throws Exception {
         nestedWrapperTestImpl(wrappers, true);
     }
     
+    
+    
     /**
      * Tests custom tools with wrapper, which calls wrapper without
      * specifying of envs.
@@ -71,6 +73,14 @@ public void testNestedLauncherCalls() throws Exception {
         nestedWrapperTestImpl(wrappers, false);
     }
     
+    //@Bug(20560)
+    public void testEmptyToolsList() throws Exception {
+        List<BuildWrapper> wrappers = new ArrayList<BuildWrapper>(0); 
+        wrappers.add(new CommandCallerInstaller());
+        wrappers.add(new CustomToolInstallWrapper(null, MulticonfigWrapperOptions.DEFAULT, false));
+        nestedWrapperTestImpl(wrappers, false);
+    }
+    
     /**
      * Implements tests for nested wrappers.
      * The test checks that environment variables have been set correctly.
