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]

@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. :disappointed:

[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?

[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: https://github.com/nodejs/node-gyp/issues/1662#issuecomment-1270232265

[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 (https://github.com/electron/electron/issues/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 👍