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

feat(gyp): update gyp to v0.16.1 #2909

Closed
wants to merge 11 commits into from

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Oct 4, 2023

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

@rzhao271
Copy link
Contributor Author

rzhao271 commented Oct 5, 2023

@cclauss one issue I have with this PR is that the Python portion of node-gyp actually requires a package now. I'm wondering whether we could change the install script to set up the Python package for the user, or will users have to install the required Python packages themselves?

I'll also have to take a look at the VS tests tomorrow.

@cclauss
Copy link
Contributor

cclauss commented Oct 5, 2023

the Python portion of node-gyp actually requires a package now

We vendor in this code into the node-gyp repo so that users do need to pip install it. We post gyp-next to PyPI but statistics show that this is not how most users access it. I do not see how anything in this release changes that vendoring process.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Oct 5, 2023

Do the 2016 and 2019 Visual Studio tests still make sense?
When I expand the Environment Information tab, the only IDE listed is VS 2022.

The failing tests also emit the following output:

gyp verb find VS msvs_version not set from command line or npm config
+ gyp verb find VS VCINSTALLDIR not set, not running in VS Command Prompt
+ gyp verb find VS checking VS2022 (17.7.34031.279) found at:
+ gyp verb find VS "C:\Program Files\Microsoft Visual Studio\2022\Enterprise"
+ gyp verb find VS - found "Visual Studio C++ core features"
+ gyp verb find VS - found VC++ toolset: v143
+ gyp verb find VS - found Windows SDK: 10.0.22621.0
+ gyp info find VS using VS2022 (17.7.34031.279) found at:
+ gyp info find VS "C:\Program Files\Microsoft Visual Studio\2022\Enterprise"

Edit: when node-gyp actually picks up the msvs-version it gives the following error

Message: Could not find any Visual Studio installation to use

So I think the 2016 and 2019 tests can be removed.

@cclauss
Copy link
Contributor

cclauss commented Oct 5, 2023

You are saying that this environment variable trick no longer works?

echo 'GYP_MSVS_VERSION=${{ matrix.msvs-version }}' >> $Env:GITHUB_ENV

@rzhao271
Copy link
Contributor Author

rzhao271 commented Oct 5, 2023

I think that the trick works, but that node-gyp doesn't listen to it.
When I pass in the variable so that node-gyp listens to it, it fails the test because it couldn't find VS 2016 or 2019. I changed the visual-studio test matrix a bit so that it tries running the pairs (VS 2019, msvs-version 2019), (VS 2022, msvs-version 2022).

@cclauss
Copy link
Contributor

cclauss commented Oct 5, 2023

% npm config list -l

npm recently got rid of lots of config options including msvs-version

@cclauss
Copy link
Contributor

cclauss commented Oct 5, 2023

Does setting the environment variables GYP_MSVS_VERSION or VCINSTALLDIR work locally?

@rzhao271
Copy link
Contributor Author

rzhao271 commented Oct 6, 2023

VCINSTALLDIR works locally
GYP_MSVS_VERSION does not

Edit: It turns out the Python tests still need the GYP environment variables.

@rzhao271 rzhao271 marked this pull request as ready for review October 6, 2023 21:41
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 16, 2023

Reviewing what was said about requiring external packages, vs vendoring (click to expand):

one issue I have with this PR is that the Python portion of node-gyp actually requires a package now.

This refers to the packaging (https://pypi.org/project/packaging/) dependency, right?

We vendor in this code into the node-gyp repo so that users do need to pip install it.

This refers to gyp-next itself, right?

...

Just to clarify, there is a new requirement (a Python package called packaging) that is needed to run this, and which users won't have on their system by default?

I don't think gyp-next has vendored in the packaging dependency... So, this updated gyp-next wouldn't run out-of-the-box on a system with just Python, right? Is it an option to vendor in a copy of packaging into gyp-next repo? That would solve the problem, if I've understood the problem correctly in the first place.

How to vendor a python dependency, per StackOverflow: https://stackoverflow.com/a/65470764

@cclauss
Copy link
Contributor

cclauss commented Oct 16, 2023

Is packaging a runtime requirement or just a test-time requirement?

@rzhao271
Copy link
Contributor Author

@cclauss packaging is now a runtime requirement for node-gyp, but I have no experience with vendoring. Would it make sense for the repository to include a copy of the packaging package's source?

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 25, 2023

I think this PR is obsolete and can be superseded by a bump directly to gyp-next 0.16.0.

If someone who actually worked hard on these changes in gyp-next wants to open a PR bumping straight to gyp-next 0.16.0, I'll let you do that. Since it will show the commits here with the author as whoever performs the gyp-next bump.

Otherwise, I would want to open such a PR bumping straight to gyp-next 0.16.0 myself soon.

Thanks for the hard work on this, folks! It will be both exciting and a relief to have node-gyp working on Python 3.12.

EDIT: On closer look, there's more work here than just a gyp-next bump, the CI workflow files are reworked a bit. So, maybe just bump gyp-next again on this branch and update the PR title to match?

@rzhao271
Copy link
Contributor Author

@DeeDeeG go ahead with the bump to v0.16.0
There is a script you can run in this repository to help create the commit
After that, you can copy over the CI changes from this commit.

@cclauss
Copy link
Contributor

cclauss commented Oct 27, 2023

Where are we at on this? Let's try to get Py3.12 compatibility into @lukekarrys imminent releases. #2860 (comment)

@lukekarrys
Copy link
Member

There will be an automated release PR for v10.0.0 where we can discuss waiting for any open PRs to land first. I won't release v10 without waiting a few days to make sure we get everything in.

@lukekarrys lukekarrys added this to the v10.0.0 milestone Oct 27, 2023
@rzhao271 rzhao271 changed the title feat(gyp): update gyp to v0.15.1 feat(gyp): update gyp to v0.16.1 Oct 27, 2023
@DeeDeeG DeeDeeG mentioned this pull request Oct 27, 2023
5 tasks
@rzhao271
Copy link
Contributor Author

rzhao271 commented Oct 27, 2023

I forgot to update my status.
I was busy with work yesterday but had some time today, so ended up updating my PR two minutes before @DeeDeeG created the same bump. Sorry.

@rzhao271
Copy link
Contributor Author

Closing in favour of #2923 because it has a better description

@rzhao271 rzhao271 closed this Oct 27, 2023
@rzhao271 rzhao271 deleted the rzhao271/upgrade-gyp-15-1 branch October 27, 2023 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants