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

npm update broke node-addon-api tests on AIX #44548

Closed
mhdawson opened this issue Sep 6, 2022 · 6 comments
Closed

npm update broke node-addon-api tests on AIX #44548

mhdawson opened this issue Sep 6, 2022 · 6 comments

Comments

@mhdawson
Copy link
Member

mhdawson commented Sep 6, 2022

Version

v18.8.0

Platform

AIX

Subsystem

No response

What steps will reproduce the bug?

npm install pre-commit

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior?

package is installed

What do you see instead?

npm ERR! code 127
npm ERR! path /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/aix72-ppc64/node_modules/pre-commit
npm ERR! command failed
npm ERR! command sh -c -- node install.js
npm ERR! node install.js: --: not found

npm ERR! A complete log of this run can be found in:
npm ERR! /home/iojs/build/workspace/citgm-smoker/npm_cache/_logs/2022-09-06T21_20_27_438Z-debug-0.log
iojs@nodejs04:[/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/aix72-ppc64]node-v18.8.0-aix-ppc64/bin/node node-v18.7.0-aix-ppc64/lib/node_modules/npm/bin/npm-cli.js install pre-commit

Additional information

This can out of looking at node-addon-api failures on AIX - like this - https://ci.nodejs.org/job/node-test-node-addon-api-new/nodes=aix72-ppc64/6348/console

pre-commit is one of the packages in the package.json for node-addon-api

@mhdawson mhdawson changed the title npm update broke node-addon-api tests npm update broke node-addon-api tests on AIX Sep 6, 2022
@mhdawson
Copy link
Member Author

mhdawson commented Sep 6, 2022

From some investigation, it looks to be the update to npm 8.18.0. Using that npm version with an older/newer version of Node.js results in the error. Using 8.15.0 (which was in the previous Node.js version) with a newer or older version of Node.js does not result in the error.

@mhdawson
Copy link
Member Author

mhdawson commented Sep 6, 2022

Looking at what is new in that version of npm I see - npm/cli#5297 which seems to be the cause.

Reverting that allows the install to work.

@mhdawson
Copy link
Member Author

mhdawson commented Sep 6, 2022

Setting npm config set script-shell "bash" seems to allow the install to succeed.

@mhdawson
Copy link
Member Author

mhdawson commented Sep 6, 2022

@lux01 can you comment on whether the behaviour we are seeing is what you expected based on the fix for npm/cli#5297 ?

The tests used to pass I believe because the node-addon-api job set the shell to bash before doing the install. npm/cli#5297 seems to have caused npm to ingore the current shell by default.

I've fixed the node-addon-api test job by setting the script-shell as part of the job, but not sure if that the new behaviour is as expected.

@lux01
Copy link
Contributor

lux01 commented Sep 27, 2022

@mhdawson Sorry my GitHub notifications don't seem to be working for some reason.

The setting npm config set script-shell "bash" combined with npm/cli#5297 is the work around for a regression introduced by npm/run-script#98. This latter PR implicitly made npm require a bash compatible shell (-- is not POSIX compatible) and so has broken a number of less common platforms.

As it stands npm@8.17.0 just doesn't really work on AIX, IBM i, FreeBSD, and perhaps many others. npm@8.18.0 and above work provided that a bash compatible shell has been installed and has been set as the script-shell thanks to npm/cli#5297.

See npm/run-script#103 and npm/cli#5489.

@mhdawson
Copy link
Member Author

Ok, so onced we get an update to 8.18.0 things will be resolved and even after then the work around I put in place for the node-addon-api test will continue to work so I think we can close this. @lux01 thanks for the info.

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

No branches or pull requests

2 participants