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

Add global node-gyp for yarn #381

Closed
wants to merge 1 commit into from

Conversation

teohhanhui
Copy link

@Daniel15
Copy link

I feel like the proper fix is for packages to include node-gyp in their dependencies if they need it, rather than relying on it to be installed globally. This looks good to me though.

@teohhanhui
Copy link
Author

teohhanhui commented Apr 20, 2017

@Daniel15 Isn't yarn intended as a drop-in replacement for npm? Then it's compulsory for node-gyp to be available out of the box: https://github.com/nodejs/node-gyp/wiki/Updating-npm's-bundled-node-gyp

npm bundles its own, internal, copy of node-gyp. This internal copy is independent of any globally installed copy of node-gyp that you may have installed via npm install -g node-gyp.

This means that while node-gyp doesn't get installed into your $PATH by default, npm still keeps its own copy to invoke when you attempt to npm install a native addon.

@chorrell chorrell added the yarn label Apr 20, 2017
@Daniel15
Copy link

Daniel15 commented Apr 21, 2017

Yes, this should be fixed in the next Yarn release. For now I think it's totally fine to yarn global add it.

I still think packages that depend on node-gyp should make that explicit rather than having the implicit dependency.

@teohhanhui
Copy link
Author

While you are still figuring out yarnpkg/yarn#3114, I think we should unbreak the Docker image.

@SimenB
Copy link
Member

SimenB commented Apr 25, 2017

I agree on unbreaking the image, but it's fixed in yarn now, just need to wait on a release. Hopefully really soon 😄 yarnpkg/yarn#3240

@Daniel15
Copy link

Daniel15 commented Apr 26, 2017 via email

@vladwing
Copy link

vladwing commented Apr 26, 2017

@Daniel15: I tested that with the alpine tag, and it doesn't work.

Dockerfile:

FROM node:7.7.4-alpine
COPY package.json /srv/application/
RUN apk add --no-cache --virtual .build-deps \
        binutils-gold \
        findutils \
        curl \
        g++ \
        gcc \
        make \
        python && apk add --no-cache postgresql-dev \
&& cd /srv/application \
&& yarn install \
&& apk del .build-deps

package.json:

{
  "name": "test-yarn",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "libpq": "^1.8.7",
    "node-gyp": "^3.6.0"
  }
}

Command:

docker build .

Error:

error /srv/application/node_modules/libpq: Command failed.
Exit code: 127
Command: sh
Arguments: -c node-gyp rebuild
Directory: /srv/application/node_modules/libpq
Output:
sh: node-gyp: not found

Adding a yarn global add node-gyp before yarn install solves the problem.

PS. I tested with other versions of node:alpine as well, but I want to keep the version locked for this bug report.

@Daniel15
Copy link

@vladwing - The issue is that libpq doesn't specify node-gyp in its dependencies.

@razor-x
Copy link
Contributor

razor-x commented May 8, 2017

@teohhanhui I am eager for this fix to be included in images that will not get the version of yarn with this fix. Is there any reason this PR is stalled? Anything I can help with to get it moving along?

@chorrell
Copy link
Contributor

chorrell commented May 8, 2017

So if we add node-gyp to the images we won't be able to remove it going forward unless we want to deal with more breakage.

Has there been discussion on an official yarn image? Having and image tagged to a given yarn version would certainly help here.

@Daniel15
Copy link

Daniel15 commented May 8, 2017

So if we add node-gyp to the images we won't be able to remove it going forward unless we want to deal with more breakage.

The images should already have node-gyp given npm comes bundled with a version of it. Adding npm's version to the PATH would also solve the problem.

Upcoming Yarn releases have fixed this issue by using npm's version of node-gyp if available.

@SimenB SimenB mentioned this pull request May 12, 2017
@pesho pesho closed this in #400 May 13, 2017
@teohhanhui teohhanhui deleted the yarn-node-gyp branch May 15, 2017 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants