Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

npm-test-install #6736

Closed
mhdawson opened this issue May 13, 2016 · 19 comments · Fixed by #6797
Closed

npm-test-install #6736

mhdawson opened this issue May 13, 2016 · 19 comments · Fixed by #6797
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. test Issues and PRs related to the tests.

Comments

@mhdawson
Copy link
Member

  • Version: master
  • Platform: x64
  • Subsystem: npm

Sorry if this has been discussed elsewhere but I've not found an open issue that sounds related. Over the last couple of days when I run the check-in tests (make test) I get failures with:

=== release test-npm-install ===
Path: parallel/test-npm-install
assert.js:90
  throw new assert.AssertionError({
  ^
AssertionError: npm install should run without an error
    at ChildProcess.handleExit (/home/mhdawson/pulls/land/stress/node/test/parallel/test-npm-install.js:43:10)
    at ChildProcess. (/home/mhdawson/pulls/land/stress/node/test/common.js:394:15)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:204:12)
Command: out/Release/node /home/mhdawson/pulls/land/stress/node/test/parallel/test-npm-install.js

make: *** [test] Error 1

Anybody else seeing this ?

@mhdawson mhdawson added the test Issues and PRs related to the tests. label May 13, 2016
@mhdawson
Copy link
Member Author

I'll also note that I know it does pass in the CI on the same checkout so its likely something in the environment setup

@mscdex mscdex added the npm Issues and PRs related to the npm client dependency or the npm registry. label May 13, 2016
@Fishrock123
Copy link
Contributor

cc @thealphanerd

@Fishrock123
Copy link
Contributor

@mhdawson Does npm install in a test package work?

Maybe you can try running make test-npm? (You'll need to leave it for about 10 minutes to do so..)

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 13, 2016

(Also it looks like that test isn't piping stderr from npm or something, we probably should be..)

@Fishrock123
Copy link
Contributor

@mhdawson actually, can you try that test with this patch?

diff --git a/test/parallel/test-npm-install.js b/test/parallel/test-npm-install.js
index b6ef67b..91a3247 100644
--- a/test/parallel/test-npm-install.js
+++ b/test/parallel/test-npm-install.js
@@ -38,6 +38,8 @@ const proc = spawn(process.execPath, args, {
     PATH: path.dirname(process.execPath)
   }
 });
+// To double-check this test, uncomment the line below.
+proc.stderr.pipe(process.stderr);

 function handleExit(code, signalCode) {
   assert.equal(code, 0, 'npm install should run without an error');

@MylesBorins
Copy link
Contributor

@mhdawson which version of node are you using? We found this week that this test was using the system version of node, not the freshly compiled version... this has been fixed upstream though, so if you are running off an up to date master when you saw this error it is not the problem.

I'd be interested to see the output of the error using @Fishrock123's patch above

@mhdawson
Copy link
Member Author

mhdawson commented May 16, 2016

Sorry for slow response, catching up after being a bit under the weather.

I was using master. All I did was:

<PRE<
git clone https://github.com/nodejs/node.git
./configure
cd node
make -j8 test

on ubuntu 14.04 x64

I'll try with @Fishrock123 patch now

@mhdawson
Copy link
Member Author

This was the output

make[2]: Leaving directory `/home/mhdawson/pulls/land/test/node/out'
ln -fs out/Release/node node
Running main() from gtest_main.cc
[==========] Running 3 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 3 tests from UtilTest
[ RUN      ] UtilTest.ListHead
[       OK ] UtilTest.ListHead (0 ms)
[ RUN      ] UtilTest.StringEqualNoCase
[       OK ] UtilTest.StringEqualNoCase (0 ms)
[ RUN      ] UtilTest.ToLower
[       OK ] UtilTest.ToLower (0 ms)
[----------] 3 tests from UtilTest (0 ms total)
[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (1 ms total)
[  PASSED  ] 3 tests.
make[1]: Leaving directory `/home/mhdawson/pulls/land/test/node'
/usr/bin/python tools/test.py --mode=release -J \
                addon doctool known_issues message parallel sequential
=== release test-npm-install ===
Path: parallel/test-npm-install
npm ERR! addLocal Could not install /home/mhdawson/pulls/land/test/node/test/fixtures/packages/main
npm ERR! Linux 3.13.0-85-generic
npm ERR! argv "/home/mhdawson/pulls/land/test/node/out/Release/node" "/home/mhdawson/pulls/land/test/node/deps/npm/bin/npm-cli.js" "install"
npm ERR! node v7.0.0-pre
npm ERR! npm  v3.8.9
npm ERR! path /team
npm ERR! code EACCES
npm ERR! errno -13
npm ERR! syscall mkdir
npm ERR! Error: EACCES: permission denied, mkdir '/team'
npm ERR!     at Error (native)
npm ERR!  { Error: EACCES: permission denied, mkdir '/team'
npm ERR!     at Error (native) errno: -13, code: 'EACCES', syscall: 'mkdir', path: '/team' }
npm ERR!
npm ERR! Please try running this command again as root/Administrator.
npm ERR! Please include the following file with any support request:
npm ERR!     /home/mhdawson/pulls/land/test/node/test/tmp.2/npm-debug.log
assert.js:90
  throw new assert.AssertionError({
  ^
AssertionError: npm install should run without an error
    at ChildProcess.handleExit (/home/mhdawson/pulls/land/test/node/test/parallel/test-npm-install.js:45:10)
    at ChildProcess. (/home/mhdawson/pulls/land/test/node/test/common.js:394:15)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:204:12)
Command: out/Release/node /home/mhdawson/pulls/land/test/node/test/parallel/test-npm-install.js
make: *** [test] Error 1

@mhdawson
Copy link
Member Author

mhdawson commented May 16, 2016

Will look into why it would select "team" as the place to do the install tomorrow. HOME is

/home/mhdawson

@MylesBorins
Copy link
Contributor

@mhdawson this is using the latest master? Very odd it isn't setting the cwd for the test, I was under the impression that was being done

@mhdawson
Copy link
Member Author

Yes, I just did a clean checkout/compile/test

@MylesBorins
Copy link
Contributor

@mhdawson the project is supposed to be setting cwd to common.tmpdir for the chilprocess running npm.

very odd and unexpected to see it state your path is /team

@mhdawson
Copy link
Member Author

This seems to work:

mhdawson@duv-aurora:~/pulls/land/test/node/test/fixtures/packages/main$ /home/mhdawson/pulls/land/test/node/out/Release/node /home/mhdawson/pulls/land/test/node/deps/npm/bin/npm-cli.js install
normalizeTree             â
normalizeTree â install   â â¢âââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
npm WARN package-name@1.2.3 No description
npm WARN package-name@1.2.3 No repository field.
npm WARN package-name@1.2.3 No license field.
npm WARN You are using a pre-release version of node and things may not work as expected
mhdawson@duv-aurora:~/pulls/land/test/node/test/fixtures/packages/main$

if I cd first into ~/pulls/land/test/node/test/fixtures/packages/main

The problem may be that HOME is not being properly propagated to childprocess. My default HOME is invalid because it is set of a different set of systems than we use for Node.js. So once logged in I manually "export HOME=/home/mhdawson", I'd only expect /team to be used if for some reason it does not get propagated to the child process

@MylesBorins
Copy link
Contributor

I wonder if CWD is relying on home at all... this seems like it might be a deeper bug,

@mhdawson
Copy link
Member Author

mhdawson commented May 17, 2016

So adding this (part in bold) to the test seems to resolve the issue so seems related to the HOME that was set not being propagated into the child process:

const proc = spawn(process.execPath, args, {
  cwd: common.tmpDir,
  env: {
    HOME: '/home/mhdawson',
    PATH: path.dirname(process.execPath)
  }
});

@mhdawson
Copy link
Member Author

or more generally:

const proc = spawn(process.execPath, args, {
  cwd: common.tmpDir,
  env: {
    HOME: process.env.HOME,
    PATH: path.dirname(process.execPath)
  }
});

@mhdawson
Copy link
Member Author

mhdawson commented May 17, 2016

From the API doc:

Use env to specify environment variables that will be visible to the new process, the default is process.env.

So possibly having specified env, without including HOME that masked out the HOME value that had been set in the environment ?

@MylesBorins
Copy link
Contributor

OHHHH... this is likely due to the change I did recently over writing process.env

@mhdawson I have a fix incoming

@mhdawson
Copy link
Member Author

k thanks

MylesBorins pushed a commit to MylesBorins/node that referenced this issue May 18, 2016

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Currently we are overwriting the entire env object of the child-process
spawned in `npm-test-install`. This commit alternatively clones the
`process.env` object and modifies it with the neccessary changes before
passing it the the spawned process.

Fixes: nodejs#6736

PR-URL: nodejs#6797
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this issue May 23, 2016
Currently we are overwriting the entire env object of the child-process
spawned in `npm-test-install`. This commit alternatively clones the
`process.env` object and modifies it with the neccessary changes before
passing it the the spawned process.

Fixes: #6736

PR-URL: #6797
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Jun 2, 2016
Currently we are overwriting the entire env object of the child-process
spawned in `npm-test-install`. This commit alternatively clones the
`process.env` object and modifies it with the neccessary changes before
passing it the the spawned process.

Fixes: #6736

PR-URL: #6797
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 2, 2016
Currently we are overwriting the entire env object of the child-process
spawned in `npm-test-install`. This commit alternatively clones the
`process.env` object and modifies it with the neccessary changes before
passing it the the spawned process.

Fixes: #6736

PR-URL: #6797
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 24, 2016
Currently we are overwriting the entire env object of the child-process
spawned in `npm-test-install`. This commit alternatively clones the
`process.env` object and modifies it with the neccessary changes before
passing it the the spawned process.

Fixes: #6736

PR-URL: #6797
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 24, 2016
Currently we are overwriting the entire env object of the child-process
spawned in `npm-test-install`. This commit alternatively clones the
`process.env` object and modifies it with the neccessary changes before
passing it the the spawned process.

Fixes: #6736

PR-URL: #6797
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants