Newer NodeJS Source Files for Specs

[DeeDeeG]

As per usual, I'm writing really long stuff in these PR bodies.

The short story is, this fixes the specs.

See the diff and commit messages for details. (Although I wrote some really long commit messages, too. Whoops.)

There is the summary below, which was meant to be the condensed info, but ended up being long again.

I also bolded the bare minimum info in the paragraphs below, so one can skim to know what this PR is about/why.

---

Summary:

• I bumped the version of the NodeJS source/headers used by a few specs.
  • v10.20.1 --> v18.12.1
  • This is the change that actually fixes the tests.
• And to do so, I added a new script (script/ensure-node-source-fixture-files.js) to fetch the files from NodeJS.org on the web as-needed, (or verify the local copies if you already fetched them once) rather than check them into the repo.
  • (Newer NodeJS versions have bigger file sizes. They're getting too big as more and more features get added. Over 70MB! Not ideal for a git repo to keep adding files that big over time, if you don't absolutely need to, and GitHub warns if you upload anything over 50MB.)
• Added the save destination dir for new updated NodeJS source files to .gitignore, since the whole point is we do not want these huge ~70MB+ files checked into git under any circumstances. They are to be fetched from the web as-needed only.
• Updated the npm run test script to run the new ensure-fixtures script (which I wrote for this PR) before running the specs, so the files will (or at least should) exist locally on disk, and the specs that need these files can pass.
• Updated the specs to look for these files in the new location
• Added caching for these new files in CI, so these files should still be fairly reliable in CI even if NodeJS.org flakes or goes down, despite them being fetched on-demand now from the web rather than checked in to the git repo.
  • Also makes CI faster by, surprisingly, only ~2 seconds??? And saves NodeJS.org their precious bandwidth from having to serve these files over and over again for our CI.

---

Hi folks.

When bumping to bundled NodeJS to v16.0.0 (#41 --> 12ae15d), it turned out ppm stopped being able to pass certain specs.

On closer look, these specs failing in this particular way seemed very familiar to me, reminding me of the time I had to bump the copy of NodeJS's source and headers that we use to build some basic example/test modules against in certain tests. I had to do this to get specs passing again during the upgrade to node-gyp v5.X (atom/apm#887 --> atom/apm@77227d8)

However, the same solution again (bumping to newer NodeJS source/headers for the specs) would baloon this git repo's size by just over 100 megabytes. (Newer NodeJS versions keep getting larger and larger and larger as they continue adding new features to NodeJS core.)

So I put together a script to download these large files off the web from NodeJS.org as-needed and just keep them around for subsequent test runs, since they won't be changing often. I then made sure this script is run (and the source/header files verified or re-downloaded as needed) before each test run -- I added it to the steps that get run when you do ./bin/npm run test. I also reconfigured some spec files to expect these files to be in the new spec/fixtures/node-source dir where these files will live. And I .gitignored the new files, since they are not meant to be checked in to git. And I added a step to the CI workflow to cache these files, so we can somewhat reliably have these files, even if NodeJS.org flakes or goes down. This also saves NodeJS.org some bandwidth, and makes the tests run ever so slightly faster. (Although it's only faster by a second or two! None of this file downloading/verifying takes very long at all, nor does the caching in CI! I am impressed that it only takes about 5 seconds or less either way. Usually less than 5 seconds!)

In other words, I fixed the specs, and made it easier to change the version of NodeJS these test/example modules build against during the specs, and did so without causing this git repo to become really super huge to download.

(Most of this repo's download size is from the NodeJS 10.20.1 source/headers I added in atom/apm#887! I have always regretted that fact after I realized there might have been a better way to do it. So here is my shot to redeem the situation!)

(Historical note: NodeJS used to be so small that checking its entire source into git was a fairly reasonable thing to do. For version v0.10.3, all NodeJS files needed for these specs were under 1 megabyte! For NodeJS v10.20.1, one file at 0.5MB, two files just over 6.5 MB each and one file at 45MB (the NodeJS main source tarball) were needed. So that's almost exactly the size to download this repo, and it's all those NodeJS source files I added in that PR some time ago. Ah, well.)

[DeeDeeG]

This comment ended up being long also, bold stuff is for skimming.

---

While I'm pretty confident this PR works as it should, I would note in particular that the code style of the new JS script I wrote for this PR was not linted. I don't really have any strong opinions on it, but would be willing to accept any wisdom/suggestions for changes.

For example: I would be willing to run prettier or some other linter on it, to make it neater and enforce some limit to the line lengths by indenting some stuff onto the next line. Or whatever the more JS-experienced folks here might want. But I have tested on my local machine, and in CI, and it does what it's supposed to do so far.

(Codacy seems pretty unhappy about it, but a lot of that is it doesn't like single quotes ' and prefers double quotes ". And I didn't put semicolons at the ends of lines in a bunch of places. And there are some rather long lines that it wants to indent to the next line, which is understandable IMO. On closer look, it appears to be running prettier. So that would be easy enough to fix.)

(Note: You can do node script/ensure-node-source-fixture-files.js --verbose to get a lot of verbose logging output, for troubleshooting or if you're curious what the file is doing. Although the multiple file downloads happen async in parallel, so certain messages post-download for each file will be interspersed and print somewhat jumbled/out of order. So it might be a bit hard to read.)

I put a lot of comments in the new script file, so I hope it is easy to quickly get a handle of what the new script is doing.

Which reminds me, one more thing. The script I wrote has a kind of ridiculously long and verbose filename. I would be open to shortening that filename if people want it to not be quite so long.

[Spiker985]

I have spent far too long transposing most of the Codacy suggestions

[confused-Techie]

It's amazing to see all passing CI here, and know that our lives will be easier in the future, to have to avoid this exact thing in the next couple years. Of wondering what we did all that time ago to fix it. So this is looking great from the commits and summary, but I'll review the actual source in a moment here.

Otherwise I personally love the long descriptions, especially the history I keep finding out about Atom as it was created with them.

Additionally I appreciate @Spiker985 added all the Codacy suggestions, but do want to note, these shouldn't be taken as law. They can be great suggestions, but this Codacy instance on PPM is basically the default. I haven't spent the same amount of time tweaking it as I did on Pulsar, so if a suggestion feels unnecessary, it just might be. Specifically about ' to " or vice versa, that isn't a convention anyone on this codebase has agreed on, so feel free to ignore it until such time as we have agreed on it.

But I'll create an issue to keep a reminder to myself to fine tune these Codacy rules in the future to avoid any confusion.

[Spiker985]

Yeah, the primary reason I went through the effort of transposing them was simply that there were mixed cases of ', ", and `

So I at least transposed the ' -> " rules so there would only be " and `

[DeeDeeG]

Hi folks, I made some changes, and this is ready for review again!

---

Indentation

All but one of the remaining Codacy suggestions are "indentation level" suggestions from prettier, suggestions which I personally object to on readability grounds. See the suggested diff if you'd like:

suggested diff from prettier (click to expand):

diff --git a/script/ensure-node-source-fixture-files.js b/script/ensure-node-source-fixture-files.js
index 5c78e76..e5122c3 100644
--- a/script/ensure-node-source-fixture-files.js
+++ b/script/ensure-node-source-fixture-files.js
@@ -12,32 +12,43 @@ const filesToFetch = [
   {
     url: `https://nodejs.org/dist/${nodeVersion}/node-${nodeVersion}.tar.gz`,
     filename: `node-${nodeVersion}.tar.gz`,
-    sha256sum: "ba8174dda00d5b90943f37c6a180a1d37c861d91e04a4cb38dc1c0c74981c186"
+    sha256sum:
+      "ba8174dda00d5b90943f37c6a180a1d37c861d91e04a4cb38dc1c0c74981c186",
   },
   {
     url: `https://nodejs.org/dist/${nodeVersion}/node-${nodeVersion}-headers.tar.gz`,
     filename: `node-${nodeVersion}-headers.tar.gz`,
-    sha256sum: "9d55ee072ba6d5a141db092cef1a0f715f7d3fc938285a6d927a1d0a0c7442f7"
+    sha256sum:
+      "9d55ee072ba6d5a141db092cef1a0f715f7d3fc938285a6d927a1d0a0c7442f7",
   },
   {
     url: `https://nodejs.org/dist/${nodeVersion}/win-x64/node.lib`,
     filename: "node_x64.lib",
-    sha256sum: "1bd376a23d181d85096d1a9c46e6be7fcd20d30f9b8f77a2a847d3dbff8e25c7"
+    sha256sum:
+      "1bd376a23d181d85096d1a9c46e6be7fcd20d30f9b8f77a2a847d3dbff8e25c7",
   },
   {
     url: `https://nodejs.org/dist/${nodeVersion}/win-x86/node.lib`,
     filename: "node.lib",
-    sha256sum: "b1c6dc670911d85ef1704fa56f4cc4c7e1071f4869778398e6d88b3b0b565978"
+    sha256sum:
+      "b1c6dc670911d85ef1704fa56f4cc4c7e1071f4869778398e6d88b3b0b565978",
   },
   {
     url: `https://nodejs.org/dist/${nodeVersion}/SHASUMS256.txt`,
     filename: "SHASUMS256.txt",
-    sha256sum: "64aad1211a6003dd6529ebf9f19167769d0356ce5affc4245bb26c35aa66a9ed"
+    sha256sum:
+      "64aad1211a6003dd6529ebf9f19167769d0356ce5affc4245bb26c35aa66a9ed",
   },
 ]; // If you ever need to update to a newer Node version, just calculate the sha256sums
-   // for the new files and update them here to match the new files.
-
-const sourceFixtureDir = path.resolve(__dirname, "..", "spec", "fixtures", "node-source");
+// for the new files and update them here to match the new files.
+
+const sourceFixtureDir = path.resolve(
+  __dirname,
+  "..",
+  "spec",
+  "fixtures",
+  "node-source"
+);
 fs.mkdirSync(sourceFixtureDir, { recursive: true });
 
 for (const details of filesToFetch) {
@@ -55,7 +66,10 @@ async function ensureFile(details) {
   const destinationPath = path.resolve(sourceFixtureDir, details.filename);
   logVerbose(`destinationPath is: ${destinationPath}`);
 
-  const existingFileIsCorrect = await verifyExistingFile(destinationPath, details.sha256sum);
+  const existingFileIsCorrect = await verifyExistingFile(
+    destinationPath,
+    details.sha256sum
+  );
 
   if (!existingFileIsCorrect) {
     logVerbose("Hash did not match, re-downloading...");
@@ -65,7 +79,10 @@ async function ensureFile(details) {
         console.log(`Successfully downloaded file from ${details.url}.`);
         logVerbose("checking if hash matches in for...of loop.");
         // Check its hash
-        const hashDidMatch = await verifyHash(destinationPath, details.sha256sum);
+        const hashDidMatch = await verifyHash(
+          destinationPath,
+          details.sha256sum
+        );
         if (!hashDidMatch) {
           console.error(`Hash did not match for ${destinationPath}`);
         }
@@ -138,13 +155,16 @@ function downloadFileToDestination(url, filePath) {
       // Technically any 2XX series HTTP response code means success,
       // but in practice this should always be exactly "200"? Adjust if needed.
       if (res.statusCode === 200) {
-        res.pipe(fs.createWriteStream(filePath))
+        res
+          .pipe(fs.createWriteStream(filePath))
           .on("error", reject)
           .once("close", resolve);
       } else {
         // Consume response data to free up memory
         res.resume();
-        reject(new Error(`Request Failed With a Status Code: ${res.statusCode}`));
+        reject(
+          new Error(`Request Failed With a Status Code: ${res.statusCode}`)
+        );
       }
     });
   });

I'm willing to revisit these "indentation level" code style things, but I personally like the code as-is, and I kind of despise the "towering sandwich" style of putting every comma-separated argument on a new line... I prefer related things on the same line most of the time. Just makes more sense to me when viewing and trying to comprehend what the code is actually doing.

---

Opera Mini???

It's also warning me that doing new Promise isn't compatible with... Opera Mini. Which I'm not really concerned with here. (I guess we can disable that lint rule, since none of this code is going to be running in Opera Mini!)

---

Let's reduce those filesizes, baybee!

Incidentally: Not sure how well we're filtering ppm's files in the Pulsar electron-builder config, but it looks to me that this will shrink the bundled ppm dir's size by about ~58 MB in Pulsar, because these spec fixture files relating to the node source tarball are no-longer checked into the repo. A happy side-effect of fetching these files only on-demand, and only when running the tests is needed!

This PR should reduce packaged Pulsar's size by a bit!!!

[DeeDeeG]

So, having thoroughly bikeshedded the JS style... I'm still open to that I suppose, but the more meaningful feedback would be:

On this branch, has anyone tested the new code?

1. You can do npm run ensure-fixtures to try out the new script I wrote. (It's working for me on Windows, macOS, Linux, and likewise in CI on all three OSes. But if someone can find a way to break it, I'm interested. It runs without any dependencies installed, so it can be tested independently of the install process or the specs.)
2. You can fully install the repo's deps and run specs, to make sure I didn't screw anything up when changing the paths to the fixtures. Although it works for me locally, and it's passing in CI.

So if anyone wants to test, or any additional comments on the new JS script, that'd be great. Otherwise if it's all looking good, I'm looking for an approve to merge this. Thanks everyone.

Best Regards,
- DeeDeeG

[DeeDeeG]

I just squashed the 11 lint commits into one. We don't need all those separate in the repo's history, IMO.

If anyone needs to see all the individual lint commits for whatever reason, they are still around in the git objects here: commits/7895404 or here: DeeDeeG/commits/old-pr-42.

I'd like to merge this ~within the next week if there are no outright objections.

(The old "Changes requested" review of this is outdated and has already been addressed. Not sure why it's still showing, but who knows?)

[confused-Techie]

Overall this looks fantastic!

I've left a couple notes about nice to haves, or in general comments on the code, but otherwise I'd be happy to merge as is.

Especially love your liberal use of declaring variables to contain node versions, rather than the madness of having it declared a dozen times across files.

The biggest concern here is this would need work to be redone on some of the Spec PR's which like we discussed I'm just now trying to finally get a handle on. So would be rough to ask them to redo the work, but if this is a needed fix then we do what we have to, otherwise if I were to set a strict deadline for myself on reviewing and merging those, would you be comfortable merging this after and making the needed changes to have this work on the decaffed spec?

Feel free to let me know your thoughts, but again overall super rad work

[confused-Techie]

I personally would wrap this in a try...catch block just to ensure we are aware of a failure and can fail gracefully.

Otherwise as this is a simple script file we may not want to fail gracefully, and in that case feel free to leave as is.

[DeeDeeG]

At least for the case of the path already existing, recursive: true makes it so no error is raised for that case.

It's inspired by how certain recursive Unix commands (cp, rm, etc.) don't raise certain errors when you run with --recursive.

See: https://nodejs.org/api/fs.html#fsmkdirpath-options-callback

Calling fs.mkdir() when path is a directory that exists results in an error only when recursive is false.

If that's the only error you had in mind, then it is handled by using the recursive: true option.

Otherwise if there are other concerns, I can look at try...catch. We do need to make that dir, or the script won't work. The whole objective in this script is to download some files to that dir. But we could print something more helpful than just the bare error and then exit, I guess.

[DeeDeeG]

An alternative to even making the dir in this script would be to commit the dir with an empty .keep file in it, so git can track the folder and we can commit the folder as already existing in the repo.

(git won't track a truly empty dir. So I guess it wants to track files and record their paths, not track dirs, per se. So I needed to either do the .keep thing and commit the dir in git, or generate the folder during this script.)

With that, we could avoid using fs.mkDirSync() in this script.

[confused-Techie]

I'm glad you left the comment here, otherwise I was about to write up worried about unneeded resource usage.

But I've seen that before funnily enough in native modules that aren't context aware, or what's more likely is this builder works with a globally defined object (in relation to the actual module) and since NodeJS will require the same module it would likely actually retain it's state, including any global variables declared. So probably why it gets all worried about being used multiple times simultaneously.

[DeeDeeG]

Yeah, I'm convinced there's a proper way to clear that reused state, but I tried a bunch of things and wasn't able to figure it out. I would not at all mind a fix for this that only require()s it once. But I couldn't figure it out myself, personally. All the hashes were coming out wrong starting at the second file I tried to hash, IIRC.

[DeeDeeG]

I probably went overboard on the changes...

And uh, sorry this is so long, bolded the important bits.

• Moved to building (the dummy native modules in these specs) against the C/C++ headers for Electron 12.2.3, to match Pulsar's Electron version, instead of some arbitrary Node version's C/C++ headers. Theoretically ensures (trivially simple) dummy C/C++ native addons for Node modules can be built by ppm against Pulsar's actual Electron version (so long as we remember to update the nodeVersion thing.) Reminder that this is still only about running the specs. Has nothing to do with "prod" other than good validation for our PRs before they merge.
• Moved nodeVersion to a .JSON file and re-used that in any file that uses this value. No consts hard-coded everywhere that you have to remember to update. Update it once and be done! Makes changing the version of Node/Electron headers we build against almost a one line PR for the future! ... Almost. One line, other than also updating the SHA256SUMs. See next bullet point for that...
• Added instructions to calculate new SHA256SUMs if changing the nodeVersion.
  • Note: We kind of don't need these SHA256SUMs, as CI will generally have the right files regardless... We could just not cache for CI, it wouldn't be a lot of bandwidth to Electron's header server I'm sure. But it could mean obscure spec failures when testing locally. Something we don't seem to do as often as we should, but yeah. Instead of validating existing files, we could just... trust users to manually keep them up-to-date? Or auto-delete and download fresh headers for every test run??? IDK. So, while these SHA256SUMs may be annoying to maintain, but I hope they strike the right balance.
  • Alternatively, we could find a way to read the official SHA256SUMS file into the JS logic and just rely on that. But I kind of don't know what we'd do if there was an error downloading the SHA256SUMS itself, and this was the most robust and simple method I could come up with, just fetch the files once and hard-code their SHA256SUM hashes once per nodeVersion change. Avoids entire categories of weird debugging if we can't get SHA256SUMS off the server, theoreticall guards us against maliciously uploaded files on the official download server... which sounds unlikely but IDK?
• Improved the "HTTP status wasn't 200" message in the new script, to include the URL of the request, because not knowing which URL gave a 404 or 500 was confusing.
• Rearranged the order of the filesToFetch to more closely match the output you're going to see if you calculate the hashes of the new files when updating to a different Node or Electron version. A little quality of life improvement if this ever needs to be updated again. (Should be updated if we update Electron in Pulsar, ideally.)
• Fix the file structure the node.lib files are served under when running the specs on Windows. Note: Specs are still not passing on Windows. :[ Wasn't able to figure out what's causing this, as there is literally no output from the failing tests other than the dots . showing each passed test and the Fs showing the failed tests. Weird and frustrating, super hard to diagnose. I think it's related to node-gyp somehow, since my CMD.exe app title briefly switches to say node-gyp just before each of two failures (F) that I see. This is arguably super tangential to the point of this PR, but I noticed it was broken and I wanted to fix it. Hasn't gotten me very far, as the Windows tests are pretty much just as useless before/after this change. But in theory this is one less extant issue to track down for Windows? We'll see, I guess.

I guess I just had too much freedom here and went a little wild waiting for this to have the right moment to be merged.

Let me know if this is too much to review right now and I'll break it off into a follow-up PR, since the stuff that was there was also working. These are nice improvements, IMO, but I don't want to overburden reviewers.

Also: I already got an approve on the prior design, so I am willing to YOLO merge (as in, without a reviewer) these additional changes at some point if it goes a while longer and there are no objections, when the timing is right vis-a-vis decaf work, and I'll be available for any questions/complaints/reverts anyone wants.

[DeeDeeG]

(Also, I am starting to be inclined to remove logVerbose(), since it's basically a noob programmer's "throw console.log() everywhere" debugging style, and I think the code works okay and has okay error handling without logging every little detail, and the comments are probably more than sufficient to help people understand what is going on. Without the logVerbose() stuff, this file is about 30 lines shorter, 184 --> 154 lines.)

[DeeDeeG]

IMPORTANT UPDATE: So... The Electron files are a ton smaller than Node's.

NodeJS v10.20.1's node-v10.20.1.tar.gz alone is 45 MB.

This alternate implementation (see link just below) of what this PR is trying to do can check in all the relevant files for Electron v12.2.3, and have the entire spec/fixtures dir weigh in at ~2.5 MB. (Or ~1.8 MB if we give up on testing on Windows 32-bit, ~613 KB - ~1.1 MB if we give up on testing on Windows altogether.)

Alternate implementation: master...DeeDeeG:ppm:alt-pr-42 as of commit 399a40f

(Tangent/Reminder: The spec runner is super broken on Windows for this repo, not even outputting anything other than dots . for passing tests and F's F for failing tests. All the rest of the output is truncated. I have no insight into even which specs are failing on Windows, much less how to get them to pass.)

So... I think this whole effort I put in is a moot point, and we can just check in the newer Electron fixture files directly in git. No added complexity needed. They are tiny. About ~1.75 MB!!!

[confused-Techie]

@DeeDeeG Lets go ahead and get this merged.
I had hoped to have the decaf PR's merged by end of week, and that has passed some time ago. So lets not hold up other fixes and improvements for decaf work.

I still do want to get to those, but may need to restructure things a bit more to assist in that process, since as is, can still be rather painful. And when that's done we will just have to work around your changes here, thanks as always.

[DeeDeeG]

Okay, thanks for touching base.

But I think I may spin up a much simpler version.

Reasons for the simpler version (click to expand):

since the Electron headers (~ < 1mb per file?) turned out to be much smaller than the NodeJS headers, and the big source tarball turns out can be just deleted, so I'm comfortable checking those into the repo and getting rid of this complexity -- @Spiker985 commented in the Discord about the complexity and I realized I might have gone a bit overboard. But besides, when I found out the Electron files were just so small... it's like on the order of a large JS file or something... I suddenly felt like that would be pretty manageable.

Once I get the simpler version up I will give people a chance to weigh in which approach they like better. It'll be a much, much simpler PR than this one, hopefully a breeze to review. I didn't want to post it until decafing the specs was done, but given your comment above I'll go ahead and post that now.

[DeeDeeG]

UPDATE: I posted #52 as the simpler alternative to this.

[DeeDeeG]

Gonna close this, just a memo that if anyone wants a convenient script to automate bumping these spec fixtures to be ones for a newer Electron version, please ping me, as the JS script I put together here is still good for that and can be adapted to meet that use-case a little better than what I was going for here, but could also be used pretty much as-is with tweaks to match PR #52's dir structure instead of how this PR set things up.