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

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

Closed
cclauss opened this issue Jul 5, 2019 · 6 comments
Closed

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

cclauss opened this issue Jul 5, 2019 · 6 comments
Labels
python PRs and issues that require attention from people who are familiar with Python.

Comments

@cclauss
Copy link
Contributor

cclauss commented Jul 5, 2019

As discussed at #28537 (comment), in order to keep advancing towards Python 3 compatibility, we should re-vendor the repo node/node-gyp --> this repo's tools/gyp/ directory.

What is the best approach for vendoring in?

Is your feature request related to a problem? Please describe.
Please describe the problem you are trying to solve.
Node.js does not yet build on Python 3 and Python 2 EOL is in < 6 months.

Describe the solution you'd like
Please describe the desired behavior.
Node.js builds on Python 3.

Describe alternatives you've considered
Please describe alternative solutions or features you have considered.
Going kitesurfing.

@sam-github
Copy link
Contributor

I'm not involved in gyp maintenance, so no comment on whether we should do this (though python3 support is clearly something we want).

But in terms of how, look at 3a334b1 as an example, expecially of the commit message, and you can just rm -rf tools/gyp; copy new-gyp to tools/gyp; git add --all tools/gyp; git commit tools/gyp and PR it. I suggest the commit message have the shell commands you used be in it, so its clear its a mechanical task (I assume there will be lots of churn).

@nodejs/gyp

@richardlau
Copy link
Member

richardlau commented Jul 5, 2019

We were (are?) floating patches on top of gyp. I think these have been ported across to node-gyp (nodejs/node-gyp#1518) but it’s something we need to watch out for (e.g. nodejs/node-gyp#1661 had to address one case where the relative directory structure differs between node-gyp and here).

@richardlau
Copy link
Member

As discussed at #28537 (comment), in order to keep advancing towards Python 3 compatibility, we should re-vendor the repo node/node-gyp --> this repo's tools/gyp/ directory.

node-gyp !== gyp. We’d only want to vendor the gyp part of the node-gyp repository (in the absence of a more official maintained upstream gyp source).

@cclauss
Copy link
Contributor Author

cclauss commented Aug 13, 2019

This all seems to be done.

nodejs/node-gyp#1791 would still be a huge leap forward.

@cclauss cclauss closed this as completed Aug 13, 2019
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Aug 13, 2019
@sam-github sam-github reopened this Aug 14, 2019
@sam-github
Copy link
Contributor

@cclauss this doesn't seem to be done at all! Isn't the suggestion that we re-vendor "the gyp part of the node-gyp repository"? I don't think that has happened.

I just opened #28555

@sam-github
Copy link
Contributor

Perhaps we should be vendoring gyp3 in? #26620 would be an attempt at that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants