Upstream gyp has actually been updated somewhat recently. Review those changes and sync (some of) them?

[DeeDeeG]

Take a look at the commit history for upstream (Google) gyp: https://chromium.googlesource.com/external/gyp/+log/refs/heads/master

As I am writing this, upstream gyp has updates as recently as the 12th of May 2020. These might fix bugs that we are still affected by. (I don't think the NodeJS or node-gyp copies of gyp have been synced with upstream (Google) gyp in quite some time, right? Maybe as long ago as 2015?)

Unexpectedly to me, there are many Python 3-related patches landed there in 2018, 2019 and 2020. About half of the commits in those years are Python 3 related. But there are other fixes as well.

Perhaps these are redundant to changes here, but perhaps not. I think it would be worthwhile seeing what has changed there, and if we could benefit from those things here/in the NodeJS world.

[cclauss]

What is broken that we are trying to fix?

[DeeDeeG]

For example, Pull Request #98 was needed because we lacked a fix that was in upstream for three and a half years: #98 (comment)

[ryzokuken]

@DeeDeeG would you mind making a PR that applies these new commits on top of this fork? That would make it much easier to review and merge.

[DeeDeeG]

I have some other things coming up, but I suppose I will put this on my to-do list.

There are hints in the nodejs/node repo about when the last sync happened. The best indication I can find is that we are synced with upstream (Google) gyp circa August 2017.

(nodejs/node PR: nodejs/node#14718, upstream commit: 324dd16)

[ryzokuken]

Yeah, thanks for finding this. That seems from ages ago, but I am not surprised. Perhaps we could maintain that information in this repo now?

[DeeDeeG]

Now that the NodeJS org's copies of gyp are unified in this gyp-next repo, maybe just having it in the commit history/changelog is good enough? I was planning to title any sync PR as (roughly):

• feat: sync with upstream gyp [short_commit_hash_here].

With a footer in the commit message body:

• Ref: https://chromium.googlesource.com/external/gyp/+/[full_commit_hash_here]

If that's not enough, I suppose we could have a file LAST_SYNCED_UPSTREAM_COMMIT with the sha (and the URL?) that we last synced. And document somewhere that this file should be updated when syncing... (perhaps mention it in CONTRIBUTING.md?)

[cclauss]

feat: sync with chromium gyp [short_commit_hash_here] might be more self-documenting because the break was so complete and there has been so much divergent evolution that many might be confusing by calling chromium to be upstream.

[cclauss]

My sense is that we have done a lot of modernization that "upstream" has not done.

Their most recent PR uses six to maintain compatibility with Python 3.3 which is long dead. This repo no longer even uses six because we have no need/desire to support legacy Python.

If there are good ideas that we want to cherry-pick from upstream, please open individual, focused issues explaining why it would be worth the effort.

[DeeDeeG]

FWIW, I did try and go through and apply most of the commits from Google's repo some time ago.

My informal (subjective) takeaway (click to expand)

I got the impression that most of it had actually already been cherry-picked to here, usually in some modified form making for minor conflicts (yay merge conflicts! /s). And still more work was re-implemented here so as to make the Google commits redundant to what we already have at this repo. (Especially Python 3 compatibility was redundant, as we obviously do have at NodeJS's gyp-next repo, at least for the files we care about.)

A couple of things were genuinely present in the Google repo and absent in the gyp-next repo... See the chart for those.

That said, here are my notes, in case anyone is interested...

How to read/interpret these notes (click to expand)

These are listed in reverse-commit-order (oldest commits are at the bottom, most-recent commits are at the top...) but I tried applying them in normal order, so my work started at the bottom of this table and went up from there. And I stopped part-way through upon encountering a massive amount of print(something) vs print something that I didn't know how to deal with... Something like that. Anyhow, here are my notes from way back then:

Commits I didn't get to:

• caa60026 (chromium/master) Add support for CLANG_ENABLE_OBJC_WEAK
• e87d37d6 Prevent propogation to configurations of nested 'dependent-settings' directives
• fcd686f1 Add .tbd to the .xcodeproj generator’s extension map
• a8921fca fix encoding issues with inputs for better Python 3 support
• 12ef00df fix make output checks under C locale
• aca1e2c3 Fix Python 3 incompatibilities
• 4f1618ab Add missing b'' qualifier from da63cb0f.
• | da63cb0f Misc python3 fixes in xcode_emulation. | skipped because unsure |
• 5c270f76 Fix incorrect depfile path relativization
• 9f2a7bb1 Add flake8 checks to travis configuration.
• 8bee09f4 Changes for windows and easy xml to get gyp to run under python 3.
• bd11dd1c More miscellaneous fixes for Python3 compatibility.

Commits I did look at, and what I did about them at the time:

commit	emoji	verdict	rationale
703706c4 src,win: add VS2019 version			We have this already... small discrepancies in project_version and default_toolset
e22714e5 Fixes needed for Python3 on Win32.		TEST_ONLY
732bde62 Fix one dangling utf8 decode call needed for python 3.		TEST_ONLY
ab4aca86 Mostly mechanical changes for Python3 support.	✅	partly already applied	Seems much of this was cherry-picked during #11; although that PR claims to simply be cherry-picking a much smaller Chromium commit, it truly cherry-picks much of this one as well. Note: not sure about all the collections.Iterable and basestring stuff, so I left it out as previous cherry-pickers at this repo seem to have done when cherry-picking this Chromium commit for gyp-next.
f2dca32f Update simple_copy.py for Python3 types.	⎗🚫	Independently re-implemented by us	This was independently re-done in NodeJS-land with effectively identical behavior in nodejs/node-gyp#1793
f989ef9f Use ast module instead of compiler module for parsing files.	⎗🚫	Already cherry-picked	We have pulled this commit from Chromium already. What a long cherry-pick chain... See: nodejs/node#29140 --> nodejs/node-gyp#1820 --> nodejs/node-gyp#1820 --> https://chromium-review.googlesource.com/c/1360352 --> https://codereview.chromium.org/1454433002/
9df93ee4 Make Visual Studio 2017 Community Edition work w/ GYP.	🚫	not wanted	This commit only modifies a test file we don't have. Not applicable to our fork.
6dbf304b Add a copyright header to TestCmd.py.	🚫	not wanted	This adds a copyright header to a test-related file we don't have. Not applicable to our fork.
f825c98e Fix issue with missing resources in Xcode ui tests targets.	✅	wanted	Some bug fix about compile targets being left out I guess. For the Xcode generator.
197c82b7 Patch GYP so that building with Xcode 10 works.	✅	wanted	Nice. (I guess, I haven't tested this personally.) This updates the xcode generator to (apparently) add Xcode 10 support. See this for discussion/review comments by author and reviewer at Chromium gyp: https://chromium-review.googlesource.com/c/external/gyp/+/1318929
81286d38 infra: remove cq.cfg, which is no longer used.	🚫	not wanted	Deleting some old Chromium infrastructure file in favor of Travis CI. We don't care about this at gyp-next.
4d467626 Make Travis build only the master branch	🚫	not wanted	Travis CI
deb62526 Disable some tests that fail on bots to try to get a green build	🚫	not wanted	Disabling some tests to try to get Travis CI working, apparently. We don't even have these tests anymore in gyp-next; It seems they have been deliberately excluded.
d22dd971 and try clang	🚫	not wanted	Travis CI
7f294103 readd osx	🚫	not wanted	Travis CI
0afd3fc1 try a wrapper script	🚫	not wanted	Travis CI
007db9ef see if ninja is getting pulled	🚫	not wanted	Travis CI
39ad9f30 see if ninja is getting pulled	🚫	not wanted	Travis CI
541da539 try other order for sync:	🚫	not wanted	Travis CI
c3b797d8 try exporting PATH and only build ninja for now	🚫	not wanted	Travis CI
834a0592 fiddling with directories	🚫	not wanted	Travis CI
2ea7773b set +x on buildbot/travis-checkout.sh	🚫	not wanted	Travis CI
2f9ae921 add copyright to .sh	🚫	not wanted	Travis CI
6dbd6e1e .	🚫	not wanted	Travis CI
ac24c9a9 osx	🚫	not wanted	Travis CI
85a21920 .	🚫	not wanted	Travis CI
52d9dcea Add prototype of Travis config	🚫	not wanted	This is some Travis CI configuration/setup. I'm pretty sure we don't want this, in favor of GitHub Actions.
5e2b3ddd Remove Rietveld CQ config.	🚫	not commit-able	A Chromium CI metadata file. This file was deliberately deleted in gyp-next.
365ffa05 Flip to LUCI for tryjobs.	🚫	not commit-able	A Chromium CI metadata file. This file was deliberately deleted in gyp-next.
f7258620 Provide backward compatibility for python 2.7.6 on z/OS	✅⎗	fixup	fixup for Chromium dd0eafbd and 30a29189, redundant to code already in gyp-next that handles both "zos" and "os390" platform identities in a unified way.
44ad5a0a Add LIBS to the link command on z/OS	✅⎗	fixup for Chromium dd0eafbd, redundant to/already included in nodejs/node-gyp#1276	This really shouldn't be cherry-picked, we have the better version of this already.
30a29189 Change z/OS platform flavor name based on python 2.7.13	🚫	Incompatible with/downgrade from code already in gyp-next	fixup to existing commits, but we already handle both zos and os390, so this change doesn't really make a lot of sense. It shouldn't have removed os390 in favor of zos, it should have supported both like we already do in gyp-next.
c6f47168 make,ninja: Add support for an LDFLAGS_host environment variable.	⎗	Duplicate: #98	fix was re-implemented in a gyp-next PR, intentionally referencing/copying the Chromium commit.
dd0eafbd Support z/OS platform	⎗	Duplicate: nodejs/node-gyp#1276	We have had (almost identically) this for a while, same author, landed at node-gyp and Chromium gyp in parallel.

Legend (meaning) for the emojis:

• checked mark ✅ means I decided to cherry-pick this commit locally
• "previous page" symbol ⎗ means already included via a similar or duplicate commit in nodejs/node or nodejs/node-gyp
• "prohibited" symbol 🚫 means not commit-able, due to being wholly redundant or incompatible with changes in nodejs/gyp-next, or simply not relevant/wanted at gyp-next