Repository navigation
Upstream gyp has actually been updated somewhat recently. Review those changes and sync (some of) them? #108
Description
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.
Activity
cclauss commentedon May 12, 2021
What is broken that we are trying to fix?
DeeDeeG commentedon May 12, 2021
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 commentedon May 14, 2021
@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 commentedon May 14, 2021
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/noderepo about when the last sync happened. The best indication I can find is that we are synced with upstream (Google)gypcirca August 2017.(
nodejs/nodePR: nodejs/node#14718, upstream commit:324dd16)ryzokuken commentedon May 14, 2021
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 commentedon May 14, 2021
Now that the NodeJS org's copies of
gypare unified in thisgyp-nextrepo, 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_COMMITwith the sha (and the URL?) that we last synced. And document somewhere that this file should be updated when syncing... (perhaps mention it inCONTRIBUTING.md?)cclauss commentedon May 15, 2021
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 commentedon Sep 4, 2022
My sense is that we have done a lot of modernization that "upstream" has not done.
Their most recent PR uses
sixto maintain compatibility with Python 3.3 which is long dead. This repo no longer even usessixbecause 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 commentedon Sep 4, 2022
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)vsprint somethingthat 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:
Commits I did look at, and what I did about them at the time:
project_versionanddefault_toolsetcollections.Iterableandbasestringstuff, 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.xcodegenerator 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/+/1318929gyp-next.gyp-next; It seems they have been deliberately excluded.gyp-next.gyp-next.gyp-nextthat handles both "zos" and "os390" platform identities in a unified way.gyp-nextzosandos390, so this change doesn't really make a lot of sense. It shouldn't have removedos390in favor ofzos, it should have supported both like we already do ingyp-next.gyp-nextPR, intentionally referencing/copying the Chromium commit.node-gypand Chromiumgypin parallel.Legend (meaning) for the emojis:
nodejs/nodeornodejs/node-gypnodejs/gyp-next, or simply not relevant/wanted atgyp-next