spec: Fixtures Node v10.20.1 --> Electron v12.2.3

DeeDeeG commented 1 year ago

Warning

I went and wrote too much stuff again, please consider just reading the commit message and the diff, it's a lot easier to read than all this.

Summary

Run certain specs having to do with building native C/C++ code addons against Electron v12.2.3 (matching Pulsar's Electron version), Not against some arbitrary Node version.

Bonus: The file sizes are much, much smaller!!!

Also:

Stop tracking/serving the "node-v[version].tar.gz" file, only bother with the "node-v[version]-headers.tar.gz" file, since it's the only one of the two node-gyp requests anyway.
- See history notes below if you'd like to know more about this.
Fix the paths on Windows that we serve the "node.lib" files at, on Windows. Again, for compat with modern node-gyp.

(Alternate implementation of PR https://github.com/pulsar-edit/ppm/pull/42.)

What is this good for, anyway?

(IN SHORT: Works around https://github.com/nodejs/node-gyp/issues/1457.)

This is needed to fix the specs, since old `node-gyp` (currently node-gyp v5.x) running on newer NodeJS (currently the bundled NodeJS v16.0.0) will break when trying to build anything against the NodeJS 10.x headers. (NodeJS v10 headers are among the spec fixture files this PR updates). Updating the fixtures + specs to build against Electron v12 headers both _fixes the specs_ and also matches the Electron version used in Pulsar. So that's what I did here.

Notes, context and trivia

Note: Something similar was last needed in https://github.com/atom/apm/pull/887.

History notes on why we used to need the full source tarball, but now only need the headers tarball (click to expand):

The headers-only tarball was introduced with NodeJS [v0.12.8](https://nodejs.org/dist/v0.12.8/) and [v0.10.41](https://nodejs.org/dist/v0.10.41/), and node-gyp moved to it for those versions and newer as of [node-gyp v3.3.0](https://github.com/nodejs/node-gyp/blob/v3.3.0/CHANGELOG.md). This all happened back in late 2015/early 2016. (We are on node-gyp 5 now in ppm.) These spec fixture files were initially [committed in apm in 2013 as a NodeJS v0.10.3 tarball](https://github.com/atom/apm/commit/a087bf041938fc578a3ca0eca03f41ebb2bd534a#diff-3a09d6e7758452e1063aa12044f4b61345ac26e081358143ec4206a68a508199), before the headers-only tarballs were available. As explained above, this is obsolete and we only need the headers tarball. The headers tarball in the repo was eventually added... by me... in the year 2020, in https://github.com/atom/apm/commit/77227d829386dfb26d83fe36fc306c769d812c80 as part of a PR to update the fixture files to NodeJS v10.20.1. Which was done to fix a similar issue as we have to solve now by updating the fixture files again... such as in this present PR.

DeeDeeG commented 1 year ago

UPDATE Jan 23rd 2023: PLEASE ignore this, I will only consider this stuff in follow-up PRs, if at all. I want to merge the basic essential bit (this PR as-is) ASAP.

Original comment below:

Notes for reviewers

This PR (as opposed to https://github.com/pulsar-edit/ppm/pull/42) means the specs can happen without relying on the internet.

https://github.com/pulsar-edit/ppm/pull/42 does work as written, but it requires network downloads from electronjs.org to succeed for our specs to pass (at least the first time they're run on a given machine), which I'd like to avoid given how small these files turned out to be.

Optional bits of https://github.com/pulsar-edit/ppm/pull/42 to consider for inclusion here:

(If reviewers have time to consider these, otherwise this PR works as-posted and I can break these items out into follow-up PRs at some point, or just leave it be since we may never have to update these fixtures again, honestly.):

https://github.com/DeeDeeG/ppm/commit/d73b0b067cbb835d147d31debc499c9c272f9321
- Moves hard-coded "v12.2.3" repeated multiple times throughout six spec files into one value in a JSON file. Specs can just require() the value from the JSON one time instead of repeatedly hard-coding it. Check it out, I think the diff/commit message are pretty self-explanatory
- I personally like this, and if reviewers want it I will include it.
- Only really improves things if this hypothetically ever needs to be updated again, which it obviously doesn't need to be updated very frequently if this is only the second time in like 10 years anyone has needed to update it.
https://github.com/pulsar-edit/ppm/blob/662f7d52bd322bbe4f6b5927533e20f01c113127/script/ensure-node-source-fixture-files.js
- My script from PR 42.
- This script can make the instructions to update the fixtures as simple as "update nodeVersion in spec/config.json and run script/update-node-source-fixture-files.js."
- Would need changes to be optimal for updating rather than validating the current expected binaries. But can basically do that job as-is and be refined later, I guess.
- Again, I dunno if adding a new convenience script for this is good or not, will take reviewer feedback. Left it out by default to make this PR bare-bones and simple.

DeeDeeG commented 1 year ago

@pulsar-edit/core can I get a review for this PR? It makes tests pass here, and I kind of need it for that reason.

It also would decrease Pulsar's installed-to-disk size by ~40 to 44 MB, so there's that, too!

(To me this is a super important thing to merge, and it's a minimal self-contained fix, only affects 9 tests in this one repo (does not affect Pulsar per se/in production, only relevant to the tests themselves) and it's been available to review for a while, so at some point I will merge it myself even if no reviews.)

Explanation of the PR below:

Okay so I keep being too verbose.

This changes the headers.tar.gz file and a couple .lib files that go along with it for Windows, that are used to build some dummy C/C++ packages during specs.

Bumping them to something newer makes tests pass again.

Why did this have to change the spec files?

_(Of course, we have to change the path to the files in the specs, because the paths/filenames are fully hard-coded in the specs currently, and include the version number and everything. No two ways around it, this is the minimum change I can do, basically. So that's why I couldn't land this without changing the spec files, albeit in a super straight-forward way that should be trivially easy to port over to decaf, IMO.)_

Nerdy details note: This is a workaround for https://github.com/nodejs/node-gyp/issues/1457.

Thanks for taking a look, if you have any objections or comments please post them, otherwise I will be merging this soon.

- DeeDeeG

Spiker985 commented 1 year ago

@DeeDeeG Question: does it make sense to parameterize this, so that when we bump in the future, we don't have to update another 24 lines? (Or is that included in your #42 PR?)

DeeDeeG commented 1 year ago

@Spiker985 I have a commit ready to go: https://github.com/DeeDeeG/ppm/commit/d73b0b067cbb835d147d31debc499c9c272f9321

If you (or anybody) would approve that change, then I'll pop that commit it into this PR right away.

(Note: I was just worried about bogging this PR down with optional changes that make this harder to review. See the hidden comment above for all the things I took out that could be re-added here. You can't see it I'll un-hide it.)

Spiker985 commented 1 year ago

Yeah, I'd honestly add that to this commit so that we don't have to touch it again (for this specific problem/fix)

And then I'll approve it

Because having the hardcoded fixture pass all the tests, followed up with a parameterized version passing all the tests would be a glowing PR to me

DeeDeeG commented 1 year ago

@Spiker985 I am tempted to move these files to a tidier arrangement, assuming I can get an approve for that too?

Before:

fixtures/
  |- SHASUMS256.txt
  |- node-v12.2.3-headers.tar.gz
  |- node.lib
  |- node_x64.lib
  |- ...
  |- tons of other, unrelated fixture files loose in this folder

After:

fixtures/
  |- node-headers/
    |- SHASUMS256.txt
    |- node-v12.2.3-headers.tar.gz
    |- win-x86/
      |- node.lib
    |- win-x64/
      |- node.lib
  |- ...
  |- Other unrelated fixture files

Better to do this once than to skip it now and do a follow-up, as that would mean two PRs to track for porting to the decaf effort. If you (or anybody) would approve that I'll do it, otherwise the existing paths are okay (just plopped directly under spec/fixtures/) and I'll go with those from here on out.

Spiker985 commented 1 year ago

If you do it in separate commits and they both pass spec, I don't see why not

Do one, keeping the hard code but moving the headers, and then a follow-up with parameterization

DeeDeeG commented 1 year ago

Okay, this is ready to review again!

DeeDeeG commented 1 year ago

Thanks for the approve, merging!

Let's get those tests passing again!

And drop that installed-to-disk Pulsar app bundle size actually around ~57 MB if I just measured correctly?

Before:

$ ncdu /opt/Pulsar/resources/app/ppm/spec/fixtures
[ . . . ]
Total disk usage:  59.7 MiB  Apparent size:  59.2 MiB  Items: 184

After?:

$ ncdu ~/ppm/spec/fixtures
[ . . . ]
Total disk usage:   2.5 MiB  Apparent size:   2.1 MiB  Items: 191

(The two node.lib files are smaller too, not just the headers.tar.gz and getting rid of the not needed full NodeJS source .tar.gz. Not sure why 7 additional items, maybe some are already filtered out in our electron-builder file exclusions, IDK. Speaking of which, that's a sensible follow-up, to totally exclude the specs dir of ppm from being bundled with Pulsar.)

pulsar-edit / ppm