Removed superfluous and erroneous node-gyp command replacement #2645

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

einaros commented Jul 23, 2012

lib/utils/lifecycle.js has a codeblock which is supposed to replace a package.json script call such as node-gyp configure with path/to/node full/path/of/node-gyp configure.

The original code:

env.npm_lifecycle_script = pkg.scripts[stage]
// if the command is "node-gyp <args>", then call ours instead.
try {
    var ourGyp = require.resolve("node-gyp/bin/node-gyp.js")
} catch (er) {
    return cb(new Error("No gyp installed with npm"))
}
var gyp = path.execPath + " " + JSON.stringify(ourGyp)
pkg.scripts[stage] = pkg.scripts[stage].replace(/^node-gyp( |$)/, gyp)

Two things seem to be off here:

  • env.npm_lifecycle_script, which is what is actually being executed later, is assigned before the replacement.
  • path.execPath is undefined. I'm guessing it should have been process.execPath.

Furthermore, this replacement doesn't seem to have any meaning in either case, as the PATH environment variable is updated a few lines up, favoring the locally bundled version of node-gyp prior to anything else. That means that a script referencing node-gyp should hit the right version anyhow.

Owner

isaacs commented Aug 4, 2012

Indeed, it is clearly wrong, and removing is the best option.

The original intent was to handle cases where our node-gyp script was not executable, but clearly this isn't doing that anyway, and no one seem to be complaining about that.

Landed on cb5a9de. Thanks!

@isaacs isaacs closed this Aug 4, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment