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

Added node-gyp to dependencies list #924

Closed
wants to merge 1 commit into from
Closed

Added node-gyp to dependencies list #924

wants to merge 1 commit into from

Conversation

viatsko
Copy link

@viatsko viatsko commented Aug 28, 2017

Otherwise, an installation will fail on yarn unless node-gyp was installed globally via npm first.

[4/4] ⠁ sharp
[4/4] Building fresh packages...
error /Users/viatsko/projects/homepage/node_modules/sharp: Command failed.
Exit code: 127
Command: sh
Arguments: -c node-gyp rebuild
Directory: /Users/viatsko/projects/homepage/node_modules/sharp
Output:
sh: node-gyp: command not found

Otherwise, an installation will fail on yarn unless node-gyp was installed globally via npm first.

```
[4/4] ⠁ sharp
[4/4] Building fresh packages...
error /Users/viatsko/projects/homepage/node_modules/sharp: Command failed.
Exit code: 127
Command: sh
Arguments: -c node-gyp rebuild
Directory: /Users/viatsko/projects/homepage/node_modules/sharp
Output:
sh: node-gyp: command not found
```
@lovell
Copy link
Owner

lovell commented Aug 28, 2017

Hello, thanks for this.

Is having node-gyp as a direct dependency a common pattern used elsewhere? In my experience the node installation will make node-gyp available so I would have expected this issue to have come up long before now if it was causing problems. Could this be a documentation improvement?

@viatsko
Copy link
Author

viatsko commented Aug 28, 2017

Hey, I've checked multiple packages and they were using node-gyp/node-pre-gyp as part of dependencies, e. g. https://github.com/nodegit/nodegit/blob/master/package.json

The problem might also be related to the inconsistency of where yarn stores binary packages as I see that yarn tries to install node-gyp as well as a part of a process (probably, side-effect of having yarnpkg installed by multiple ways).

But as adding to dependencies solves the problem for this case, I'd go for it.

@lovell
Copy link
Owner

lovell commented Aug 28, 2017

Thanks. yarnpkg/yarn#3240 suggests recent versions of yarn will attempt to auto-install node-gyp on-demand. Is this not working for you?

@viatsko
Copy link
Author

viatsko commented Aug 28, 2017

Hi. As I said, it's kinda working, but then path of node-gyp installation is unpredictable if you have multiple yarn installs. While this is exceptional case, I'll leave up to you whether we can still help users with such an issue taking into account many other libraries keep node-gyp as a dep.

@lovell
Copy link
Owner

lovell commented Aug 28, 2017

Might those using multiple yarn installations be considered "power users"? I think I'd prefer to avoid adding a ~6MB dependency for everyone (sharp currently has ~2MB of dependencies, so this would represent a ~300% increase) for the sake of helping a few people who can probably work out what's wrong.

Let's leave this open for now to gather further opinions. Thanks again for bringing this possible problem to my attention via a PR.

Repository owner deleted a comment from coveralls Sep 9, 2017
@lovell
Copy link
Owner

lovell commented Mar 4, 2018

There have been no further requests for this change and #186 removes the need for node-gyp for most users so let's close this PR. Thanks for the suggestion and feedback.

@lovell lovell closed this Mar 4, 2018
@pronebird
Copy link

pronebird commented Mar 4, 2018

@lovell I stumbled upon exactly the same issue and I thought I was losing my mind. I am not exactly sure who's the one to blame (either yarn or or absence of node-gyp in sharp package) and I am not sure either why it works for npm, does it install node-gyp automatically? I don't know... I switched one of my projects to NPM because of this.

Whooooa #186 fixes this!

@arcanis
Copy link

arcanis commented Mar 25, 2019

Hey @lovell! Would you consider reconsidering this?

  • Not listing node-gyp in your dependencies leaves it to the package manager to download any node-gyp version it wants, which can lead to non-deterministic installs.

  • Yarn is trying to enforce boundaries between packages more and more, for example by forbidding packages from doing require calls on packages they haven't listed in their dependencies. In this context, the implicit node-gyp dependency is something I'd like to deprecate (we would still support it if users explicitly add it to their own dependency list, but we wouldn't use any random version).

  • Additionally, I'm not sure whether it's expected or not, but my system doesn't benefit from the prebuilt files. Since I'm on what I would think is a regular OSX install, it would lead me to think that more users are affected than anticipated.

  • Modern package managers now have tools to prevent installing the files on the disk (pnpm uses symlinks, and Yarn uses a static resolution table to the cache). Adding a dependency on node-gyp wouldn't have any negative impact on your users.

@lovell
Copy link
Owner

lovell commented Mar 25, 2019

@arcanis Hi Maël, thank you for helping to maintain yarn and continuing to improve dependency management tooling.

As you've probably read above, the main reasons node-gyp isn't currently listed as a dependency of sharp are a combination of the assumption that package managers have been including it since time immemorial, I've yet to see a problem as a result of the "wrong" version of node-gyp, and ~90% of installs result in the use of prebuilt binaries so only a minority of users ever require it.

A decision to include node-gyp as a dependency of sharp would be made much easier by a means of separating install-time dependencies from runtime dependencies. Is this something you're aware of for either yarn and/or npm?

"my system doesn't benefit from the prebuilt files"

Are you able to provide more details of this?

@arcanis
Copy link

arcanis commented Mar 26, 2019

A decision to include node-gyp as a dependency of sharp would be made much easier by a means of separating install-time dependencies from runtime dependencies. Is this something you're aware of for either yarn and/or npm?

I don't think we have this exact feature (the problem is that we already have dependencies, devDependencies, peerDependencies and it's hard to add a new one in a scalable way - then people will ask devBuildDependencies etc 😅). However, Yarn has an "optional peer dependency" feature which might help you achieve something similar:

{
  "peerDependencies": {
    "node-gyp": "*"
  },
  "peerDependenciesMeta": {
    "node-gyp": {
      "optional": true
    }
  }
}

One potential issue is that because of its new peer dependency, sharp will get instantiated once for every package that requires it (since the exact peer dependency might change depending on who's using the sharp package). Depending on the memory footprint it might be an issue - even though in practice it will probably only be used by one or two packages at most. 🤔

Are you able to provide more details of this?

Sure, what info can I provide? I'm on Darwin 18.2, x86_64. This is the content of my sharp/vendor/lib folder:

https://gist.github.com/arcanis/c04a5343b3d086efd9372e0bd2a7bf07

@arcanis
Copy link

arcanis commented Mar 26, 2019

Oh, and I use various Node versions, usually cutting edge. At the moment I'm on 11.10.

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

Successfully merging this pull request may close these issues.

4 participants