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

Update npm and node-gyp, for macOS signing fix #94

Merged
merged 4 commits into from
Sep 17, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Sep 15, 2023

Reason for bumping node-gyp again (purpose of this PR)

There is an issue with code-signing apps built with node-gyp 9.1.0 or newer. (This is due to unforeseen consequences of a change I authored in node-gyp, unfortunately. The change in question: nodejs/node-gyp@b9ddcd5.)

So, this PR bumps node-gyp to the latest commit from node-gyp repo's main branch, currently nodejs/node-gyp@d3615c6.

This includes a fix for better compatibility with app signing on macOS (nodejs/node-gyp@0f1f667).

Said commit with the fix isn't a part of any tagged or published-to-npm-package-registry releases yet, so we have to grab it from the node-gyp repo, rather than, say, as a version from the npm registry like we usually would.

How to do this (how to bump node-gyp in our fork of npm --> how to bump node-gyp in ppm:

  • Clone Pulsar's forks of npm-lifecycle, libnpm, libcipm, and npm (this last one has a custom repo name npm-cli for our fork. Because the upstream cli repo name is rather ambiguous otherwise.)
  • Update the version of node-gyp in npm-lifecycle's package.json. (Recommended to bump the version number of npm-lifecycle as well, to make it easier to track changes.) Publish these changes to Pulsar's fork of npm-lifecycle.
  • Update the version of npm-lifecycle in libnpm and libcipm's respective package.jsons. (Bump versions of both packages.) Publish these changes to Pulsar's fork of each respective package.
  • Update the versions of npm-lifecycle, libnpm, libcipm and node-gyp in npm's package.json. (Bump npm's version.) Publish these changes to Pulsar's fork of npm package.
    • Be careful about checking in changes to tracked files under node_modules. Unlike most NodeJS packages, the node_modules dir has some of its content checked in and changes tracked in git. This is so, when you clone the npm repo, it has a functional basic set of dependencies already, and can bootstrap itself.
      • Rule of thumb one: If after running npx npm@6 install --ignore-scripts, the files on-disk for any of the packages under node_modules have changed, AND only if git status shows any of the package's files as modified, rather than all of them being new file or all of the package's files being listed under Untracked files:, then you should check in all the changes to files directly belonging to said package, but not any files under said package's own nested node_modules subfolder, unless those are already checked in and tracked as well.
        (Put more concisely: If a package has modified files under git status, update its checked-in files. Stage (git add) them and commit (git commit) them. And don't git add brand-new sub-dependencies that aren't already tracked in git -- ones that don't themselves show as modified in git status.
      • Rule of thumb number two: avoid checking in brand new files under node_modules, unless they are genuinely part of the changed files in that package. Can confirm by checking the changed files at that package's git repo between the version you started with and the version you bumped to. Or alternatively, you can confirm by checking that the files in question are there if you download the package version in question with npm pack [package_name]@[version], extracting the resulting tarball and inspecting the files -- you want to see that they match what you're going to commit, before committing them in npm repo. Add new files only for a package that was tracked in git at the npm repo before you started.
      • You can confirm/test that this all worked properly, if you can cleanly clone your changes and run node . install in the repo without errors, and have a large amount of the tests pass if you run node . test in the repo. I'm not aware of a way to avoid 100% of errors in the test suite, however. So, if most tests pass, that's what you're looking for.
  • Once verified that the updated npm package is in good shape, run npm pack to package the npm package properly as a tarball. Publish this tarball as a release asset on Pulsar's fork of npm. (Without this, npm's sub-dependencies install wrong and are broken when you try to install it as a dependency of ppm).
  • Finally, clone ppm repo, and bump npm and node-gyp in ppm's package.json. Run yarn install and update the yarn.lock lockfile as well. Publish the branch where you made these changes, and post a pull request.

Note: highly recommended to use npm 6 above, whenever running npm commands, since all the lockfiles for these packages are meant to be lockfile v1 format, which is the npm 6-era lockfile format.

Context/tech debt notes

This is how we get newer node-gyp in ppm at the moment. Not easy, but this is the cost of tech debt and being indebted to npm 6-era technology (especially the "require('npm')", AKA "use npm all at once as a single library" thing that got significantly changed in npm 7 and then dropped altogether in npm 8).

(It's also the consequence of npm tending to require node-gyp all over the place in multiple, roundabout ways, given the way they split up slightly related modules that handle all this stuff on the npm side of things.)

Ideally, from a technical implementation stand-point, I feel we should find some way to use the underlying libraries that make up npm as and where we need them. This has been discussed a bit on the Discord. Easier said than done.

For now, we have a fork on npm, one that hopefully we don't need to update all that often. (And especially we hopefully don't need to bump node-gyp very often, as it is one of the more painful/complicated dependencies to bump. For above-mentioned reasons.)

Verification process

  • After cloning this branch, yarn install runs cleanly without updating yarn.lock, indicating dependencies are stable.
  • Running yarn check is mostly without errors.
    • (I get two errors about two sub-dependencies of npm/node_modules/tar allegedly being the wrong minor versions, but when I check on the version actually installed, they are the correct versions yarn check said it wanted to see??!?!).
      • error "npm#tar#minipass" is wrong version: expected "2.9.0", got "2.3.3" <-- I checked, 2.9.0 is installed on my machine???
      • error "npm#tar#yallist" is wrong version: expected "3.1.1", got "3.0.3" <-- I checked, 3.1.1 is installed on my machine???
    • (There are plenty of warnings of things we could dedupe, but deduping is kind of harder in Yarn than it is in npm, as far as I could ever tell. Maybe I missed the docs where there is any way to do so automatically. I suppose there is yarn upgrade and yarn upgrade-interactive... Not as urgent as having working dependencies, IMO. But could save some disk space and install time, ToDo later I guess, maybe, but low low priority.)
  • Tests pass locally and in CI.

This is just the devDependency, bumping npm next to fully do this.

Includes a fix for better compatibility with code-signing on macOS.
(nodejs/node-gyp@0f1f667).
Includes a fix for better compatibility with code-signing on macOS.
(nodejs/node-gyp@0f1f667).
We mostly aren't using this, but while it's here
it may as well be up-to-date.
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.

While there's a bit within your PR body I'd like to comment now, those thoughts will likely need to wait a day. In the meantime, this PR looks great, seems to do exactly as suggested, tests pass, and I know you put quite a bit of effort in to make sure this works as expected.

With that said, lets get this merged, so that we can hopefully include it within this soon coming PPM bump within Pulsar, as we have our regular release tomorrow.

So great work on this, and glad to see it come together!

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 17, 2023

Curious what those other thoughts are, I'd guess it's not rosy feedback about the tech debt and difficulty of updating these things, to which I'd have to agree...

Anyway, merging as it is, since the npm fork is already part of ppm master branch, this is just tweaking the node-gyp dependency again, through all that process it requires.

Thank you for the approve, though I understand the reservations about what lies under the hood of this, the npm fork brings blessings and curses, one might say. It is by no means a "perfect solution", it is full of compromises. So if anyone is able to bring a clean and easy alternative, I will be glad to look over it and try to adopt it if we can.

@DeeDeeG DeeDeeG merged commit 93b786c into master Sep 17, 2023
11 checks passed
@DeeDeeG DeeDeeG deleted the update-npm-and-node-gyp-for-macOS-signing-fix branch December 13, 2023 06:18
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.

2 participants