|
||||||||
|
This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators. For more information on JIRA, see: http://www.atlassian.com/software/jira |
||||||||
You received this message because you are subscribed to the Google Groups "Jenkins Issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-issues+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
src/main/java/hudson/maven/AbstractMavenBuild.java
src/main/java/hudson/maven/MavenModuleSetBuild.java
http://jenkins-ci.org/commit/maven-plugin/cab3151ea023536ab789f3b74a891f38144fe302
Log:
JENKINS-20884 Mutating 'envVars' returned from getEnvironment() creates a bad precedent.
It is better to just recompute the envVars, which will reflect all the added environments.
See: https://github.com/jenkinsci/maven-plugin/pull/14
Relevant conversation:
(11:31:53 AM) KostyaSha: kohsuke, while windows is eating your IO, could you advice with https://github.com/jenkinsci/maven-plugin/pull/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