Revert setting of --force-process-config…

[savetheclocktower]

…now that we’re on Electron 30.

This is the result of an afternoon-long investigation trying to determine why ppm install didn't work right in CI for the github repo. Turns out it doesn't work right on my local machine, either!

ppm is using Node 20.11.1 to match the version that Electron 30.0.9 runs. But Node's node_module_version for 20.11.1 is 115, whereas Electron's is 123. So our usage of force-process-config (inheriting node-gyp configuration from process.config rather than from $nodedir/include/node/config.gypi) was leading to a version mismatch and this error:

/Users/andrew/.pulsar/.node-gyp/Library/Caches/node-gyp/30.0.9/include/node/node.h:27:2: error: "It looks like you are building this native module without using the right config.gypi.  This normally means that you need to update electron-rebuild (>=3.2.8) or node-gyp (>=9.0.0) if you're building modules directly."

All this goes away if we remove the force-process-config option. (The github package needs further massaging for CI to work properly, but that's another story.)

Usage of this option was introduced in #79 and was documented properly in the code:

# node-gyp >=8.4.0 needs --force-process-config=true set for older Electron.
# For more details, see: https://github.com/nodejs/node-gyp/pull/2497,
# and also see the issues/PRs linked from that one for more context.

The PR in question explains that “old” Electron versions would still need --force-process-config to be true so as to work around “a malformed config.gypi.” I don't know exactly how to define “old” in this context, but I take it to mean that any version of Electron newer than that PR itself (from September 2021) would not fall under that definition. Our Electron is certainly newer than that, so that's further confirmation that removing this option is the correct thing to do.

I don't know why having this option set didn't result in errors in the spec suite. But you can test this change the same way I did:

1. Go into the ppm directory within your Pulsar installation
2. open src/command.js and remove the two usages of force_process_config temporarily
3. check out the github repo, cd to its root, and run ppm install

Annoyingly, this is now probably a must-land before 1.131.0. But once this gets approved I can bump the ppm we use in Pulsar and put out a new rolling release.

[savetheclocktower]

Pinging @DeeDeeG because they're the original author of #79, so a confirmation from them that this is the right thing to do would give me more confidence.

[DeeDeeG]

Honestly, the screwiness of node-gyp, Node and Electron feels like a so-called Mexican standoff at times. This feels very par for the course. Thank you past me for leaving some sort of code comment justifying/contextualizing the --force-process-config flag usage. And thank you for following that trail of breadcrumbs!

I can try and find some time to follow those relatively simple verification steps.

[DeeDeeG]

From the Electron 16.0 blog post PR discussion, of all places...:

electron/website#143 (comment)

Building native modules for old versions of Electron (<=13, < 14.2.0, < 15.3.0) will require passing --force-process-config to node-gyp. You are likely to encounter this issue if your project is using custom scripts to build native modules. You can use #2497 to automatically pass --force-process-config.

That clarifies which Electron versions are meant by "old."

(I don't think I've known that set of version ranges before in my life, despite my past digging around/researching in this space... or if I did know, I've had plenty of time to forget. But anyway!... Found it!)

[DeeDeeG]

I don't know why having this option set didn't result in errors in the spec suite.

The existing dummy native module (spec fixture) exemplifies an extremely basic native module.
https://github.com/pulsar-edit/ppm/blob/b49194b5a3e1a7ac7890aec9cfb9da7fcbf40248/native-module/src/native.cc

It doesn't #include very many headers in C-land. Perhaps so few that Node header stuff never gets loaded into compilation. Adding an even modestly more complex test fixture for native code addon module compilation than that one linked above might be useful for flagging stuff like this! I think!

(I invoke the usual incantation "I'm not really that good at C" to ward off nasal demons at this point, for safety's sake. But alas, I [get to / must] deal with C to some extent. And deal with it I will, from time to time. Anyhow.)

In conclusion:

WANTED: A MORE COMPLEX DUMMY MODULE FOR USE AS A TEST FIXTURE.
REWARD: THANKFUL PULSAR DEV(S).

[savetheclocktower]

Hey, at least we caught it before 1.131.0 shipped.

[DeeDeeG]

Tested in the manner suggested at the bottom of the PR body.

Confirming the issue: Yep. B0rked without this change.

  CXX(target) Release/obj.target/superstring/src/bindings/bindings.o

In file included from ../src/bindings/bindings.cc:1:
In file included from ../src/bindings/marker-index-wrapper.h:1:
In file included from ../../nan/nan.h:56:
/Users/User/.pulsar/.node-gyp/Library/Caches/node-gyp/30.0.9/include/node/node.h:27:2: error: "It looks like you are building this native module without using the right config.gypi.  This normally means that you need to update electron-rebuild (>=3.2.8) or node-gyp (>=9.0.0) if you're building modules directly."
#error "It looks like you are building this native module without using the right config.gypi.  This normally means that you need to update electron-rebuild (>=3.2.8) or node-gyp (>=9.0.0) if you're building modules directly."
 ^
In file included from ../src/bindings/bindings.cc:1:
In file included from ../src/bindings/marker-index-wrapper.h:1:
In file included from ../../nan/nan.h:176:
../../nan/nan_callbacks.h:55:23: error: no member named 'AccessorSignature' in namespace 'v8'
typedef v8::Local<v8::AccessorSignature> Sig;
                  ~~~~^

Result: Still some errors with this change (error: no member named 'AccessorSignature' in namespace 'v8'), but no "It looks like you are building this native module without using the right config.gypi. [and so on]".

So, I think this is progress?

(Note: ppm install the second time "succeeds" but probably skips rebuilding packages with partly-built native code stuff. The more clean-slate ppm rebuild still has errors for me.)

I may want to double-check I can actually build something at some point, but github package is a rather heavy test-case, maybe I can find something easier to build...

I hope to give an Approve if I find further success building a package due to this change specifically, or if much time goes by without a chance for me to confirm the fix does/doesn't fully work, given basic indication of it being an improvement, despite my testing hurdles locally.

[savetheclocktower]

To get ppm install to run successfully for the github package I also had to bump keytar to a newer version. I thought I had put that in the description but clearly I just hallucinated that I had.

[savetheclocktower]

Oh, also: I bet that's related to trying to install old superstring instead of @pulsar-edit/superstring. Switching to the new @pulsar-edit/whats-my-line should fix that!

It might be easier to just checkout github#46, since it makes both of these changes. My deepest apologies for not writing better testing instructions.

[DeeDeeG]

I for some reason assumed we were stuck on older keytar. I dropped in newer nan and node-abi manually but didn't think to update keytar itself. Hmm! Can probably try that real quick...

[DeeDeeG]

Okay, let's try this again. Round 2, go!

Testing:

• Confirming the reported issue, again: Checking out pulsar-edit/github#46 and doing rm -rf node_modules and ppm install without this fix:

  • It no work. Error is long but includes the "[not] using the right config.gypi" part as expected.

• Manually applying this fix as mentioned in this PR's body text, then rm -rf node_modules and ppm install:

  • It work! 🎉

Conclusion:

LGTM! 👍 It fixes the thing. This appears consistent with the lore (ancient Electron blog post discussions).

Thank you for digging into the weeds and finding the fix for this one!! Much appreciated!

[DeeDeeG]

Nothing else needed on this and I confirmed it can help, and seems consistent with explanations I could find on upstream projects, so, merging!

Thanks again! 🙇