cleaned up the MAVEN_OPTS variable expansion story.

Most notably, we weren't expanding environment variables. (CLOUDBEES-1403)

changelog.html

@@ -76,6 +76,8 @@ Upcoming changes</a>
   <li class=bug>
     Maven jobs building plugins were no longer identified as upstream snapshot dependencies.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-10530">issue 10530</a>)
+  <li class=bug>
+    MAVEN_OPTS configuration wasn't expanding environment variables.
 </ul>
 </div><!--=END=-->
 <h3><a name=v1.424>What's new in 1.424</a> (2011/08/01)</h3>

maven-plugin/src/main/java/hudson/maven/AbstractMavenBuild.java

@@ -23,11 +23,17 @@
  */
 package hudson.maven;
 
+import hudson.EnvVars;
 import hudson.model.AbstractBuild;
+import hudson.model.TaskListener;
+import hudson.util.ReflectionUtils;
 
 import java.io.File;
 import java.io.IOException;
+import java.lang.reflect.Method;
 import java.util.Calendar;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 public abstract class AbstractMavenBuild<P extends AbstractMavenProject<P,B>,B extends AbstractMavenBuild<P,B>> extends AbstractBuild<P, B>  {
 
@@ -47,7 +53,51 @@ public abstract class AbstractMavenBuild<P extends AbstractMavenProject<P,B>,B e
     public AbstractMavenBuild(P project, File buildDir) throws IOException {
         super(project, buildDir);
     }
- 
-    
-    
+
+    @Override
+    public EnvVars getEnvironment(TaskListener log) throws IOException, InterruptedException {
+        EnvVars envs = super.getEnvironment(log);
+
+        String opts = getMavenOpts(log,envs);
+
+        if(opts!=null)
+            envs.put("MAVEN_OPTS", opts);
+
+        return envs;
+    }
+
+    /**
+     * Obtains the fully resolved MAVEN_OPTS with all the tokens and variables expanded.
+     *
+     * @see MavenModuleSet#getMavenOpts()
+     * @param envVars
+     *      Caller must pass in the environment variables obtained from {@link #getEnvironment(TaskListener)}
+     *      This method takes this as a parameter as opposed to recomputing it since the caller always have this handy.
+     */
+    public abstract String getMavenOpts(TaskListener listener, EnvVars envVars);
+
+    /**
+     * Expand tokens with token macro.
+     */
+    protected final String expandTokens(TaskListener listener, String str) {
+        if (str==null)      return null;
+        try {
+            Class<?> clazz = Class.forName( "org.jenkinsci.plugins.tokenmacro.TokenMacro" );
+            Method expandMethod =
+                ReflectionUtils.findMethod(clazz, "expand", new Class[]{AbstractBuild.class, TaskListener.class, String.class});
+            return (String) expandMethod.invoke( null, this, listener, str );
+            //opts = TokenMacro.expand(this, listener, opts);
+        }
+        catch(Exception tokenException) {
+            //Token plugin not present. Ignore, this is OK.
+            LOGGER.log(Level.FINE, "Ignore problem in expanding tokens", tokenException);
+        }
+        catch(LinkageError linkageError) {
+            // Token plugin not present. Ignore, this is OK.
+            LOGGER.log(Level.FINE, "Ignore problem in expanding tokens", linkageError);
+        }
+        return str;
+    }
+
+    private static final Logger LOGGER = Logger.getLogger(AbstractMavenBuild.class.getName());
 }

maven-plugin/src/main/java/hudson/maven/MavenBuild.java

@@ -30,7 +30,6 @@ import hudson.maven.reporters.SurefireArchiver;
 import hudson.slaves.WorkspaceList;
 import hudson.slaves.WorkspaceList.Lease;
 import hudson.maven.agent.AbortException;
-import hudson.maven.reporters.SurefireArchiver;
 import hudson.model.AbstractBuild;
 import hudson.model.BuildListener;
 import hudson.model.Computer;
@@ -45,15 +44,12 @@ import hudson.model.TaskListener;
 import hudson.remoting.Channel;
 import hudson.scm.ChangeLogSet;
 import hudson.scm.ChangeLogSet.Entry;
-import hudson.slaves.WorkspaceList;
-import hudson.slaves.WorkspaceList.Lease;
 import hudson.tasks.BuildWrapper;
 import hudson.tasks.Maven.MavenInstallation;
 import hudson.tasks.Publisher;
 import hudson.util.ArgumentListBuilder;
 import hudson.util.DescribableList;
 import hudson.util.IOUtils;
-import hudson.util.ReflectionUtils;
 import org.apache.maven.BuildFailureException;
 import org.apache.maven.artifact.versioning.ComparableVersion;
 import org.apache.maven.execution.MavenSession;
@@ -72,7 +68,6 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.Serializable;
-import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Calendar;
 import java.util.Collections;
@@ -81,17 +76,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.logging.Logger;
 
-import org.apache.maven.BuildFailureException;
-import org.apache.maven.artifact.versioning.ComparableVersion;
-import org.apache.maven.execution.MavenSession;
-import org.apache.maven.execution.ReactorManager;
-import org.apache.maven.lifecycle.LifecycleExecutionException;
-import org.apache.maven.monitor.event.EventDispatcher;
-import org.apache.maven.project.MavenProject;
-import org.kohsuke.stapler.Ancestor;
-import org.kohsuke.stapler.Stapler;
-import org.kohsuke.stapler.StaplerRequest;
-
 /**
  * {@link Run} for {@link MavenModule}.
  * 
@@ -229,9 +213,7 @@ public class MavenBuild extends AbstractMavenBuild<MavenModule,MavenBuild> {
     @Override
     public EnvVars getEnvironment(TaskListener log) throws IOException, InterruptedException {
         EnvVars envs = super.getEnvironment(log);
-        String opts = project.getParent().getMavenOpts();
-        if(opts!=null)
-            envs.put("MAVEN_OPTS", opts);
+
         // We need to add M2_HOME and the mvn binary to the PATH so if Maven
         // needs to run Maven it will pick the correct one.
         // This can happen if maven calls ANT which itself calls Maven
@@ -700,25 +682,14 @@ public class MavenBuild extends AbstractMavenBuild<MavenModule,MavenBuild> {
 
             LOGGER.fine(getFullDisplayName()+" is building with mavenVersion " + mavenVersion + " from file " + mavenInformation.getVersionResourcePath());
             
-            ProcessCache.MavenProcess process = null;
-            
-            boolean maven3orLater = new ComparableVersion (mavenVersion).compareTo( new ComparableVersion ("3.0") ) >= 0;
-           
-            if ( maven3orLater )
-            {
-                process =
-                    MavenBuild.mavenProcessCache.get( launcher.getChannel(), listener,
-                                                      new Maven3ProcessFactory( getParent().getParent(), launcher,
-                                                                                envVars, getMavenOpts(listener), null ) );
-            }
-            else
-            {
-                process =
-                    MavenBuild.mavenProcessCache.get( launcher.getChannel(), listener,
-                                                      new MavenProcessFactory( getParent().getParent(), launcher,
-                                                                               envVars, getMavenOpts(listener), null ) );
-            }
 
+            boolean maven3orLater = new ComparableVersion(mavenVersion).compareTo( new ComparableVersion ("3.0") ) >= 0;
+
+            ProcessCache.MavenProcess process = MavenBuild.mavenProcessCache.get( launcher.getChannel(), listener, maven3orLater
+                ? new Maven3ProcessFactory(
+                        getParent().getParent(), launcher, envVars, getMavenOpts(listener, envVars), null )
+                : new MavenProcessFactory(
+                        getParent().getParent(), launcher, envVars, getMavenOpts(listener, envVars), null ));
 
             ArgumentListBuilder margs = new ArgumentListBuilder("-N","-B");
             if(mms.usesPrivateRepository())
@@ -785,26 +756,8 @@ public class MavenBuild extends AbstractMavenBuild<MavenModule,MavenBuild> {
 
     }
 
-    public String getMavenOpts(TaskListener listener) {
-        MavenModuleSet mms = getProject().getParent();
-        String opts = mms.getMavenOpts();
-        if (opts == null ) return null;
-        try {
-            Class<?> clazz = Class.forName( "org.jenkinsci.plugins.tokenmacro.TokenMacro" );
-            Method expandMethod =
-                ReflectionUtils.findMethod(clazz, "expand", new Class[]{ AbstractBuild.class, TaskListener.class, String.class} );
-            opts = (String) expandMethod.invoke( null, this, listener, opts );
-            //opts = TokenMacro.expand(this, listener, opts);
-        }
-        catch(Exception tokenException) {
-            //Token plugin not present. Ignore, this is OK.
-            //listener.error("Ignore Problem expanding maven opts macros " + tokenException.getMessage());
-        }
-        catch(LinkageError linkageError) {
-            // Token plugin not present. Ignore, this is OK.
-        }
-
-        return opts;
+    public String getMavenOpts(TaskListener listener, EnvVars envVars) {
+        return envVars.expand(expandTokens(listener, getProject().getParent().getMavenOpts()));
     }
 
     private static final int MAX_PROCESS_CACHE = 5;

maven-plugin/src/main/java/hudson/maven/MavenModuleSet.java

@@ -27,6 +27,7 @@ package hudson.maven;
 import static hudson.Util.fixEmpty;
 import static hudson.model.ItemGroupMixIn.loadChildren;
 import hudson.CopyOnWrite;
+import hudson.EnvVars;
 import hudson.Extension;
 import hudson.FilePath;
 import hudson.Indenter;
@@ -38,6 +39,7 @@ import hudson.model.DependencyGraph;
 import hudson.model.Descriptor;
 import hudson.model.Descriptor.FormException;
 import hudson.model.Executor;
+import hudson.model.TaskListener;
 import jenkins.model.Jenkins;
 import hudson.model.Item;
 import hudson.model.ItemGroup;
@@ -738,6 +740,11 @@ public class MavenModuleSet extends AbstractMavenProject<MavenModuleSet,MavenMod
      * to be used to launch Maven process.
      *
      * If mavenOpts is null or empty, we'll return the globally-defined MAVEN_OPTS.
+     *
+     * <p>
+     * This method returns a configured value as-is, which can include variabl references.
+     * At runtime, use {@link AbstractMavenBuild#getMavenOpts(TaskListener, EnvVars)} to obtain
+     * a fully resolved value.
      */
     public String getMavenOpts() {
         if ((mavenOpts!=null) && (mavenOpts.trim().length()>0)) { 

maven-plugin/src/main/java/hudson/maven/MavenModuleSetBuild.java

@@ -35,7 +35,6 @@ import hudson.maven.MavenBuild.ProxyImpl2;
 import hudson.maven.reporters.MavenAggregatedArtifactRecord;
 import hudson.maven.reporters.MavenFingerprinter;
 import hudson.maven.reporters.MavenMailer;
-import hudson.model.AbstractBuild;
 import hudson.model.AbstractProject;
 import hudson.model.Action;
 import hudson.model.Build;
@@ -62,7 +61,6 @@ import hudson.tasks.Maven.MavenInstallation;
 import hudson.util.ArgumentListBuilder;
 import hudson.util.IOUtils;
 import hudson.util.MaskingClassLoader;
-import hudson.util.ReflectionUtils;
 import hudson.util.StreamTaskListener;
 
 import java.io.File;
@@ -70,7 +68,6 @@ import java.io.IOException;
 import java.io.InterruptedIOException;
 import java.io.PrintStream;
 import java.io.Serializable;
-import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -157,10 +154,7 @@ public class MavenModuleSetBuild extends AbstractMavenBuild<MavenModuleSet,Maven
     @Override
     public EnvVars getEnvironment(TaskListener log) throws IOException, InterruptedException {
         EnvVars envs = super.getEnvironment(log);
-        String opts = getMavenOpts(log);
 
-        if(opts!=null)
-            envs.put("MAVEN_OPTS", opts);
         // We need to add M2_HOME and the mvn binary to the PATH so if Maven
         // needs to run Maven it will pick the correct one.
         // This can happen if maven calls ANT which itself calls Maven
@@ -545,24 +539,8 @@ public class MavenModuleSetBuild extends AbstractMavenBuild<MavenModuleSet,Maven
         }
     }
 
-    public String getMavenOpts(TaskListener listener) {
-        String opts = project.getMavenOpts();
-        if (opts == null ) return null;
-        try {
-            Class<?> clazz = Class.forName( "org.jenkinsci.plugins.tokenmacro.TokenMacro" );
-            Method expandMethod =
-                ReflectionUtils.findMethod(clazz, "expand", new Class[]{ AbstractBuild.class, TaskListener.class, String.class} );
-            opts = (String) expandMethod.invoke( null, this, listener, opts );
-        }
-        catch(Exception tokenException) {
-            //Token plugin not present. Ignore, this is OK.
-            //listener.error("Ignore Problem expanding maven opts macros " + tokenException.getMessage());
-        }
-        catch(LinkageError linkageError) {
-            // Token plugin not present. Ignore, this is OK.
-        }
-
-        return opts;
+    public String getMavenOpts(TaskListener listener, EnvVars envVars) {
+        return envVars.expand(expandTokens(listener, project.getMavenOpts()));
     }
 
     /**
@@ -673,7 +651,7 @@ public class MavenModuleSetBuild extends AbstractMavenBuild<MavenModuleSet,Maven
                             LOGGER.fine( "using maven 3 " + mavenVersion );
                             process =
                                 MavenBuild.mavenProcessCache.get( launcher.getChannel(), slistener,
-                                                                  new Maven3ProcessFactory( project, launcher, envVars, getMavenOpts(listener),
+                                                                  new Maven3ProcessFactory( project, launcher, envVars, getMavenOpts(listener, envVars),
                                                                                             pom.getParent() ) );
                         }
                         else
@@ -681,7 +659,7 @@ public class MavenModuleSetBuild extends AbstractMavenBuild<MavenModuleSet,Maven
                             LOGGER.fine( "using maven 2 " + mavenVersion );
                             process =
                                 MavenBuild.mavenProcessCache.get( launcher.getChannel(), slistener,
-                                                                  new MavenProcessFactory( project, launcher, envVars,getMavenOpts(listener),
+                                                                  new MavenProcessFactory( project, launcher, envVars,getMavenOpts(listener, envVars),
                                                                                            pom.getParent() ) );
                         }
                         ArgumentListBuilder margs = new ArgumentListBuilder().add("-B").add("-f", pom.getRemote());

test/src/test/java/hudson/maven/MavenOptsTest.java

@@ -93,5 +93,21 @@ public class MavenOptsTest extends HudsonTestCase {
 	
     }
 
+    /**
+     * Makes sure that environment variables in MAVEN_OPTS are properly expanded.
+     */
+    public void testEnvironmentVariableExpansion() throws Exception {
+        configureDefaultMaven();
+        MavenModuleSet m = createMavenProject();
+        m.setMavenOpts("$FOO");
+        m.setScm(new ExtractResourceSCM(getClass().getResource("maven-opts-echo.zip")));
+        m.setGoals("validate");
+        m.setAssignedLabel(createSlave(new EnvVars("FOO", "-Dhudson.mavenOpt.test=foo -Dhudson.mavenOpt.test2=bar")).getSelfLabel());
+
+        buildAndAssertSuccess(m);
+
+        assertLogContains("[hudson.mavenOpt.test=foo]", m.getLastBuild());
+    }
+
 }