3 PATH variables are assigned to child process launched by NPM

cli
help-wanted
priority:low
triaged

(Siarhei Kuchuk) #1

What I Wanted to Do

Launch application from npm

What Happened Instead

Application crashes.

Reproduction Steps

A. Install npm 6.1.0
B. Create package.json with the following content

{
  ...
  "scripts": {
    "check": "SET",
  },
}

C. Run command
npm run check

Details

Npm duplicates variable PATH in OS Windows 10 Pro. This breaks executing of applications since they can’t decide which PATH variable is use.

Within npm launch there’re 3 variables

Path
PATH
Path

Platform Info

$ npm --versions
{ quotebuilder: '2.0.0',    
  npm: '6.1.0',             
  ares: '1.14.0',           
  cldr: '33.0',             
  http_parser: '2.8.0',     
  icu: '61.1',              
  modules: '64',            
  napi: '3',                
  nghttp2: '1.29.0',        
  node: '10.1.0',           
  openssl: '1.1.0h',        
  tz: '2018c',              
  unicode: '10.0',          
  uv: '1.20.2',             
  v8: '6.6.346.27-node.6',  
  zlib: '1.2.11' }          
$ node -p process.platform
win32

Please see


(Kat Marchán) #2

Please reformat this bug report following the template. I’ll triage it and leave the post here instead of moving it to #support this time, but it does make things harder for me if the template gets ignored.


(Siarhei Kuchuk) #4

Hello. I updated issue.


(Kat Marchán) #5

thank you so much! And yes, this is totally a thing we’ve been running into recently, due to a PR that was trying to fix something else. I think we’re gonna have to sit down and talk about what the actual solution is instead of hopping around between solutions and hoping one sticks. Thanks for the report :slight_smile:


(Rebecca Turner) #6

The associated PR is: https://github.com/npm/npm-lifecycle/pull/17

@drweb86-work: Question: What actually breaks when having multiple path variables? Like you say “applications” but… does this stop cmd from being able to run commands? Or is it something else? A repro that produces an error would be helpful!


(Siarhei Kuchuk) #7

First, its against OS rules. There can’t be several variables with the same name as its breaks idea behind them.

This breaks .Net applications that launch other applications because .Net validates environment variables that should be assigned to child processes and when it detects 3 PATH variables, it can’t do Process.Start and fails with exception.


(Siarhei Kuchuk) #8

Variable name is not case sensitive. So could you please refix mentioned pull request in a following way

  1. if there’re 2 or more variables with name PATH then just fall with error as its a problem of external environment doing something weirdo by duplication of them. Or take all its values and combine them together using ; as delimiter (which is bad because its masks issue of external environment)
  2. find first variable whose lowercase name is path and do whatever you want with it.

Thank you.


(Rebecca Turner) #9

Does MS have a technet on this? Having that would be helpful.

It is though, and that’s the problem. If it weren’t case sensitive what we’re doing would be actually impossible. (Each would just overwrite the other.)

What I’d prefer that we do is match the behavior of cmd and/or Powershell and follow whatever their priority order is. That is, update the one that they’ll use, but only introduce a new Path env if somehow none is set.

The thing is, that’s what we were already doing:

Whatever change we make the original bug needs to not regress:


(Siarhei Kuchuk) #10

Microsofts utilities works like that
Type
set aaa=13
echo %aaa%
set AaA=22
echo %aaa%

Then you will see that last command shows 22.
Type
set
and see if there’re multiple aaa variable (aaa and AaA): you will see that there’re just one environment variable. Its because in Windows environment variables are case INSENSITIVE.

I think its an issue of MSDN API that it does not validate it (to assign duplicates to processes).
PATH variable always exist in Windows.


#11

There is already a PR waiting @ https://github.com/npm/npm-lifecycle/pull/22 which also resolves https://github.com/npm/npm-lifecycle/issues/20 (a.k.a. Windows: submodules fail to get installed with npm 6.1+)


(Jordan Tucker) #12

This is causing an issue with windows-build-tools.


(Kat Marchán) #13

https://github.com/npm/npm-lifecycle/pull/22 has been merged, and npm-lifecycle@2.1.0 has been released. It will be included in the next npm release, so I consider this fixed now. I’ve published the patch to the npm canary, so you can test that it works for you now using $ npx npmc install ....etc.


(Siarhei Kuchuk) #14

Hello

Thank you for fixing this.


(Kat Marchán) #15