Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bin conflict with gulp & gulp-cli #7

Closed
heikki opened this issue Jan 5, 2015 · 53 comments
Closed

Bin conflict with gulp & gulp-cli #7

heikki opened this issue Jan 5, 2015 · 53 comments
Labels

Comments

@heikki
Copy link
Contributor

heikki commented Jan 5, 2015

Npm install fails when having gulp-cli as dependency and gulp as devDependency. This is probably because both of them try to link their own gulp bin.

∴ slush git:(4.0) npm install

> [email protected] install /Users/heikki/Desktop/heikki/slush/node_modules/gulp/node_modules/v8flags
> node fetch.js


> [email protected] install /Users/heikki/Desktop/heikki/slush/node_modules/gulp-cli/node_modules/v8flags
> node fetch.js

npm ERR! Darwin 14.0.0
npm ERR! argv "node" "/Users/heikki/.nvm-fish/v0.10.35/bin/npm" "install"
npm ERR! node v0.10.35
npm ERR! npm  v2.1.17
npm ERR! path ../gulp-cli/bin/gulp.js
npm ERR! code EEXIST
npm ERR! errno 47

npm ERR! EEXIST, symlink '../gulp-cli/bin/gulp.js'
File exists: ../gulp-cli/bin/gulp.js
Move it away, and try again. 

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/heikki/Desktop/heikki/slush/npm-debug.log
@heikki
Copy link
Contributor Author

heikki commented Jan 5, 2015

Is gulp-cli meant to be installed globally in the future? I'm wondering if both of them need the bin.

I run into this issue in my slush fork: https://github.com/heikki/slush/tree/4.0

@phated
Copy link
Member

phated commented Jan 5, 2015

gulp-cli was meant to be installed globally but then aliased in gulp so when it is installed locally, you can easily do gulp whatever in npm scripts inside package.json without adding gulp-cli to your devDeps. This was a problem @tkellen pointed out with grunt's approach to grunt-cli. His comment is gulpjs/gulp#805 (comment)

@tkellen
Copy link

tkellen commented Jan 5, 2015

Whoops, didn't read this code yet. By making gulp-cli a dependency of gulp it should make gulp available in node_modules/.bin when installed locally, but it won't make it available globally--for that you'd have to install gulp-cli. Let me confirm these assumptions later today, it's been a long time since I looked at this.

@heikki
Copy link
Contributor Author

heikki commented Jan 5, 2015

I'd be happy to use a workaround for this case. So far I have found only bad ones 😄
Anyways, there could be issues if people install both gulp & gulp-cli locally or globally.

"postinstall": "[ \"$NODE_ENV\" == \"production\" ] || npm install gulp"

@tkellen
Copy link

tkellen commented Jan 6, 2015

So, my previous assumptions were incorrect, but, @heikki why are you depending on both gulp-cli and gulp at the same time? You should never need gulp-cli in your dependencies, just install the version of gulp you want and it'll have the binary you need.

@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

@tkellen I linked the slush fork above. I'm trying to modify slush to use gulp-cli instead of copying the same code there. gulp was there just for tests i.e. there's a gulpfile that gets called by slush.

Workaround is fine for me. I just thought to bring this up because I'd imagine ppl can get easily things messed. For example install both globally, then uninstall one and boom there's no bins anymore.

Should this be closed as wontfix or is there alternatives left?

@tkellen
Copy link

tkellen commented Jan 6, 2015

Unfortunately, I think this is a wontfix, both because I think your particular implementation is enough of an edge case, and because I can't think of a good solution :/.

@tkellen
Copy link

tkellen commented Jan 6, 2015

cc @contra was going to close this but looks like I don't have privs here? Just checked for the gulp repo--same deal.

@heikki heikki closed this as completed Jan 6, 2015
@phated
Copy link
Member

phated commented Jan 6, 2015

@heikki I'd like a way to solve this but we really only have 2 options:

  1. Have as a dep of gulp and have the clashing bins when installed next to each other. - I want to look into npm further to see if we can avoid failing, like an optional bin or something
  2. Make gulp-cli a peerDep of gulp so they both get installed upon npm install, this works with --save but the remove doesn't. On top of that, peerDeps have a lot of weird edge cases.

@phated phated reopened this Jan 6, 2015
@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

3 . Remove bin from gulp-cli and keep using gulp for both local & global installs

Global install would be bigger but wouldn't it solve this issue? Am I overlooking something?

@phated
Copy link
Member

phated commented Jan 6, 2015

It's a versioning thing. It will be confusing that gulp 3.whatever globally installed can run gulp 4.whatever but only if it is local. Having a separate (smaller being another benefit) module that is versioned separately makes that distinction easier.

@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

I agree. I'd still rather have that if it prevents breaking things accidentally. I usually install global modules without specifying version and occationally update all of them to latest and expect that things keep working. Random notes:

  • Separating gulp-cli would still have benefits (including only code, no bin)
  • From user perspective local/global gulp would keep working as usual
  • 'Warning: gulp version mismatch:' could be more explicit 'Warning: global gulp is outdated, please run npm update -g gulp' or similar
  • Crazy idea of using only global gulp for quick tests might be possible Split CLI out into a module gulp#827 (comment)

--edit

Just noticed that 4.0 branch warnings are already different

@phated
Copy link
Member

phated commented Jan 6, 2015

Currently, local/global already works as expected. You can still install gulp globally and have it work. That is the reason for the duplicate bin files. I don't like the hack to work with NODE_PATH because it will work when the bug in the resolve module is fixed.

@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

Ahaa, wasn't aware of that. Looks like PR got rejected though.

@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

Hmm.. what about having bin only in gulp-cli and requiring user to install that locally in order to have npm script thing (grunt does that). That would break existing behaviour. Is it possible to make gulp / gulp-cli separation only from 4.0 up and have gulp-cli launch 3.x gulp?

Lol, let me know if that didn't make sense..

@phated
Copy link
Member

phated commented Jan 6, 2015

It makes sense but @tkellen suggested not to do that, as it was a pain point in grunt, which is what lead to the current implementation.

@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

I'm curious what kind of problems there were.

gulpjs/gulp#805 (comment)

grunt-cli was created to address this exact problem. our codebase was never re-factored to remove the grunt dependency in the cli itself, which makes our usage of this pattern a little strange, though (it needs to be there for the help functionality and a bunch of other things).

I'm probably reading this wrong but I don't understand the dependency part. At least I can't see direct dependencies in grunt / grunt-cli package.jsons.

@phated
Copy link
Member

phated commented Jan 6, 2015

@heikki it is for the people that ONLY use npm scripts, like myself. It is a pain to install gulp and gulp-cli in my devDeps so I can use npm scripts

@heikki
Copy link
Contributor Author

heikki commented Jan 6, 2015

That explains.. 😄 Can't get better (and more logical) cli separation than that.

@phated
Copy link
Member

phated commented Jan 11, 2015

Looking into this more.

I just attempted npm install gulpjs/gulp#4.0 gulpjs/gulp-cli#4.0 and it works sometimes. This seems to be a race condition in the unlinking of the previous bin and I will talk with @othiym23 about it. They don't seem to have a problem when installed separately because of the unlink. I think the EEXIST is also a bug in npm given the behavior of global. Will ask about that also.

@othiym23
Copy link

I would believe there's an issue here, and it would be a lot easier for me to get traction on establishing where the issue is if you could do me a solid and reduce the failure to a minimal test case. File an issue in the meantime, so I have something I can track. Thanks!

It's worth pointing out that this will 100% be affected by the changes to the install in npm@3, which @iarna is getting close to releasing as a beta. I'm mentioning her so she knows this is an edge case to watch out for.

@heikki
Copy link
Contributor Author

heikki commented Jan 13, 2015

@othiym23 Is simple package.json ok for test case?

https://gist.github.com/heikki/eae172b8d1c5a97ed5ef#file-package-json

@othiym23
Copy link

@heikki I can't get the attached package.json to barf, at least with [email protected] on Windows 8.1. If it were failing semi-reliably, yes, that would absolutely be enough for me to work from.

@heikki
Copy link
Contributor Author

heikki commented Jan 13, 2015

@othiym23 on osx & [email protected] I see fails maybe 50% of the time. I delete node_modules after each try. Not sure if that makes any difference.

@phated
Copy link
Member

phated commented Jan 14, 2015

@othiym23 was there a change in 2.2 that would cause this to not happen anymore? Also, the command npm install -g gulpjs/gulp#4.0 gulpjs/gulp-cli#4.0 can be used to get this to happen for a global install ~50% of the time on OSX.

@heikki
Copy link
Contributor Author

heikki commented Jan 14, 2015

I'm seeing this in 2.2 too. Mentioned in npm issue: npm/npm#7130

@phated phated added the blocked label Jan 26, 2015
@silkentrance
Copy link

@phated while it seems nice to have both packages expose a bin/gulp.js, it might not be the wisest decision and also might oppose all best practices established so far. I would go for the standard way and have both a gulp-cli package that exposes the binary and a second API package that exposes the API.

And since the tool/API is in a rather early stage of development I would simply go for it instead of waiting for the peops over at npm to fix the issue, considering their overall backlog and their current focus on enterprise level features and npm registry2 and what not.

@phated
Copy link
Member

phated commented Jan 26, 2015

We are going to ship both. It is annoying to have to have something like gulp-cli in devDeps just to have access to the bin in your npm scripts

@sindresorhus
Copy link
Contributor

@phated I disagree. Separation of concerns is a good thing. "one way of doing things" is too.

@phated
Copy link
Member

phated commented Jan 26, 2015

@sindresorhus I don't care because @tkellen's advice was correct. I hate how grunt does it currently

@silkentrance
Copy link

@phated the way grunt and other tools do it works just fine. @tkellen please advise.

@phated
Copy link
Member

phated commented Jan 26, 2015

@silkentrance dude, it's already been quoted and linked in this thread. Locking.

@phated
Copy link
Member

phated commented Jul 12, 2015

@heikki would you be able to test these use cases again but with npm 3.x? If npm3 has solved this (or at least gave us a determinate behavior), then I'd be good with shipping the CLI.

@heikki
Copy link
Contributor Author

heikki commented Jul 13, 2015

@phated

I don't see EEXIST or ENOENT errors anymore with npm 3.1.0.

Linked gulp bin varies:

∴ npm-install-bin-conflicts git:(master) ./enoent.sh
Running 'npm install gulp gulp-cli' repeatedly
install 1, gulp bin -> ../gulp-cli/bin/gulp.js
install 2, gulp bin -> ../gulp/bin/gulp.js
install 3, gulp bin -> ../gulp/bin/gulp.js
install 4, gulp bin -> ../gulp/bin/gulp.js
install 5, gulp bin -> ../gulp/bin/gulp.js
install 6, gulp bin -> ../gulp/bin/gulp.js
install 7, gulp bin -> ../gulp/bin/gulp.js

∴ npm-install-bin-conflicts git:(master) ./eexist.sh
Running 'rm -fr node_modules && npm install gulp gulp-cli' repeatedly
install 1, gulp bin -> ../gulp-cli/bin/gulp.js
install 2, gulp bin -> ../gulp-cli/bin/gulp.js
install 3, gulp bin -> ../gulp/bin/gulp.js
install 4, gulp bin -> ../gulp-cli/bin/gulp.js
install 5, gulp bin -> ../gulp-cli/bin/gulp.js
install 6, gulp bin -> ../gulp-cli/bin/gulp.js
install 7, gulp bin -> ../gulp-cli/bin/gulp.js

npm uninstall -g gulp still removes the gulp bin even if it belongs to gulp-cli package:

~ npm list -g
/Users/heikki/.nvm/versions/node/v0.12.7/lib
├── [email protected]
└── [email protected]~ npm install -g gulp-cli
/Users/heikki/.nvm/versions/node/v0.12.7/bin/gulp -> /Users/heikki/.nvm/versions/node/v0.12.7/lib/node_modules/gulp-cli/bin/gulp.js
/Users/heikki/.nvm/versions/node/v0.12.7/lib
└── [email protected]~ gulp -v
[15:10:14] CLI version 0.3.0

∴ ~ npm uninstall -g gulp

∴ ~ gulp -v
fish: Unknown command 'gulp'

Test scripts here

@iarna
Copy link

iarna commented Jul 13, 2015

These both look like npm bugs to me– given the same module list it should be linking modules in the same order and apparently it isn't. And given that gulp-cli owns the link after the second install, removing gulp absolutely shouldn't remove it.

I'll open these as issues over on the npm tracker and link here.

@phated
Copy link
Member

phated commented Jul 13, 2015

@iarna thank you so much!

@iarna
Copy link

iarna commented Aug 14, 2015

Hey you all, can any of you reproduce the "different install orders" thing with [email protected]? I'm pretty sure that can't happen any more.

@heikki
Copy link
Contributor Author

heikki commented Aug 14, 2015

Yes, details here -> npm/npm#8995 (comment)

@yocontra
Copy link
Member

The new npm is out, can we test and verify that this is fixed?

@phated
Copy link
Member

phated commented Aug 21, 2015

@contra it isn't out yet. Should be out today, I think

@phated
Copy link
Member

phated commented Aug 21, 2015

For reference, the version we want to test this against will be 3.3.1 (currently "next" is 3.3.0)

@iarna
Copy link

iarna commented Aug 22, 2015

Yup, 3.3.1 got bumped to next week for process reasons. If you already have 3.x installed somewhere, you can try out 3.3.1 by installing npm/npm#release-3.3.1 with it.

@heikki
Copy link
Contributor Author

heikki commented Aug 22, 2015

Install order looks consistent with npm/npm#release-3.3.1.

Running 'npm install gulp gulp-cli' repeatedly
install 1, gulp bin -> ../gulp-cli/bin/gulp.js
install 2, gulp bin -> ../gulp-cli/bin/gulp.js
install 3, gulp bin -> ../gulp-cli/bin/gulp.js
install 4, gulp bin -> ../gulp-cli/bin/gulp.js
install 5, gulp bin -> ../gulp-cli/bin/gulp.js
install 6, gulp bin -> ../gulp-cli/bin/gulp.js
install 7, gulp bin -> ../gulp-cli/bin/gulp.js

@phated
Copy link
Member

phated commented Sep 9, 2015

🎉 🎈 🎂 🎆 🍻 Closing this now.

@amcsi
Copy link

amcsi commented Nov 27, 2016

For those of you having this issue with yarn (since they didn't fix it there I think), you can fix this by manually adding this to your package.json:

  "bin": {
    "react-native": "node_modules/react-native-cli/index.js"
  },

And then removing node_modules and your lockfile and installing again.

With this your local package.json's bin entry will take precedence over the ones defined in the external packages, and you should be able to control whose bin file to use this way.

EDIT: actually even locally defining it doesn't give it precedence -.-

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

No branches or pull requests

10 participants