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

test-net-GH-5504.js is failing because environment variables are missing #3183

Closed
john-yan opened this issue Oct 5, 2015 · 4 comments
Closed
Labels
test Issues and PRs related to the tests.

Comments

@john-yan
Copy link

john-yan commented Oct 5, 2015

In test https://github.com/nodejs/node/blob/master/test/sequential/test-net-GH-5504.js

   var opt = {
     env: {
       NODE_DEBUG: 'net',
       NODE_COMMON_PORT: process.env.NODE_COMMON_PORT,
     }
   };
   ...
   var s = spawn(node, [__filename, 'server'], opt);

This code loses the original environment variables passing into the program, which causes failure for the spawn function. Specifically, the program could not start because the loader couldn't find the correct version of standard c++ library which is not in the default location. The loader is supposed to find the needed libraries thought LD_LIBRARY_PATH environment variable which is missing.

I suggests to change the above code to this instead:

   var opt = {
     env: process.env
   };
   opt.env.NODE_DEBUG = "net";
   opt.env.NODE_COMMON_PORT = process.env.NODE_COMMON_PORT;
@rvagg
Copy link
Member

rvagg commented Oct 5, 2015

good catch @john-yan, care to open a pull request against master to fix these? You don't need the NODE_COMMON_PORT line if we go with process.env copying though.

@mscdex mscdex added the test Issues and PRs related to the tests. label Oct 5, 2015
@john-yan
Copy link
Author

john-yan commented Oct 5, 2015

@rvagg Thanks. The pull request is opened for the master branch, and I also back port it for v4.x.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 5, 2015

We experienced this on joyent/node. I thought the appropriate commit was cherry picked over. You should just have to extend() env before creating a child process.

Does anyone think it would be useful to include an option to automatically extend the environment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants