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

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

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Jan 17, 2023

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 #42.)

What is this good for, anyway?

(IN SHORT: Works around nodejs/node-gyp#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 atom/apm#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 and v0.10.41, and node-gyp moved to it for those versions and newer as of node-gyp v3.3.0. 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, 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 atom/apm@77227d8 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.

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.
- 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 42.)
@DeeDeeG

This comment was marked as outdated.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jan 23, 2023

@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 nodejs/node-gyp#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
Copy link
Member

@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
Copy link
Member Author

DeeDeeG commented Jan 23, 2023

@Spiker985 I have a commit ready to go: DeeDeeG@d73b0b0

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
Copy link
Member

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
Copy link
Member Author

DeeDeeG commented Jan 23, 2023

@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
Copy link
Member

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

Just tidying up, and making it more obvious these files are related.
This version string is part of the filenames and paths (dist server
directory structure) used by node-gyp when fetching headers and
dynamic libraries, which are used to build native C/C++ addons against
NodeJS or Electron for use in various JS modules.

So, we have to have the version number of these files in order to run
a working "dist server" serving these files for use in the specs.

We can put this version string in a JSON file that is read from
all the spec files that need it, rather than hard-coding "v12.2.3"
several times per file, across several different spec files.

This version string should always match the version of the headers
tarball, node.lib, and SHASUMS256.txt files under `spec/fixtures/...`.
Update the version string and the files together, at the same time.
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jan 24, 2023

Okay, this is ready to review again!

Copy link
Member

@Spiker985 Spiker985 left a comment

Choose a reason for hiding this comment

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

Header files have been organized, parameterized, and reduced in size. All checks are passing.

After this is merged, we can decaffeinate the source code

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jan 24, 2023

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.)

@DeeDeeG DeeDeeG merged commit 4645ba2 into master Jan 24, 2023
@DeeDeeG DeeDeeG deleted the specs-electron-12-headers-not-node-10 branch January 30, 2023 01:27
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