Upgrade node-gyp-build to support python 3.11+ users

csydvs commented 1 year ago

Software versions

Please provide at least OS and version of pact-js

OS: Mac OSX 12.6.1
Consumer Pact library: Pact JS@Latest

Issue Checklist

Please confirm the following:

[*] I have upgraded to the latest
[*] I have the read the FAQs in the Readme
[*] I have triple checked, that there are no unhandled promises in my code and have read the section on intermittent test failures
[*] I have set my log level to debug and attached a log file showing the complete request/response cycle

Expected behaviour

Just trying to run npm i -S @pact-foundation/pact@latest on a box with python 3.11.2 installed, would like it to install

Actual behaviour

Error encountered. See this gyp issue for more info on what's going on here.

Relevant log files



> @pact-foundation/pact-core@13.13.8 install /Users/casey/Workspace/service/graphql/node_modules/@pact-foundation/pact-core
> node-gyp rebuild

Traceback (most recent call last):
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/gyp_main.py", line 50, in <module>
    sys.exit(gyp.script_main())
             ^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 554, in script_main
    return main(sys.argv[1:])
           ^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
           ^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 520, in gyp_main
    [generator, flat_list, targets, data] = Load(
                                            ^^^^^
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 136, in Load
    result = gyp.input.Load(build_files, default_variables, includes[:],
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 2782, in Load
    LoadTargetBuildFile(build_file, data, aux_data,
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 391, in LoadTargetBuildFile
    build_file_data = LoadOneBuildFile(build_file_path, data, aux_data,
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 234, in LoadOneBuildFile
    build_file_contents = open(build_file_path, 'rU').read()
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid mode: 'rU' while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:351:16)
gyp ERR! stack     at ChildProcess.emit (events.js:400:28)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:285:12)
gyp ERR! System Darwin 21.6.0
gyp ERR! command "/usr/local/bin/node" "/opt/homebrew/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd ...
gyp ERR! node -v v14.21.3
gyp ERR! node-gyp -v v5.1.1
gyp ERR! not ok ```

YOU54F commented 1 year ago

hmm

node-gyp suggest npm install -g node-gyp on the end user side.

We have some troubleshooting notes here that warrant updating :)

edit: forgot to add link 😅

csydvs commented 1 year ago

The global node-gyp install didn't work for me, but another good option for anyone who runs into this is to run npm config set python <different version of python> to downgrade the version of python that node uses to something lower that you have locally.

YOU54F commented 1 year ago

Thanks for the update,

I saw the same issue running node 15 in a test earlier today

https://github.com/pact-foundation/pact-5-minute-getting-started-guide/actions/runs/4861112527/jobs/8665756390#step:6:81

This line fixed it

https://github.com/pact-foundation/pact-5-minute-getting-started-guide/commit/0bffc8bd1fb4f23b7b47f42ba9292316473a6397#diff-014228303dff9a1af15f4bbd18401f906380129b10ae2a2c62f8b8be592ff88eR37

taken from node-gyp's guide

https://github.com/nodejs/node-gyp/blob/main/docs/Updating-npm-bundled-node-gyp.md#linux-macos-solaris-etc

passing run

https://github.com/pact-foundation/pact-5-minute-getting-started-guide/actions/runs/4861221627/jobs/8666000861

eugenk commented 1 year ago

node-gyp suggest npm install -g node-gyp on the end user side.

This cannot be the solution, though. A simple npm i on every machine that has Python 3 installed (no matter if 3.10 or 3.11) should just work.

This line fixed it

The same goes for the pipelines. You should not be required to take additional steps to just install the pact package.

Couldn't the node-gyp (current version) be a devDependency of pact-core that is then used for the postinstall step?

Out of curiosity: Why are you (transitively) depending on Python in a node project in the first place? This seems a little odd.

YOU54F commented 1 year ago

It's not the only javascript project to do so

see electrons guidance

https://www.electronjs.org/docs/latest/tutorial/using-native-node-modules

See this issue for https://github.com/pact-foundation/pact-js/issues/899 for tracking.

I agree its suboptimal

Happy for someone to propose a PR with the suggested change

YOU54F commented 1 year ago

The same goes for the pipelines. You should not be required to take additional steps to just install the pact package.

I don't agree, you may have to take additional steps, dependant on the package and how its built, that is just life, however they should be very well documented, so as to cause minimal frustration to end users.

YOU54F commented 1 year ago

Closing this issue as invalid

node-gyp-build isn't included in the package.json for pact-js and its core pact-js-core

https://github.com/pact-foundation/pact-js/blob/master/package.json https://github.com/pact-foundation/pact-js-core/blob/master/package.json

JVKeller commented 10 months ago

If anyone stumbles here again...

Change the line with the error from rU to r, and run 'nmp rebuild'

YOU54F commented 10 months ago

thanks, they should just install the latest version 12.x or later as that doesn't require this build step at all 👍🏾

pact-foundation / pact-js