gyp: sync code base with nodejs repo #1975

targos · 2019-11-20T18:58:36Z

This syncs the gyp code base so that it is identical to the one in nodejs/node.

This was done by removing the code in nodejs/node-gyp, copying from nodejs/node and then manually reviewing and adapting the diff.

targos · 2019-11-20T18:59:01Z

PR in nodejs/node: nodejs/node#30563

rvagg · 2019-11-21T05:17:22Z

gyp/pylib/gyp/generator/make.py

@@ -1951,13 +1951,11 @@ def _InstallableTargetInstallPath(self):
    """Returns the location of the final output for an installable target."""
    # Xcode puts shared_library results into PRODUCT_DIR, and some gyp files
    # rely on this. Emulate this behavior for mac.
-
-    # XXX(TooTallNate): disabling this code since we don't want this behavior...


note the TODO here specifically for node-gyp, I wonder how much this matters still? is it just a case of disallowing end-user from specifying shared_library in their binding.gyp? 🤷‍♂

Change was done in a2ed0df

If we need to keep it here, can we also apply it to node? I don't know the implications...

I don't know what to do with this. The change is in node-gyp since 2012, the commit message doesn't explain why it was done and I can't find the pull request that introduced it.

I'll try to remove this code in nodejs/node and see if CI is happy. Maybe the gyp files that rely on this are not ours.

Well, removing these lines breaks the Node.js build: https://ci.nodejs.org/job/node-test-commit-linux-containered/16453/nodes=ubuntu1804_sharedlibs_shared_x64/console
So we're back to the original question: how does it matter to node-gyp?
@TooTallNate sorry to bother you, but since you committed the change, maybe you remember why it was needed?

Will this be a blocker ? or we can merge this for now and left as it is ?

I think this is not a blocker anymore.

rvagg · 2019-11-21T05:24:55Z

Very brave of you @targos.
I feel far from qualified to review this properly. Hopefully @cclauss @joaocgreis @bnoordhuis and others could pitch in?
It seems to me that there are a lot of formatting changes that can be ignored. There's a ton of Windows changes which I believe were applied to nodejs/node but are not necessary here because we make up for it with JS, but wouldn't hurt to be here. The rest is the grey stuff that it would be good to have some knowledgeable eyes over.
We could bump to v7 to release this since there's a lot of changes and I suspect we wouldn't be able to guarantee this doesn't break existing use.

targos · 2019-11-21T06:51:59Z

Most of the changes here come from upstream gyp updates that were applied to nodejs/node two to three years ago.

ryzokuken

RSLGTM, some comments in the complementary PR. Will try to review this more thoroughly later.

targos · 2019-12-01T10:32:57Z

/cc @nodejs/node-gyp

gyp/pylib/gyp/common.py

joaocgreis · 2019-12-02T15:39:32Z

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/169/

joaocgreis

This is failing in all Windows 2016 runners. This needs to be fixed before this can land.

targos · 2019-12-02T15:48:49Z

I'll need help, because it doesn't seem like the actual error message from MSBuild.exe was printed to the console.

rvagg · 2020-01-06T03:37:01Z

how about we bump semver-major with this and go with a node-gyp v7. node-gyp 5 is still being used by npm so 6 is currently for people using it manually, a bump to 7 gives us some breakage room and lets people opt-in to an upgrade, then npm could choose to jump to v7 at some point if it wants instead of v6.

gyp/pylib/gyp/generator/msvs.py

Co-Authored-By: João Reis <reis@janeasystems.com>

targos · 2020-01-30T11:21:44Z

Re-synced. Thanks @joaocgreis for the fix!

GitHub Actions passed.
Here's a CI run on Jenkins: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/175/

targos · 2020-01-30T11:33:25Z

All green!

@nodejs/node-gyp PTAL and merge if you like :)

From my point of view, this can be semver-major. I only care about unifying the code between this repo and node core, so it doesn't matter in which release this goes first.

rvagg

rubber-stamp, too hard for me to review in detail but if it passes then I guess the best we can do is plough ahead and find out by pushing it into a v7.

gengjiawen · 2020-02-15T18:18:01Z

@rvagg let's merge this to master ?

Outdated review, this looks fixed now.

joaocgreis · 2020-02-18T20:48:14Z

Sorry for not dismissing my review earlier, I missed it when I investigated the failure.

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/176/ ✔️ (this does not rebase, would be good to run CI on the final rebased version before landing)

gengjiawen · 2020-02-19T02:04:35Z

Sorry for not dismissing my review earlier, I missed it when I investigated the failure.

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/176/ ✔️ (this does not rebase, would be good to run CI on the final rebased version before landing)

Looks like the CI passed. If we have no future questions, I want to land this.

gengjiawen · 2020-02-19T03:44:02Z

@rvagg I have launched the PR. If you have any problems, feel free to revert this.

@targos I think we continue to work on nodejs/node#30563.

targos mentioned this pull request Nov 20, 2019

tools: sync gyp code base with node-gyp repo nodejs/node#30563

Closed

targos requested a review from cclauss November 20, 2019 19:07

rvagg reviewed Nov 21, 2019

View reviewed changes

ryzokuken approved these changes Nov 26, 2019

View reviewed changes

cclauss approved these changes Dec 1, 2019

View reviewed changes

cclauss reviewed Dec 1, 2019

View reviewed changes

gyp/pylib/gyp/common.py Show resolved Hide resolved

targos force-pushed the sync-gyp branch from d0e93e0 to a8f3416 Compare December 2, 2019 13:14

joaocgreis previously requested changes Dec 2, 2019

View reviewed changes

joaocgreis reviewed Jan 9, 2020

View reviewed changes

gyp/pylib/gyp/generator/msvs.py Show resolved Hide resolved

gengjiawen mentioned this pull request Jan 29, 2020

tools: sync gyp from upstream nodejs/node#31562

Closed

4 tasks

gengjiawen requested a review from joaocgreis January 29, 2020 04:57

targos and others added 4 commits January 30, 2020 12:13

gyp: sync code base with nodejs repo

9c0f3ab

fixup! gyp: sync code base with nodejs repo

5e8003d

Update gyp/pylib/gyp/generator/msvs.py

5ee5eea

Co-Authored-By: João Reis <reis@janeasystems.com>

fixup

605cb84

targos force-pushed the sync-gyp branch from 59e1ea4 to 605cb84 Compare January 30, 2020 11:15

rvagg approved these changes Feb 3, 2020

View reviewed changes

richardlau mentioned this pull request Feb 10, 2020

doc: update gyp link libuv/libuv#2681

Closed

rvagg mentioned this pull request Feb 11, 2020

ci,libuv: http://ci.nodejs.org/downloads/zos/gyp.tar.gz? nodejs/build#2172

Closed

gengjiawen merged commit 972780b into master Feb 19, 2020

targos deleted the sync-gyp branch February 19, 2020 10:22

rvagg mentioned this pull request Mar 9, 2020

build: fix compile_commands.json generation nodejs/node#32090

Closed

2 tasks

rvagg added the semver-major label May 13, 2020

rvagg mentioned this pull request May 26, 2020

v7.0.0 Proposal #2124

Closed

targos mentioned this pull request Oct 5, 2020

deps: upgrade npm to 7.0.0-rc.3 nodejs/node#35474

Closed

rvagg mentioned this pull request Oct 6, 2020

shared_library builds have moved location #2233

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gyp: sync code base with nodejs repo #1975

gyp: sync code base with nodejs repo #1975

targos commented Nov 20, 2019

targos commented Nov 20, 2019

rvagg Nov 21, 2019

targos Nov 21, 2019

targos Dec 2, 2019

targos Dec 2, 2019

targos Dec 2, 2019

gengjiawen Feb 15, 2020

targos Feb 15, 2020

rvagg commented Nov 21, 2019

targos commented Nov 21, 2019

ryzokuken left a comment

targos commented Dec 1, 2019

joaocgreis commented Dec 2, 2019

joaocgreis left a comment

targos commented Dec 2, 2019

rvagg commented Jan 6, 2020

targos commented Jan 30, 2020

targos commented Jan 30, 2020

rvagg left a comment

gengjiawen commented Feb 15, 2020

joaocgreis commented Feb 18, 2020

gengjiawen commented Feb 19, 2020

gengjiawen commented Feb 19, 2020

gyp: sync code base with nodejs repo #1975

gyp: sync code base with nodejs repo #1975

Conversation

targos commented Nov 20, 2019

targos commented Nov 20, 2019

rvagg Nov 21, 2019

Choose a reason for hiding this comment

targos Nov 21, 2019

Choose a reason for hiding this comment

targos Dec 2, 2019

Choose a reason for hiding this comment

targos Dec 2, 2019

Choose a reason for hiding this comment

targos Dec 2, 2019

Choose a reason for hiding this comment

gengjiawen Feb 15, 2020

Choose a reason for hiding this comment

targos Feb 15, 2020

Choose a reason for hiding this comment

rvagg commented Nov 21, 2019

targos commented Nov 21, 2019

ryzokuken left a comment

Choose a reason for hiding this comment

targos commented Dec 1, 2019

joaocgreis commented Dec 2, 2019

joaocgreis left a comment

Choose a reason for hiding this comment

targos commented Dec 2, 2019

rvagg commented Jan 6, 2020

targos commented Jan 30, 2020

targos commented Jan 30, 2020

rvagg left a comment

Choose a reason for hiding this comment

gengjiawen commented Feb 15, 2020

joaocgreis commented Feb 18, 2020

gengjiawen commented Feb 19, 2020

gengjiawen commented Feb 19, 2020