https://github.com/jenkinsci/envinject-plugin/pull/37

Should be considered a blocker if it affects jobs that don't enable any env-inject related config options.

Git bisect shows this was introduced in 414dfe4.

Is this related to the inability to override parameter values, and the weird option to allow it anyway in 'Prepare environment'?

Still relevant on Jenkins 2.7

same here
I confirm that the EnvInjectPluginAction.buildEnvVars() overrides all enviroment variables contribuited to the context (for example by NodeJS plugin in master code in a SimpleBuildWrapper) in the AbstractBuild.getEnvironment() method when cycles on actions.

So the PATH variable contributed by context.env("PATH+NODEJS", path) is override with old one. Now this is blocking because no command are available in the shell resetting any action of the plugin.

any news on this??

Code changed in jenkins
User: Nikolas Falco
Path:
src/main/java/jenkins/plugins/nodejs/NodeJSBuildWrapper.java
src/main/java/jenkins/plugins/nodejs/NodeJSCommandInterpreter.java
src/main/java/jenkins/plugins/nodejs/tools/NodeJSInstallation.java
src/main/java/jenkins/plugins/nodejs/tools/pathresolvers/FixEnvVarEnvironmentContributingAction.java
http://jenkins-ci.org/commit/nodejs-plugin/06c1c1b56865bdf93dad79b5440e822ec680191b
Log:
Workaround for JENKINS-26583

Code changed in jenkins
User: Nikolas Falco
Path:
src/main/java/jenkins/plugins/nodejs/NodeJSBuildWrapper.java
src/main/java/jenkins/plugins/nodejs/NodeJSCommandInterpreter.java
src/main/java/jenkins/plugins/nodejs/tools/pathresolvers/FixEnvVarEnvironmentContributingAction.java
http://jenkins-ci.org/commit/nodejs-plugin/dfd4018b3202898d8a20207df996a5ae111bcd1a
Log:
Workaround for JENKINS-26583

Code changed in jenkins
User: Nikolas Falco
Path:
pom.xml
src/main/java/jenkins/plugins/nodejs/NodeJSCommandInterpreter.java
src/main/java/jenkins/plugins/nodejs/NodeJSPlugin.java
src/main/java/jenkins/plugins/nodejs/configfiles/NPMConfig.java
src/main/java/jenkins/plugins/nodejs/configfiles/NPMRegistry.java
src/main/java/jenkins/plugins/nodejs/configfiles/Npmrc.java
src/main/java/jenkins/plugins/nodejs/tools/CPU.java
src/main/java/jenkins/plugins/nodejs/tools/NodeJSInstallation.java
src/main/resources/lib/nodejs/select.jelly
src/test/java/jenkins/plugins/nodejs/NodeJSBuildWrapperTest.java
src/test/java/jenkins/plugins/nodejs/configfiles/NPMConfigTest.java
src/test/java/jenkins/plugins/nodejs/configfiles/NPMConfigValidationTest.java
http://jenkins-ci.org/commit/nodejs-plugin/bf452be060c0a723991de990ac6833181ef6089d
Log:
Merge branch 'master' into JENKINS-26583

Compare: https://github.com/jenkinsci/nodejs-plugin/compare/2d08527b32e7...bf452be060c0

Sorry, I've rename personal branch for a dedicated workarounf on NodeJS plugin so JIRA should not bother anymore

Are there any ideas for fixing this problem? Workarounds in all other plugins are not good ideas... Where in the code we can start inspecting the problem?

EnvInjectPluginAction#buildEnvVars

I do not know the reason of this action, anyway it save an initial environment and put it back After that all buildwrappeer has contribuire the initial environment. So new variables are not affected by this problem, instead changed variables like PATH are restore to its initial value.

Maybe this behaviour / action should be done/registered only if "Override environment variables" is selected in the job configuration page.

There is also a PR in github provides a lot of tests for an eventual solution

Any news on this issue? When will it be fixed? We are currently blocked on updating NodeJS plugin from 0.2.2 to 1.1.0 because it will no longer work together with EnvInject plugin. So right now we are stuck on NodeJS plugin 0.2.2.

I did not managed to do a real progress on this thing and I do not see I will have time to get back to this any time soon.

Can you describe some good starting points for search and modifications in the source?

afischer, see attached PR.

afischer see my previous post I wrote the class that cause this issue. The git hub PR37 it's a good start point with a lot of UnitTest for this issue.

swf In nodejs github repo there is a branch named workaround-26583 for this issue. You can make a custom build and move into 1.1 snapshot

This bug it's a real pain in the @$$, I have 8/9 bug open on nodejs component due to this.

I have no short-time plans to work on this, unfortunately. No time && the plugin is open for adoption for more then one year

I will check if I can finalize the fix by olivergondza.

I am hit by this issue, too.

 This is what happens with SimpleBuildWrapper and the Environment
Injector Plugin installed:

• CommandInterpreter.perfom() calls AbstractBuild.getEnvironment().
• getEnvironment() assembles all values from its buildEnvironments list – one of these is
  SimpleBuildWrapper.EnvironmentWrapper. which inserts my extended value for $Path.
• After that, EnvironmentContributingActions inject their variables. One of them is an EnvInjectPluginAction, it overwrites the $Path variable we just extended.

Said EnvInjectPluginAction will overwrite every environmental variable (Path, JENKINS_URL, ALLUSERSPROFILE,PROCESSOR_ARCHITECTURE, ...) that a node inherits from System.getenv().

I`m wondering why that EnvInjectPluginAction is added to the build at all: All those inherited system environment variable are already present in the build; there is no need to add these.

And why is it added to jobs that don't enable any env-inject related config options?

AFAICS, EnvInjectActionSetter. is responsible for this behavior.

Could this be fixed by simply deleting EnvInjectActionSetter.java?

Could this be fixed by simply deleting EnvInjectActionSetter.java?

I do not have the confidence this will not have other undesired consequences.

> I do not have the confidence this will not have other undesired consequences. 

It will according to my tests. And it is not something what is going to happen. We could probably add a global configuration setting which allows opting-out from that logic, but it is rather a hack than a fix.

I hae started working on a better patch in April, but I was unable to finish it before the priority switch

Maybe

git blame

can tell why EnvInjectActionSetter.java was introduced?

Instead of patch the affected plugins I had modify the EnvInjectPluginAction of EnvInject plugin with the following patch in the previous 1.9x version

  public void buildEnvVars(@Nonnull AbstractBuild<?, ?> build, @Nonnull EnvVars env)
  {
    if (this.envMap != null) {
      for (String key : this.envMap.keySet()) {
        if (!env.containsKey(key)) {
          // with env.override() should cover the case user wants extends variable like PATH
          env.put(key, (String)this.envMap.get(key));
        }
      }
    }
  }

If I remember right just only 1 test fails

@oleg_nenashev: I am still confident this could be fixed by simply deleting EnvInjectActionSetter.java.

I traced a build and found the following in my case:

1. CommandInterpreter.perfom() calls AbstractBuild.getEnvironment(), which calls hudson.EnvVars.initMaster() #430 which calls System.getenv() on the node and populates the envvar of the build with the ones inherited from the OS.
2. Other plugins modify values (e.g. "PATH+XYZ")...
3. The EnvInjectActionSetter is run, it adds an EnvInjectPluginAction filled with the node-specific EnvVars inherited from the OS (EnvInjectPluginAction.java#65).
4. When the EnvInjectPluginAction from above is run, it puts in exactly the same [1] variables as in step 1, thus wiping out any changes made to the variables values.

 [1] Except some sensitive values, which are masked out. But nobody of a sane mind will pass around sentixve values in a system environment variable.

I recently stumbled over this one too with the NodeJS plugin that tries to inject the node bin folder into the path. After trying and searching for the issue I found this one. Definitely +1 from my side!!!

+1

Surprisingly, fix for https://github.com/jenkinsci/envinject-plugin/pull/56 made it even worse in 1.92. Fixing it in parallel

Thank you for working on this oleg_nenashev .   It is blocking our upgrade to Jenkins 2, and our team lacks developers to work on it, so we really appreciate what you are doing!

15knots

Even "just deleting EnvActionSetter" does not really resolve all cases where the variables get overridden by EnvInject. Yes, it fixes the Global variables, but the same is achieved by https://github.com/jenkinsci/envinject-plugin/pull/124/commits/560c5df32bcdd67570f166312cedbf223b2e6d75 from what I see in the tests.

oleg_nenashev not a big surprise. The PR adds other EnvironmentContributor that overrides the enviroment variable map in a different phase than EnvInjectBuilderContributionAction.

In our organisation to get PATH works we patch the EnvInjectBuilderContributionAction simply with

    @Override
    public void buildEnvVars(@Nonnull AbstractBuild<?, ?> build, @Nonnull EnvVars env) {
        if (envMap != null) {
            for (String key : envMap.keySet()) {
                if (!env.containsKey(key)) {
                    env.put(key, envMap.get(key));
                }
            }
        }
    }

the "if (!env.containsKey(key)) {" do the work, just only 1 test fails

nfalco carltongbrown I have created another pull request, which solves the failing tests. It is something you could try, and it seems mergeable to me. https://github.com/jenkinsci/envinject-plugin/pull/125 

oleg_nenashev your fix worked for me... built and tested from PR 125.

Code changed in jenkins
User: Oliver Gondža
Path:
src/test/java/org/jenkinsci/plugins/envinject/EnvInjectActionTest.java
http://jenkins-ci.org/commit/envinject-plugin/178315eea8c952de7594f4258d35e11e3b2f006a
Log:
JENKINS-26583 Reproduce in unittest

Code changed in jenkins
User: Oliver Gondža
Path:
src/test/java/org/jenkinsci/plugins/envinject/EnvInjectActionTest.java
http://jenkins-ci.org/commit/envinject-plugin/1ab1de604bb3c3b7e291d3c581553cb75827bfbb
Log:
JENKINS-26583 More tests

Code changed in jenkins
User: Oliver Gondža
Path:
src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java
src/test/java/org/jenkinsci/plugins/envinject/BuildCauseRetrieverTest.java
src/test/java/org/jenkinsci/plugins/envinject/EnvInjectActionTest.java
http://jenkins-ci.org/commit/envinject-plugin/2948c711b05b68c6a94c334c26fe4e6fdcecda9f
Log:
[FIXED JENKINS-26583] Do not capture slave variables

I hope to release the fix on this weekend

the fix has been released in 2.1.4

There is still an issue with plugin overriding env variables even if it is disabled (meaning not enabled in the project configuration). And PATH property still fully changed. I suggest to merge PATH property instead of overriding it (split both PATH's to elements and merge into Set)

> I suggest to merge PATH property instead of overriding it (split both PATH's to elements and merge into Set)

I will be happy to review a pull request for that.
It's not that trivial in the code though