|
||||||||
|
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 [email protected].
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