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.

Sign up for GitHub

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

Jump to bottom

tools: replace GYP with GYP3 #26620

Closed
wants to merge 3 commits into from
Closed

tools: replace GYP with GYP3 #26620

wants to merge 3 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 12, 2019 •
edited by sam-github
Loading

Replace the abandoned GYP with GYP3, which has the following benefits:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Fixes: #28555

@nodejs-github-bot
Copy link
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2887/pipeline

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Mar 12, 2019
@refack refack self-assigned this Mar 12, 2019
@refack refack added wip Issues and PRs that are still a work in progress. gyp Issues and PRs related to the GYP tool and .gyp build files labels Mar 12, 2019
@refack refack added this to the 12.0.0 milestone Mar 12, 2019
@refack

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member

Trott commented Mar 12, 2019

(If possible to change now that it's opened, maybe make this a draft pull request?)

@refack
Copy link
Contributor Author

refack commented Mar 12, 2019 •
edited
Loading

(If possible to change now that it's opened, maybe make this a draft pull request?)

I was looking for how to do that... Only found it after reading the docs.

Anyway, this should be working now, and it's ready for Rubber Stamp reviews.
https://ci.nodejs.org/job/node-test-pull-request/21487/
https://ci.nodejs.org/job/node-test-commit/26674/
(I might patch GYP3 a bit more)

@refack refack changed the title [WIP]tools: replace GYP with GYP3 tools: replace GYP with GYP3 Mar 12, 2019
@refack refack removed the wip Issues and PRs that are still a work in progress. label Mar 12, 2019
@refack refack force-pushed the bump-gyp3-86ab71caad branch from e918e3f to 2b6fd3f Compare March 12, 2019 23:57
@refack refack mentioned this pull request Mar 13, 2019
Closed
3 tasks
This was referenced Mar 14, 2019
Closed
Closed
@richardlau
Copy link
Member 

* Lives in GitHub - https://github.com/refack/GYP.

Whatever happened to the plan to transfer into this org? nodejs/admin#247

@refack
Copy link
Contributor Author

refack commented Mar 14, 2019

Whatever happened to the plan to transfer into this org? nodejs/admin#247

That has been shelved for various $REASONS.

@targos
Copy link
Member

targos commented Mar 14, 2019

There are unrelated changes in the LICENSE file

@refack
Copy link
Contributor Author

refack commented Mar 14, 2019 •
edited
Loading

There are unrelated changes in the LICENSE file

That's what I got from re-generating 🤷‍♂️

BTW: for easy review, first commit is pure GYP vendoring. Second (ATM 2b6fd3f) is just changes in node file.

@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Mar 15, 2019
@cclauss cclauss mentioned this pull request Mar 15, 2019
Closed
63 tasks
ryzokuken
ryzokuken approved these changes Mar 16, 2019
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major +1.

@targos
Copy link
Member

targos commented Mar 16, 2019

That's what I got from re-generating 🤷‍♂️

I can't reproduce. If I run ./tools/license-builder.sh on master, LICENSE is unchanged.

richardlau
richardlau reviewed Mar 16, 2019
View reviewed changes
LICENSE Outdated Show resolved Hide resolved
@refack refack force-pushed the bump-gyp3-86ab71caad branch from 2b6fd3f to 49a1c5f Compare March 16, 2019 19:30
@refack
Copy link
Contributor Author

refack commented Mar 16, 2019

That's what I got from re-generating 🤷‍♂️

I can't reproduce. If I run ./tools/license-builder.sh on master, LICENSE is unchanged.

Issue resolved. LICENSE regenerated.

@refack

This comment has been minimized.

Sign in to view
targos
targos approved these changes Mar 16, 2019
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamp

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@refack refack mentioned this pull request Apr 13, 2019
Closed
@gengjiawen
Copy link
Member

@refack Need rebase.

@refack
Copy link
Contributor Author

refack commented Apr 16, 2019

Anyone want to help me figure out what I broke WRT cross-compilation? refack/GYP3@bd11dd1...d1f1343

@refack refack force-pushed the bump-gyp3-86ab71caad branch from 73fd43c to a669e35 Compare April 16, 2019 16:07
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@richard-townsend-arm
Copy link
Contributor

AIX builds seem to have the following error in their logs (e.g. https://ci.nodejs.org/job/node-test-commit-aix/22631/nodes=aix61-ppc64/console)
17:13:57 ccache: error: Could not find compiler "cc" in PATH

@richard-townsend-arm
Copy link
Contributor

ARMv7 builds seem to fail with this error:

../deps/v8/src/arm/assembler-arm.cc:177:2: error: #error "CAN_USE_ARMV7_INSTRUCTIONS should match CAN_USE_VFP3_INSTRUCTIONS" #error "CAN_USE_ARMV7_INSTRUCTIONS should match CAN_USE_VFP3_INSTRUCTIONS
(https://ci.nodejs.org/job/node-cross-compile/23120/nodes=cross-compiler-armv7-gcc-4.9.4/console)

@refack refack mentioned this pull request Apr 24, 2019
Closed
@Trott Trott removed this from the 12.0.0 milestone Apr 29, 2019
@refack refack mentioned this pull request May 2, 2019
Closed
1 task
@refack refack force-pushed the bump-gyp3-86ab71caad branch from a669e35 to aec2150 Compare May 30, 2019 23:15
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23503/

@cclauss cclauss mentioned this pull request Jun 5, 2019
Closed
@sinloss sinloss mentioned this pull request Jun 27, 2019
Closed
@Croydon Croydon mentioned this pull request Jul 2, 2019
Closed
Croydon added a commit to bincrafters/conan-gyp_installer that referenced this pull request Jul 2, 2019
@Croydon
Change to Refack's GYP fork
832fa65 
See

nodejs/node#26620

and

bincrafters/community#851

for further reasoning
@Croydon
Copy link

Croydon commented Jul 2, 2019

What is the status of this?

@sam-github sam-github mentioned this pull request Aug 14, 2019
Closed
@sam-github
Copy link
Contributor

FWIW, sam-github@c70af5c is another vendoring attempt I made that duplicates this, except its done against current master.

@richardlau richardlau mentioned this pull request Aug 14, 2019
Closed
4 tasks
@cclauss cclauss mentioned this pull request Sep 20, 2019
Closed
3 tasks
@cclauss
Copy link
Contributor

cclauss commented Nov 1, 2019

While I was a huge fan of this PR back in the day, I now believe that we can close it. Is there anything that we need to carry forward?

@sam-github
Copy link
Contributor

Closing because it isn't active, but not because it isn't worthwhile if anyone wants to take it up again.

@sam-github sam-github closed this Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzoz bzoz bzoz approved these changes

@richardlau richardlau richardlau left review comments

@cclauss cclauss cclauss approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@targos targos targos approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

Assignees

@refack refack

Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI. gyp Issues and PRs related to the GYP tool and .gyp build files meta Issues and PRs related to the general management of the project. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-vendor node/node-gyp --> tools/gyp/