[BUG] npm exec unnecessarily sets $PREFIX

[ljharb]

Current Behavior:

env | grep PREFIX # no output
npm exec # only works in an interactive shell
# in the new subshell:
env | grep PREFIX # outputs whatever `dirname ""$(dirname "$(which node)")""` outputted before `npm exec`

Expected Behavior:

env | grep PREFIX # no output
npm exec # only works in an interactive shell
# in the new subshell:
env | grep PREFIX # no output

Setting $PREFIX breaks nvm, and there seems to be zero point in setting it when it wasn't set in the spawning shell.

Steps To Reproduce:

See "current behavior" above.

Environment:

• OS: Mac, n/a
• Node: n/a
• npm: v7.3.0

See nvm-sh/nvm#2379 for context.

[isaacs]

Patch welcome, yes this is unnecessary, I agree.

[ljharb]

Can you point me to the relevant package/file to start looking?

[isaacs]

Sure, sorry for the vague response, had meant to come back to this and forgot to.

It's getting set here, looks like: https://github.com/npm/config/blob/main/lib/set-envs.js#L60

[ljharb]

Hmm - looking at that, defaults doesn't have any prefix to compare to, and cliConf/envConf don't seem to have it either.

It seems important to set it when it was set in the first place, and not to set it when it wasn't, but it's not clear to me how to do that. Any tips?

[isaacs]

env.PREFIX is not an npm-specific config, but it is used by various installers and compilers (most notably, autoconf). I believe it was once upon a time necessary to get binary modules to build properly back in the Scons days.

I could see two possible approaches here.

1. don't set env.PREFIX ever.

   diff --git a/lib/set-envs.js b/lib/set-envs.js
   index 0893337..a3097ec 100644
   --- a/lib/set-envs.js
   +++ b/lib/set-envs.js
   @@ -53,12 +53,6 @@ const setEnvs = (config) => {
        list: [cliConf, envConf],
      } = config
    
   -  const { DESTDIR } = env
   -  if (platform !== 'win32' && DESTDIR && globalPrefix.indexOf(DESTDIR) === 0)
   -    env.PREFIX = globalPrefix.substr(DESTDIR.length)
   -  else
   -    env.PREFIX = globalPrefix
   -
      env.INIT_CWD = env.INIT_CWD || process.cwd()
    
      // if the key is the default value,

2. only set if already set

   diff --git a/lib/set-envs.js b/lib/set-envs.js
   index 0893337..de12a76 100644
   --- a/lib/set-envs.js
   +++ b/lib/set-envs.js
   @@ -53,11 +53,13 @@ const setEnvs = (config) => {
        list: [cliConf, envConf],
      } = config
    
   -  const { DESTDIR } = env
   -  if (platform !== 'win32' && DESTDIR && globalPrefix.indexOf(DESTDIR) === 0)
   -    env.PREFIX = globalPrefix.substr(DESTDIR.length)
   -  else
   -    env.PREFIX = globalPrefix
   +  const { DESTDIR, PREFIX } = env
   +  if (PREFIX) {
   +    if (platform !== 'win32' && DESTDIR && globalPrefix.indexOf(DESTDIR) === 0)
   +      env.PREFIX = globalPrefix.substr(DESTDIR.length)
   +    else
   +      env.PREFIX = globalPrefix
   +  }
    
      env.INIT_CWD = env.INIT_CWD || process.cwd()
    

With either approach, we should float the patch on the npm cli and make sure it doesn't break any tests there. Any tests that are broken in npm/config are probably just asserting that env.PREFIX is getting set, and can be removed or updated to reflect the new intended behavior.

I can't really justify why we set env.PREFIX. It was just carried forward by default from npm v6, and since we now use @npmcli/run-script as the One True Way to execute things in an npm-style environment (ie, both npm run-script and npm exec/npx), it's showing up there as well.

[ljharb]

Indeed; termux uses it which causes unavoidable issues for nvm :-/

It seems less breaking to me to go with option 2. I'll make a PR soon that does that, and then we can try it on the CLI.

[isaacs]

There is really no need to set PREFIX, even if already set. I think maybe we should just remove it? It looks like it was only added because Scons needed it to build node binary modules, back in the dark ages before node-gyp.

[ljharb]

Awesome; i hadn’t had a chance to get to it yet.