Support build for electron v20

[neoxpert]

This PR contains changes in order to support builds against electron 20.

Note: Used MSVC 2022, NodeJS 16.17.0 LTS. Tested builds for electron v16 to v20.

fixes #858
fixes #867

[Prinzhorn]

I don't think this is required? better-sqlite3 never had node-gyp in its dependencies and from the native modules I know this is not common. Can you link to some docs that recommend doing it? I think this will cause more trouble down the line and it's technically not a dependency but part of the build tooling you need (just like Python, make or gcc). If anything it would be a optionalDependencies or peerDependencies.

I'm just trying to improve the chances of getting this merged anytime soon.

[neoxpert]

The issue here was, that prebuild is still using node-gyp 6.0.1. If you try to build for electron 20, using this old version, the build would fail, stating that node-gyp below version 8.4.1 is not supported anymore. So the explicit use of at least node-gyp 8.4.1 is required.

In addition to that I only had msvc 2022 available on the machine I was testing on, which requires node-gyp > 9.0.0 as otherwise no compiler is found.

[Prinzhorn]

Maybe we can extend the node-gyp docs here https://github.com/WiseLibs/better-sqlite3/blob/master/docs/troubleshooting.md . There have been issues in the past where local node-gyp would override the global one. I think that's why it was left to the user (and in the best case you're getting prebuilds and don't even need to worry about it). I'm not pro or against either, I want whats recommended and is the best option for our users.

[neoxpert]

I tried using a global installation of node-gyp on a fresh VM. Without overwriting node-gyp, the local version of prebuild still overwrites the global one ending up with building using node-gyp 6.0.1 which is not viable.

Running npx --no-install prebuild -r electron -t 20.0.0 yields:

prebuild\electron\20.0.0\include\node\node.h(27): fatal error C1189: #error:  "It looks like you are building this native module without using the right config.gypi.  Thi
s normally means that you need to update electron-rebuild (>=3.2.8) or node-gyp (>=8.4.0) if you're building modules directly."

I'll remove the dependency, but how to address this issue? No matter which approach I try, to enforce the use of the global installed node-gyp version, prebuild always falls back to its local 6.1.0.

[Prinzhorn]

Well, maybe we need to include node-gyp prebuild/prebuild#286 and the overrides then. I'm not an expert on this, but if it makes the builds more predictable I'm all for it. And I changed my mind on the "it's technically not a dependency", since the build-release script literally needs node-gyp when no prebuild is found. But it should be the absolute minimum like 8.4.0 to be as compatible as possible. Otherwise if a project contains a second dependency that depends on node-gyp we're increasing the chances of conflicts. But someone else should weigh in.

[neoxpert]

As sqlite3 is using 8.4.1 I would tend to also go with this version for now. But as I am not that deep into this topic yet, I am open for any good advise.

[Prinzhorn]

@mceachen do you have the necessary permissions to trigger the workflow? I want to see if it succeeds everywhere so that we can move on from there or make changes if necessary. I have a feeling this might fail on older Node.js.

[mceachen]

Ugh, node 12:

./src/util/binder.lzz:37:51: error: ‘class v8::Object’ has no member named ‘GetCreationContext’; did you mean ‘CreationContext’?

@JoshuaWise we should drop unsupported versions of Node: v12 EOL was 2022-04-30.

[mceachen]

So unfortunately GetCreationContext was introduced after Node v14, so we may have to #ifdef around this issue. 😞

[neoxpert]

Ugh, node 12:

./src/util/binder.lzz:37:51: error: ‘class v8::Object’ has no member named ‘GetCreationContext’; did you mean ‘CreationContext’?

No, the use of GetCreationContext is intentional, as CreationContext is no longer available for v8::Object in the electron 20 header files.

v8-object.h for electron 19.0.0 line 495 and following:

/**
 * Returns the context in which the object was created.
 */
V8_DEPRECATED("Use MaybeLocal<Context> GetCreationContext()")
Local<Context> CreationContext();
MaybeLocal<Context> GetCreationContext();

v8-object.h for electron 20.0.0 line 495 and following:

 /**
  * Returns the context in which the object was created.
  */
  MaybeLocal<Context> GetCreationContext();

Unfortunately this breaks backwards compatibility as the first version to support GetCreationContext was electron 13, at least it's the first version to have it in the header files.

[quanglam2807]

Thank you for the PR. It looks like we also need to use C++ 17 instead of C++ 14 for this to work (ref, Signal's fork: https://github.com/signalapp/better-sqlite3/blob/better-sqlcipher/binding.gyp#L12)

The change failed to compile on my Mac M1 but after switching to C++ 17, it works.

[quanglam2807]

-      'cflags': ['-std=c++14'],
+      'cflags_cc': ['-std=c++17'],
       'xcode_settings': {
-        'OTHER_CPLUSPLUSFLAGS': ['-std=c++14', '-stdlib=libc++'],
+        'OTHER_CPLUSPLUSFLAGS': ['-std=c++17', '-stdlib=libc++'],
       },

[mceachen]

Kudos, @neoxpert , thanks for your work here, and for the assist, @quanglam2807 !

I’ll try this branch locally with PhotoStructure’s test suite, but if anyone else can verify it works that’d be better.

@JoshuaWise do you want to review this before it gets merged?

[Prinzhorn]

^?

[mceachen]

Although the number of times I've seen non-semver-compliant changes in npm packages is decidedly nonzero. It's safer to pin it, probably.

[Prinzhorn]

I thought this was relevant when another package declares something like ^8.4.3, then they conflict? But maybe I'm wrong about how module resolution works when two packages both require an incompatible version of the same package.

[neoxpert]

Sqlite3 for example already has node-gyp 8.4.1 as dependency, that's why I added and pinned this dependency. We can unpin or even remove it, if a users global installation should be used for better-sqlite3 and add a requirement to use 8.4.1 or higher when using electron v20+. But when it comes down to run prebuild, the overwrite would still be required, as prebuild would always use its internal node-gyp 6.1.0, which is not able to build for electron v20.

[mceachen]

lgtm

[neoxpert]

As the changes of this merge request are also required to build for the todays release of Electron v21, would we handle this within this merge request / branch or wait until this one is merged and create a new one after that?

[Prinzhorn]

@neoxpert could you fork this PR and create a new one with 21 added for the CI? I can't speak for the maintainers, but I just want to make sure that it works (new memory cage should not affects us) so that we're prepared for 21 already. In general I don't see a problem with adding the 21 to this PR, but maybe leave it for now.

[zhoukq]

Hi, just wondering when will we merge this PR?

[JoshuaWise]

I don't have an M1 mac, but maybe we should incorporate @quanglam2807's feedback before merging?

[mckravchyk]

I don't have an M1 mac, but maybe we should incorporate @quanglam2807's feedback before merging?

Yes, please. It's not just M1, but also x64 Linux - the commit in the Signal fork that adds this fix is titled: "Fix Linux builds", I can also confirm that I had to apply it before I could build it successfully (on Linux).

[neoxpert]

I added the changes mentioned by @quanglam2807.

[maximilize]

On windows, there are also build issues. This comment saved my day: nodejs/node-gyp#1662 (comment)

[neoxpert]

@maximilize Just out of curiosity, which MSVC version are you using? So far I did not encounter any issues with MS Build 2017, 2019 and 2022.

[segmentedcontrol]

people who can merge, please take another look?

[anonrig]

@segmentedcontrol My fork (@anonrig/better-sqlite3) has both arm64 prebuilds and electron 20 & 21 support: http://github.com/anonrig/better-sqlite3

[megancooper]

For anyone else waiting on the merge, you should be able to install with:

npm install WiseLibs/better-sqlite3#pull/870/head

Has saved me some time for now 🙌🏽

[ddramone]

Any plans of merging this in near future? 👀

[Juanc1to]

With yarn, the syntax that worked for me for installing this PR directly is slightly different:

yarn add node-gyp
yarn add better-sqlite3@neoxpert/better-sqlite3#fix_electron20_build

[neoxpert]

So I also added the /std:c++17 flag for the MSVC settings. But I think it is worth to mention that, as of now, builds against the electron 20 / 21 API only work with MSVC 2019 or 2022. While 2017 should work (whyever it did before I cleaned my MSVC setups...), there seem to be known issues regarding that (electron/electron#36033) leading to build errors (shortened output):

..\v8-callbacks.h(335): error C2062: "int"-Typ unerwartet
..\v8-isolate.h(292): error C3646: "legacy_oom_error_callback": Unbekannter Überschreibungsspezifizierer
..\v8-isolate.h(292): error C4430: Fehlender Typspezifizierer - int wird angenommen. Hinweis: "default-int" wird von C++ nicht unterstützt.
..\v8-isolate.h(1482): error C2061: Syntaxfehler: Bezeichner "LegacyOOMErrorCallback"
..\v8-isolate.h(1545): error C2061: Syntaxfehler: Bezeichner "WasmDynamicTieringEnabledCallback"
..\v8-initialization.h(290): error C2061: Syntaxfehler: Bezeichner "LegacyOOMErrorCallback

[JoshuaWise]

Thanks for all the work on this @neoxpert and everybody 👍