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

Make yarn not to add /usr/bin to PATH if already there #6178

Merged
merged 1 commit into from
Aug 3, 2018

Conversation

x-yuri
Copy link
Contributor

@x-yuri x-yuri commented Jul 30, 2018

Summary

Fixes #5935

Test plan

This one is supposedly going to remain untested:

commit 3759562d6a4b114723ee7055e7f872153301657b
Author: Yuri Kanivetsky <[email protected]>
Date:   Mon Jul 30 12:01:22 2018 +0300

    trying to test...
    
    `yarn test` pollutes PATH with another /usr/bin, and even if
    the child instance of yarn doesn't do that, there are at least two
    /usr/bin's in the PATH

diff --git a/__tests__/fixtures/lifecycle-scripts/usr-bin/package.json b/__tests__/fixtures/lifecycle-scripts/usr-bin/package.json
new file mode 100644
index 00000000..d825e8bd
--- /dev/null
+++ b/__tests__/fixtures/lifecycle-scripts/usr-bin/package.json
@@ -0,0 +1,5 @@
+{
+    "scripts": {
+        "cmd1": "echo $PATH | tr : \\\\n"
+    }
+}
diff --git a/__tests__/lifecycle-scripts.js b/__tests__/lifecycle-scripts.js
index 8f6660ef..aa7369e6 100644
--- a/__tests__/lifecycle-scripts.js
+++ b/__tests__/lifecycle-scripts.js
@@ -178,6 +178,12 @@ test('should throw error when the script ends with an exit code', async () => {
   await expect(execCommand('test', 'script-fail')).rejects.toBeDefined();
 });
 
+test('should not add /usr/bin to PATH if already there', async () => {
+  const stdout = await execCommand('cmd1', 'usr-bin');
+  const occurences = stdout.split('\n').filter(l => l == path.dirname(process.execPath));
+  expect(occurences.length).toBe(1);
+});
+
 if (process.platform === 'darwin') {
   test('should throw error when the script ends with an exit signal', async () => {
     await expect(execCommand('test', 'script-segfault')).rejects.toBeDefined();

@x-yuri x-yuri force-pushed the scripts-usr-bin branch from 2fc4fad to a9c8889 Compare July 30, 2018 09:41
@x-yuri
Copy link
Contributor Author

x-yuri commented Jul 30, 2018

@arcanis That isn't my fault the tests are failing, is it?

@arcanis
Copy link
Member

arcanis commented Aug 3, 2018

No indeed. Thanks for your work, I'll merge this - I have another fix in preparation that will slightly change this behavior (a path will be added, but it will only contain node & yarn), but for the time being that sounds like a reasonable approach.

@arcanis arcanis merged commit 003b6b7 into yarnpkg:master Aug 3, 2018
arcanis pushed a commit that referenced this pull request Aug 3, 2018
jasongrout added a commit to jasongrout/yarn that referenced this pull request Aug 3, 2018
Fixes yarnpkg#5935

yarnpkg#6178 is a partial fix for yarnpkg#5935. This adds the following:

1. Don’t prepend the path *after* the local directories.
2. Only prepend the path if the node binary found by searching the environment path is different from our executable path (including if no node is found in the path). This takes care of cases where another node binary precedes our node executable in the path.
This was referenced Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node executable path always prepended to PATH when running a package.json script
2 participants