search

alexanderGugel / ied

:package: Like npm, but faster - an alternative package manager for Node
http://alexandergugel.github.io/ied
MIT License
1.99k stars 53 forks source link

execSync successfully installs with npm but not ied #101

Closed kirbysayshi closed 8 years ago

kirbysayshi commented 8 years ago

While investigating #100, I also discovered that a particular npm package installs fine with npm but not ied: execSync, probably due to how ied bails when it encounters a lifecycle script error.

execSync is a very old package, but is depended upon by a few popular packages, include codecov.

npm2&3 appear to somehow allow this package to install without failing the entire install. I'm not sure how. The package has a binding.gyp with no preinstall script, but does have an install script defined that manually calls node-gyp. ied inserts node-gyp as a preinstall script, which fails due to the native extensions being old.

I'm assuming this isn't the only package that might have broken extensions like this, and that this is a blocker to ied fully replacing npm.

kirbysayshi commented 8 years ago

Also, I have a potential fix for this, but it's hacky: check if the command included node-gyp within runCommand, and if so then don't cb(Error), instead use cb(null). It's pretty hacky, but if that sounds ok I'm more than willing to submit a PR!

just-boris commented 8 years ago

Tried to reproduce it and successfully installed the package.

Do you have node-gyp globally installed? Unlike npm we don't ship it by default, so you need to do it manually npm install -g node-gyp. Then it should be okay. At least, it worked for me.

kirbysayshi commented 8 years ago

@just-boris I do have node-gyp globally installed, and it definitely fails to install on OS X 10.11.2.

But maybe it's related to me using nvm? Do you use that?

kirbysayshi commented 8 years ago

Also realized that I didn't post my node version, which is 0.12.

kirbysayshi commented 8 years ago

log with version output: https://gist.github.com/kirbysayshi/617475b81961720ebccb51e83e4084eb

just-boris commented 8 years ago

Reproduced on 0.12. I think the thing here is that the package has "engines": {"node": "<0.11"} section. So anyway it doesn't work actually, either npm or ied.

just-boris commented 8 years ago

Well, I found the actuall issue. https://github.com/npm/docs/issues/636 Docs doesn't match with actual behavior. We should change this bit of our logic too.

kirbysayshi commented 8 years ago

@just-boris Nice catch with the phase. You're right that the package doesn't work when required, and in fact codecov even conditionally requires it: https://github.com/codecov/codecov-node/blob/ae1931bbafee196fd2ad9fa91c608fcf0283a40f/lib/services/localGit.js#L3, presumably only for older node versions.

But even with your change, ied install execSync fails (returns a non-zero exit code), while npm install execSync succeeds.

So this still seems like an area where ied needs a special case or is missing some hidden npm logic.

just-boris commented 8 years ago

Updated my commit. I even tested it on install execSync. Now it gives the same output as npm