Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

fix: set only one PATH env variable for child proc #25

Closed
wants to merge 1 commit into from

Conversation

zkochan
Copy link
Contributor

@zkochan zkochan commented Sep 5, 2018

#22 caused a regression in pnpm when users had a version of npm
without the revert and a new version of pnpm with revert #22.

Old npm was duplicating the PATH env variables.
New pnpm was not overriding all of them.

Using only one PATH env variable will reduce uncertainty.

#22 caused a regression in pnpm when users had a version of npm
without the revert and a new version of pnpm with rever #22.

Old npm was duplicating the PATH env variables.
New pnpm was not overriding all of them.

Using only one PATH env variable will reduce uncertainty.
@ljharb
Copy link

ljharb commented Sep 6, 2018

It’d hard to understand what this is fixing without a test case.

@zkochan
Copy link
Contributor Author

zkochan commented Sep 6, 2018

I don't know how to write a real-world test case.
The revert #22 did not have any tests.

I can add tests that will confirm that the name of the path env variable is deterministic. That only one env variable is passed to the child process.

Before the changes in this PR, only one env variable was updated (for instance "path") and other two could be not updated (for instance, "PATH" and "Path"). And we don't know which env variable is read by the system. In my case, Windows was reading not the updated env variable.

@isaacs isaacs closed this in 042049d Jul 17, 2019
isaacs pushed a commit that referenced this pull request Jul 17, 2019
without the revert and a new version of pnpm with rever #22.

Old npm was duplicating the PATH env variables.
New pnpm was not overriding all of them.

Using only one PATH env variable will reduce uncertainty.

PR-URL: #25
Close: #25
Reviewed-by: @isaacs
Credit: @zkochan
@isaacs
Copy link
Contributor

isaacs commented Jul 17, 2019

I added a test for this to at least verify that the intended behavior is as described. It's on 3.1.0, but the CDN gremlins are taking a while to show the update. It'll be in the next npm cli release, most likely.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants