[JENKINS-20884] Mutating 'envVars' returned from getEnvironment() cre…

…ates a bad precedent. It is better to just recompute the envVars, which will reflect all the added environments. See: #14 Relevant conversation: (11:31:53 AM) KostyaSha: kohsuke, while windows is eating your IO, could you advice with #14 ? (11:34:27 AM) kohsuke: KostyaSha: the bug looks legit, but I'm not sure about the fix. Basically I agree with Jesse's comment (11:35:35 AM) KostyaSha: kohsuke, this broken by design i think, it should be a good idea to share envvars for builders. This should also reduce .getEnvironments() calls (11:36:33 AM) KostyaSha: kohsuke, is there any place where they maybe safely shared? (11:37:03 AM) kohsuke: I'm afraid I don't understand the notion of "sharing envvars" (11:37:35 AM) kohsuke: It gets computed from various things, and I thought EnvironmentContributingAction is a part of it (11:38:44 AM) KostyaSha: kohsuke, yes, and they are stored in envvars variable, then job calls prebuilders that modifies EnvironmentContributingAction and then job calls maven build with not updated envvars content (11:39:05 AM) kohsuke: then it should just call EnvVars envVars = getEnvironment(listener); again (11:39:50 AM) KostyaSha: kohsuke, i not sure that there is no any specific changes with envvars after first getEnvironment(listener) call and before prebuilders (11:40:30 AM) KostyaSha: i compared on my local instance and this should work, but i not sure... potentially some changes to envvars maybe lost... (11:40:31 AM) kohsuke: Basically, one should never modify what Run.getEnvironment() returned (11:40:51 AM) kohsuke: If the map returned is missing some desirable entries, then it should be fixing by having the getEnvironment() implementation add them (11:41:11 AM) kohsuke: It looks to me that this change violates that idea (11:42:02 AM) KostyaSha: i like idea of refreshing with simple call to getEnvironment(listener) (11:42:03 AM) kohsuke: if environment-contributing subset of rootBuild.actions should be a part of the env vars, according to the above principle it should be done in the getEnvironment() method (11:42:52 AM) kohsuke: (Also, let's not print out random stuff into "logger.println" that most users would not care (11:43:02 AM) kohsuke: Those should be j.u.logging statements) (11:43:28 AM) KostyaSha: kohsuke, yeah this logger not needed of course (11:44:55 AM) KostyaSha: kohsuke, https://github.com/zygm0nt/maven-plugin/blob/master/src/main/java/hudson/maven/MavenModuleSetBuild.java#L648 what this part do? (11:45:44 AM) kohsuke: That looks wrong to me, too, for the same reason (11:45:49 AM) kohsuke: All right, you convinced me to open this in IDE (11:46:03 AM) kohsuke: Screw the preparation for the meeting (11:47:29 AM) kohsuke: Wow, that line has jglick fixing HUDSON-3502 (11:47:31 AM) jenkins-admin: JENKINS-3502:Xvnc does not set the DISPLAY environment (Closed) https://issues.jenkins-ci.org/browse/JENKINS-3502 (11:48:06 AM) KostyaSha: 0_o (11:48:17 AM) kohsuke: From 2009 (11:49:34 AM) KostyaSha: kohsuke, and the next block is also added for resolving variables (11:49:42 AM) kohsuke: Yep (11:50:28 AM) KostyaSha: so after every step we need recalculate changes... so easy to allow do direct modifications of envvars i think
jenkinsci · Oct 9, 2014 · cab3151 · cab3151
1 parent eceb028
commit cab3151
Show file tree

Hide file tree

Showing 2 changed files with 9 additions and 8 deletions.
diff --git a/src/main/java/hudson/maven/AbstractMavenBuild.java b/src/main/java/hudson/maven/AbstractMavenBuild.java
@@ -24,7 +24,9 @@
 package hudson.maven;
 
 import hudson.EnvVars;
+import hudson.Util;
 import hudson.model.AbstractBuild;
+import hudson.model.EnvironmentContributingAction;
 import hudson.model.TaskListener;
 import hudson.util.ReflectionUtils;
 
@@ -53,6 +55,9 @@ public AbstractMavenBuild(P project, File buildDir) throws IOException {
     public EnvVars getEnvironment(TaskListener log) throws IOException, InterruptedException {
         EnvVars envs = super.getEnvironment(log);
 
+        for (EnvironmentContributingAction a : Util.filter(getProject().getActions(), EnvironmentContributingAction.class))
+            a.buildEnvVars(this,envs);
+
         String opts = getMavenOpts(log,envs);
 
         if(opts!=null)

diff --git a/src/main/java/hudson/maven/MavenModuleSetBuild.java b/src/main/java/hudson/maven/MavenModuleSetBuild.java
@@ -656,15 +656,8 @@ protected Result doRun(final BuildListener listener) throws Exception {
                                 return r;
                             }
                             buildEnvironments.add(e);
-                            e.buildEnvVars(envVars); // #3502: too late for getEnvironment to do this
-                            Collection<? extends Action> actionsFromWrapper = w.getProjectActions(project);
-                            for (Action action : actionsFromWrapper) {
-                                if(action instanceof EnvironmentContributingAction){ // #17555
-                                    ((EnvironmentContributingAction) action).buildEnvVars(MavenModuleSetBuild.this, envVars);
-                                }
-                            }                            
                         }
-                        
+
                     	// run pre build steps
                     	if(!preBuild(listener,project.getPrebuilders())
                         || !preBuild(listener,project.getPostbuilders())
@@ -678,6 +671,9 @@ protected Result doRun(final BuildListener listener) throws Exception {
                             return r;
             			}
 
+                        // reflect changes made to environment variables via BuildWrappers and other EnvironmentContributingActions
+                        envVars = getEnvironment(listener);
+
                         parsePoms(listener, logger, envVars, mvn, mavenVersion, mavenBuildInformation); // #5428 : do pre-build *before* parsing pom
                         SplittableBuildListener slistener = new SplittableBuildListener(listener);
                         proxies = new HashMap<ModuleName, ProxyImpl2>();