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

CI (Windows): Use npm (not yarn) to install ppm #185

Merged
merged 1 commit into from Dec 1, 2022

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Dec 1, 2022

Requirements for Contributing a Bug Fix (from template, click to expand):

Identify the Bug

ppm has its dependencies built for Node 14 in Windows CI (and CI artifacts). This makes ppm fail, because the bundled copy of Node in ppm is Node 16 now.

Those dependencies should be built for Node 16 now, after the recent bump to bundled Node 16 in the ppm repo, which was done for Apple Silicon compatibility reasons.

Details of the error message (click to expand):

This PR should fix this particular occurrence of the semi-infamous NODE_MODULE_VERSION error -- which indicates when you're trying to run native C/C++ code compiled against the wrong version of Node/Electron -- a different version than you're currently running.

Error: The module '\\?\C:\Users\user\AppData\Local\Programs\pulsar\resources\app\ppm\node_modules\git-utils\build\Release\git.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 83. This version of Node.js requires
NODE_MODULE_VERSION 93. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or `npm install`).

This is saying the module was built for Node 14, but you're attempting to run it with Node 16.

(To decode the meaning of the different NODE_ABI_VERSION numbers, refer to this metadata file: https://github.com/electron/node-abi/blob/main/abi_registry.json. ABI 83 is Node 14, ABI 93 is Node 16.)

Description of the Change

Use npm, not Yarn, to install ppm's dependencies.

(Fixes an obscure thing where Yarn exits the postinstall script early, earlier than NPM does.)

Speculating on why this happens in Yarn but not NPM? (Click to expand):

(Probably has something to do with Yarn handling process.exit() differently than NPM does, in ppm's postinstall.cmd. Specifically, ppm's script/download-node.js has a bit where it calls process.exit(), which I assume is handled differently when run under Yarn compared to running it under NPM? Since that's right where postinstall.js exits "early" under Yarn. Npm keeps going till the later part of postinstall.cmd.)

(Perhaps Yarn avoids running sub-processes of Node nested as deeply as NPM does? Thus process.exit() exiting a higher Node process that we expect to still have work to do later in the main postinstall script? Not totally sure. This is just a guess.)

Alternate Designs

N/A. I didn't consider any alternatives, running it under Yarn didn't work for me locally, but running it under NPM did work for me locally.

Possible Drawbacks

None expected.

The ppm (previously apm) repo was traditionally developed with NPM, and I don't think it has any particular thing that necessitates the use of Yarn, unlike the workarounds Yarn was able to provide for installing the main Pulsar dependencies.

Verification Process

Worked for me locally, hopefully CI produces an artifact with working ppm.

Update:

> pulsar-package-manager@2.7.0 postinstall C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\ppm
> node script/postinstall.js
>> Downloading bundled Node
>> Rebuilding apm dependencies with bundled Node v16.0.0 x64

Noice. (The "Rebuilding apm dependencies with bundled Node" part is working with NPM now in Cirrus CI Windows.)

Release Notes

Fixed ppm (package manager) in Windows CI builds

Should fix an obscure issue where ppm's postinstall script exits out
half-way through without resuming under Yarn, but completes fully
under NPM.

Probably has something to do with Yarn handling process.exit()
differently than NPM does, in the postinstall script/its sub-scripts.

(Perhaps Yarn avoids running sub-processes of Node nested as deeply
as NPM does? Thus process.exit() exiting a higher Node process that we
expect to still have work to do later in the main postinstall script?
Not totally sure. This is just a guess.)
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

This looks fantastic to me, and hopefully can resolve issues for many of the Windows users specifically.

Otherwise thanks for getting this one figured out, I know it was kinda a rabbit hole to dive into.

@mauricioszabo mauricioszabo merged commit f4dfa04 into master Dec 1, 2022
@mauricioszabo mauricioszabo deleted the fix-ppm-postinstall-in-Windows-CI branch December 1, 2022 19:33
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.

None yet

3 participants