We might be handling environment variables for a slave that can have different...

We might be handling environment variables for a slave that can have different path separator than the master, so this is an attempt to get it right. It's still more error prone that I'd like, but this should mostly work for our usage pattern.

The real problem is that AbstractBuild.getEnvVars() is designed to return overrides, not the whole thing. This was OK until recently, when we started using environment variables to eagerly perform variable expansions inside Builder/Publisher, which requires them to see the entire variables.
 


git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@16226 71c3de6d-444a-0410-be80-ed276b4c234a

core/src/main/java/hudson/EnvVars.java

@@ -47,6 +47,16 @@ import java.util.Arrays;
  * @author Kohsuke Kawaguchi
  */
 public class EnvVars extends TreeMap<String,String> {
+    /**
+     * If this {@link EnvVars} object represents the whole environment variable set,
+     * not just a partial list used for overriding later, then we need to know
+     * the platform for which this env vars are targeted for, or else we won't konw
+     * how to merge variables properly.
+     *
+     * <p>
+     * So this property remembers that information.
+     */
+    private Platform platform;
 
     public EnvVars() {
         super(CaseInsensitiveComparator.INSTANCE);
@@ -55,6 +65,18 @@ public class EnvVars extends TreeMap<String,String> {
     public EnvVars(Map<String,String> m) {
         this();
         putAll(m);
+
+        // because of the backward compatibility, some parts of Hudson passes
+        // EnvVars as Map<String,String> so downcasting is safer.
+        if (m instanceof EnvVars) {
+            EnvVars lhs = (EnvVars) m;
+            this.platform = lhs.platform;
+        }
+    }
+
+    public EnvVars(EnvVars m) {
+        // this constructor is so that in future we can get rid of the downcasting.
+        this((Map)m);
     }
 
     public EnvVars(String... keyValuePairs) {
@@ -82,7 +104,13 @@ public class EnvVars extends TreeMap<String,String> {
             String realKey = key.substring(0,idx);
             String v = get(realKey);
             if(v==null) v=value;
-            else        v=value+File.pathSeparatorChar+v;
+            else {
+                // we might be handling environment variables for a slave that can have different path separator
+                // than the master, so the following is an attempt to get it right.
+                // it's still more error prone that I'd like.
+                char ch = platform==null ? File.pathSeparatorChar : platform.pathSeparator;
+                v=value+ch+v;
+            }
             put(realKey,v);
             return;
         }
@@ -133,6 +161,8 @@ public class EnvVars extends TreeMap<String,String> {
      *
      * @param channel
      *      Can be null, in which case the map indicating "N/A" will be returned.
+     * @return
+     *      A fresh copy that can be owned and modified by the caller.
      */
     public static EnvVars getRemote(VirtualChannel channel) throws IOException, InterruptedException {
         if(channel==null)
@@ -159,6 +189,11 @@ public class EnvVars extends TreeMap<String,String> {
      * If you access this field from slaves, then this is the environment
      * variable of the slave agent.
      */
-    public static final Map<String,String> masterEnvVars = new EnvVars(System.getenv());
+    public static final Map<String,String> masterEnvVars = initMaster();
 
+    private static EnvVars initMaster() {
+        EnvVars vars = new EnvVars(System.getenv());
+        vars.platform = Platform.current();
+        return vars;
+    }
 }

core/src/main/java/hudson/Platform.java

+package hudson;
+
+import java.io.File;
+
+/**
+ * Strategy object that absorbs the platform differences.
+ *
+ * <p>
+ * Do not switch/case on this enum, or do a comparison, as we may add new constants.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+public enum Platform {
+    WINDOWS(';'),UNIX(':');
+
+    /**
+     * The character that separates paths in environment variables like PATH and CLASSPATH. 
+     * On Windows ';' and on Unix ':'.
+     *
+     * @see File#pathSeparator
+     */
+    public final char pathSeparator;
+
+    private Platform(char pathSeparator) {
+        this.pathSeparator = pathSeparator;
+    }
+
+    public static Platform current() {
+        if(File.pathSeparatorChar==':') return UNIX;
+        return WINDOWS;
+    }
+}

core/src/main/java/hudson/Util.java

@@ -384,6 +384,9 @@ public class Util {
         return tokenize(s," \t\n\r\f");
     }
 
+    /**
+     * Converts the map format of the environment variables to the K=V format in the array.
+     */
     public static String[] mapToEnv(Map<String,String> m) {
         String[] r = new String[m.size()];
         int idx=0;

core/src/main/java/hudson/maven/MavenBuild.java

@@ -455,7 +455,7 @@ public class MavenBuild extends AbstractBuild<MavenModule,MavenBuild> {
             if(debug)
                 listener.getLogger().println("Reporters="+reporters);
 
-            EnvVars envVars = EnvVars.getRemote(launcher.getChannel()).overrideAll(getEnvVars());
+            EnvVars envVars = getEnvironment();
             
             ProcessCache.MavenProcess process = mavenProcessCache.get(launcher.getChannel(), listener,
                 new MavenProcessFactory(getParent().getParent(),launcher,envVars,null));

core/src/main/java/hudson/maven/MavenModuleSetBuild.java

@@ -311,7 +311,7 @@ public final class MavenModuleSetBuild extends AbstractBuild<MavenModuleSet,Mave
         protected Result doRun(final BuildListener listener) throws Exception {
             PrintStream logger = listener.getLogger();
             try {
-                EnvVars envVars = EnvVars.getRemote(launcher.getChannel());
+                EnvVars envVars = getEnvironment();
                 parsePoms(listener, logger, envVars);
 
                 if(!project.isAggregatorStyleBuild()) {
@@ -344,8 +344,6 @@ public final class MavenModuleSetBuild extends AbstractBuild<MavenModuleSet,Mave
                         for (MavenModule m : project.sortedActiveModules)
                             proxies.put(m.getModuleName(),m.newBuild().new ProxyImpl2(MavenModuleSetBuild.this,slistener));
 
-                        envVars.overrideAll(getEnvVars());
-
                         // run the complete build here
 
                         // figure out the root POM location.

core/src/main/java/hudson/model/AbstractBuild.java

@@ -428,8 +428,8 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
     }
 
     @Override
-    public Map<String,String> getEnvVars() {
-        Map<String,String> env = super.getEnvVars();
+    public EnvVars getEnvironment() throws IOException, InterruptedException {
+        EnvVars env = super.getEnvironment();
         env.put("WORKSPACE", getProject().getWorkspace().getRemote());
         // servlet container may have set CLASSPATH in its launch script,
         // so don't let that inherit to the new child process.

core/src/main/java/hudson/model/Computer.java

@@ -527,11 +527,19 @@ public abstract class Computer extends AbstractModelObject implements AccessCont
         return RemotingDiagnostics.getSystemProperties(getChannel());
     }
 
+    /**
+     * @deprecated as of 1.292
+     *      Use {@link #getEnvironment()} instead.
+     */
+    public Map<String,String> getEnvVars() throws IOException, InterruptedException {
+        return getEnvironment();
+    }
+
     /**
      * Gets the environment variables of the JVM on this computer.
      * If this is the master, it returns the system property of the master computer.
      */
-    public Map<String,String> getEnvVars() throws IOException, InterruptedException {
+    public EnvVars getEnvironment() throws IOException, InterruptedException {
         return EnvVars.getRemote(getChannel());
     }
 

core/src/main/java/hudson/model/ExternalRun.java

@@ -61,7 +61,7 @@ public class ExternalRun extends Run<ExternalJob,ExternalRun> {
     public void run(final String[] cmd) {
         run(new Runner() {
             public Result run(BuildListener listener) throws Exception {
-                Proc proc = new Proc.LocalProc(cmd,getEnvVars(),System.in,new DualOutputStream(System.out,listener.getLogger()));
+                Proc proc = new Proc.LocalProc(cmd,getEnvironment(),System.in,new DualOutputStream(System.out,listener.getLogger()));
                 return proc.join()==0?Result.SUCCESS:Result.FAILURE;
             }
 

core/src/main/java/hudson/model/Run.java

@@ -53,7 +53,6 @@ import hudson.util.XStream2;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.FileReader;
 import java.io.IOException;
@@ -1230,7 +1229,22 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
     }
 
     /**
-     * Returns the map that contains environmental variable overrides for this build.
+     * @deprecated as of 1.292
+     *      Use {@link #getEnvironment()} instead.
+     */
+    public Map<String,String> getEnvVars() {
+        try {
+            return getEnvironment();
+        } catch (IOException e) {
+            return new EnvVars();
+        } catch (InterruptedException e) {
+            return new EnvVars();
+        }
+    }
+
+    /**
+     * Returns the map that contains environmental variables to be used for launching
+     * processes for this build.
      *
      * <p>
      * {@link BuildStep}s that invoke external processes should use this.
@@ -1238,14 +1252,11 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
      * to take effect.
      *
      * <p>
-     * On Windows systems, environment variables are case-preserving but
-     * comparison/query is case insensitive (IOW, you can set 'Path' to something
-     * and you get the same value by doing '%PATH%'.)  So to implement this semantics
-     * the map returned from here is a {@link TreeMap} with a special comparator.
-     *
+     * Unlike earlier {@link #getEnvVars()}, this map contains the whole environment,
+     * not just the overrides, so one can introspect values to change its behavior.
      */
-    public Map<String,String> getEnvVars() {
-        EnvVars env = getCharacteristicEnvVars();
+    public EnvVars getEnvironment() throws IOException, InterruptedException {
+        EnvVars env = Computer.currentComputer().getEnvironment().overrideAll(getCharacteristicEnvVars());
         String rootUrl = Hudson.getInstance().getRootUrl();
         if(rootUrl!=null)
             env.put("HUDSON_URL", rootUrl);

core/src/main/java/hudson/tasks/Ant.java

@@ -136,9 +136,8 @@ public class Ant extends Builder {
         
         ArgumentListBuilder args = new ArgumentListBuilder();
 
-        Map<String,String> slaveEnv = EnvVars.getRemote(launcher.getChannel());
-        EnvVars env = new EnvVars(slaveEnv);
-        env.overrideAll(build.getEnvVars());
+        EnvVars env = EnvVars.getRemote(launcher.getChannel());
+        env.overrideAll(build.getEnvironment());
         
         AntInstallation ai = getAnt();
         if (ai != null) {

core/src/main/java/hudson/tasks/CommandInterpreter.java

@@ -26,6 +26,7 @@ package hudson.tasks;
 import hudson.FilePath;
 import hudson.Launcher;
 import hudson.Util;
+import hudson.EnvVars;
 import hudson.model.AbstractBuild;
 import hudson.model.AbstractProject;
 import hudson.model.BuildListener;
@@ -74,7 +75,7 @@ public abstract class CommandInterpreter extends Builder {
 
             int r;
             try {
-                Map<String,String> envVars = build.getEnvVars();
+                EnvVars envVars = build.getEnvironment();
                 // on Windows environment variables are converted to all upper case,
                 // but no such conversions are done on Unix, so to make this cross-platform,
                 // convert variables to all upper cases.

core/src/main/java/hudson/tasks/Maven.java

@@ -184,9 +184,7 @@ public class Maven extends Builder {
 
         VariableResolver<String> vr = build.getBuildVariableResolver();
 
-        Map<String,String> slaveEnv = EnvVars.getRemote(launcher.getChannel());
-        EnvVars env = new EnvVars(slaveEnv);
-        env.overrideAll(build.getEnvVars());
+        EnvVars env = build.getEnvironment();
 
         String targets = Util.replaceMacro(this.targets,vr);
         targets = env.expand(targets);

test/src/main/java/org/jvnet/hudson/test/CaptureEnvironmentBuilder.java

@@ -25,6 +25,7 @@ package org.jvnet.hudson.test;
 
 import hudson.Launcher;
 import hudson.Extension;
+import hudson.EnvVars;
 import hudson.model.AbstractBuild;
 import hudson.model.BuildListener;
 import hudson.model.Descriptor;
@@ -42,14 +43,14 @@ import java.util.Map;
  */
 public class CaptureEnvironmentBuilder extends Builder {
 	
-    private Map<String, String> envVars;
+    private EnvVars envVars;
 
-	public Map<String, String> getEnvVars() {
+	public EnvVars getEnvVars() {
 		return envVars;
 	}
 
 	public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
-    	envVars = build.getEnvVars();
+    	envVars = build.getEnvironment();
         return true;
     }