Skip to content
This repository has been archived by the owner on Mar 4, 2022. It is now read-only.

BUG: builder envs does not get same base env ($PATH etc.) as builder run on Node 4+ #63

Closed
exogen opened this issue Dec 22, 2015 · 10 comments · Fixed by #65
Closed

BUG: builder envs does not get same base env ($PATH etc.) as builder run on Node 4+ #63

exogen opened this issue Dec 22, 2015 · 10 comments · Fixed by #65
Labels
bug

Comments

@exogen
Copy link
Contributor

exogen commented Dec 22, 2015

If my test command is something like mocha or wdio that comes from node_modules, builder run test works fine, but builder envs test --envs-path=envs.json spits out:

/bin/sh: mocha: command not found

Builder should start out with whatever $PATH additions come standard with builder run, then merge in stuff from envs on top of that.

(In case it matters: I'm using Builder just for its task running features, without an archetype.)
@ryan-roemer
Copy link
Member

@exogen -- Can you please create a reproduction for this and point me to it?

For reference, requirepack is already using builder envs (Just with straight string, not --envs-paths at https://github.com/FormidableLabs/requirepack/blob/master/.travis.yml#L48-L69 And like your project, requirepack is not using archetypes.

@ryan-roemer
Copy link
Member

@exogen -- Also when running envs can you log out process.env.PATH and paste it in this ticket? Thanks!

@exogen
Copy link
Contributor Author

exogen commented Dec 22, 2015

@ryan-roemer: Sure thing – try cloning this repo: https://github.com/exogen/builder-test

git clone https://github.com/exogen/builder-test
cd builder-test

then:

npm install
npm run test
builder run test
builder envs test '[{ "FOO": 1 }]'

npm run and builder run work, the last one fails.

EDIT: Made it echo $PATH now too. Here's what I get: /usr/gnu/bin:/usr/local/bin:/bin:/usr/bin:.

$PATH from each:

npm run test:

/Users/brianbeck/.nvm/versions/node/v4.2.3/lib/node_modules/npm/bin/node-gyp-bin:/Users/brianbeck/Projects/Tests/builder-test/node_modules/.bin:/Users/brianbeck/.nvm/versions/node/v4.2.3/bin:./node_modules/.bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Users/brianbeck/bin

builder run test:

/Users/brianbeck/Projects/Tests/builder-test/node_modules/.bin:/Users/brianbeck/.nvm/versions/node/v4.2.3/bin:./node_modules/.bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Users/brianbeck/bin

builder envs test:

/usr/gnu/bin:/usr/local/bin:/bin:/usr/bin:.

@ryan-roemer
Copy link
Member

Does not error and successfully runs for me:

$ builder envs test '[{ "FOO": 1 }]'
[builder:config:environment] {"cwd":"/Users/rroemer/scm/fmd/builder-test","dir":"/Users/rroemer/scm/fmd/builder-test/node_modules/builder/lib"}
[builder:config] Unable to load config file: .builderrc
[builder:builder-core:start:2158] Started: envs test
[builder:envs] Starting with queue size: unlimited
[builder:envs] Starting environment {"FOO":1} run for command: mocha
[builder:proc:start] mocha


  0 passing (2ms)

[builder:proc:end:0] mocha
[builder:builder-core:end:2158] Ended normally: envs test

My PATH logged out right before the exec is: /Users/rroemer/scm/fmd/builder-test/node_modules/.bin:/Users/rroemer/.nvm/v0.10.40/bin:/usr/local/heroku/bin:/Users/rroemer/.rvm/gems/ruby-2.0.0-p247/bin:/Users/rroemer/.rvm/gems/ruby-2.0.0-p247@global/bin:/Users/rroemer/.rvm/rubies/ruby-2.0.0-p247/bin:/Users/rroemer/.rvm/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/Developer/Tools:/Users/rroemer/bin:/Users/rroemer/.gem/ruby/1.8/bin:./node_modules/.bin:/bin:/Users/rroemer/.rvm/bin

@exogen
Copy link
Contributor Author

exogen commented Dec 22, 2015

@ryan-roemer: Hmm, maybe a Node/npm version?

$ node -v
v4.2.3
$ npm -v
3.5.2

@exogen
Copy link
Contributor Author

exogen commented Dec 22, 2015

@ryan-roemer Works for me with Node v0.12.7 and same npm (3.5.2), so the difference is with Node 4.

@ryan-roemer
Copy link
Member

Confirmed error:

 $ node -v; npm -v
v4.1.2
3.4.0

$ builder envs test '[{ "FOO": 1 }]'
[builder:config:environment] {"cwd":"/Users/rroemer/scm/fmd/builder-test","dir":"/Users/rroemer/scm/fmd/builder-test/node_modules/builder/lib"}
[builder:config] Unable to load config file: .builderrc
[builder:builder-core:start:2386] Started: envs test
[builder:envs] Starting with queue size: unlimited
[builder:envs] Starting environment {"FOO":1} run for command: mocha
[builder:proc:start] mocha
/bin/sh: mocha: command not found
[builder:proc:end:127] mocha
[builder:proc:error] Code: 127, Command: mocha
[builder:builder-core:end:2386] Ended with error: envs test - Command failed: /bin/sh -c mocha

Logged out PATH in builder before exec contains bin with mocha: " /Users/rroemer/scm/fmd/builder-test/node_modules/.bin:/Users/rroemer/.nvm/v4.1.2/bin:/usr/local/heroku/bin:/Users/rroemer/.rvm/gems/ruby-2.0.0-p247/bin:/Users/rroemer/.rvm/gems/ruby-2.0.0-p247@global/bin:/Users/rroemer/.rvm/rubies/ruby-2.0.0-p247/bin:/Users/rroemer/.rvm/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/Developer/Tools:/Users/rroemer/bin:/Users/rroemer/.gem/ruby/1.8/bin:./node_modules/.bin:/bin:/Users/rroemer/.rvm/bin"

@exogen exogen changed the title BUG: builder envs does not get same base env ($PATH etc.) as builder run BUG: builder envs does not get same base env ($PATH etc.) as builder run on Node4+ Dec 22, 2015
@exogen exogen changed the title BUG: builder envs does not get same base env ($PATH etc.) as builder run on Node4+ BUG: builder envs does not get same base env ($PATH etc.) as builder run on Node 4+ Dec 22, 2015
@ryan-roemer
Copy link
Member

Cuhrazy. _.merge() works differently for:

      var taskShOpts = _.merge({}, shOpts, { env: taskEnv });
      console.log("TODO HERE taskShOpts", taskShOpts.env);

in 0.10:

{ MANPATH: '/Users/rroemer/.nvm/v0.10.40/share/man:',
  NOTEBOOK_URL: 'http://localhost:8000',
  rvm_bin_path: '/Users/rroemer/.rvm/bin',
  VIRTUALENVWRAPPER_PROJECT_FILENAME: '.project',
  // ... TONS OF STUFF ...
  NODE_PATH: '/Users/rroemer/scm/fmd/builder-test/node_modules',
  FOO: 1 }

vs. 0.4:

{ FOO: 1 }

CULPRIT: Different _.merge() behavior.

@ryan-roemer
Copy link
Member

@exogen -- The following code:

      var taskShOpts = _.cloneDeep(shOpts);
      taskShOpts.env = _.extend(taskShOpts.env, taskEnv);

appears to work for https://github.com/FormidableLabs/builder/blob/master/lib/runner.js#L231 if you've got a moment to refine that and PR it up...

@exogen
Copy link
Contributor Author

exogen commented Dec 22, 2015

That is nuts, and I think I figured out why. merge behaves differently based on the result of isPlainObject.

In node 0.12:

> require("lodash").isPlainObject(process.env)
true

In node 4:

> require("lodash").isPlainObject(process.env)
false

So I think your solution works because cloneDeep will turn env into a plain object. I can open a PR shortly.

exogen added a commit that referenced this issue Dec 22, 2015
@exogen
Fix merging of process.env in Node 4+
0c8afc9 
`builder envs` did not work on Node 4+ because `_.merge` was behaving
differently due to `process.env` no longer being a plain object. This
clones `shOpts` (which contains `env`) so that merge works as expected.

I considered `_.toPlainObject(process.env)` in `environment.js` in place
of this fix, to fix the problem at its source. That worked, but I was
fearful of the implications as Environment explicitly says that `env` is
"Environment object to mutate" - and using that fix we'd no longer be
mutating `process.env` but a copy.

Fixes #63

/cc @ryan-roemer
@exogen exogen mentioned this issue Dec 22, 2015
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix merging of process.env in Node 4+ FormidableLabs/builder
2 participants
@exogen @ryan-roemer