better-sqlite3 icon indicating copy to clipboard operation
better-sqlite3 copied to clipboard

fix for build failure: electron 29.0.0-alpha.7

Open sparecycles opened this issue 1 year ago • 1 comments

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] for the project I'm working on.

I'm building better-sqlite3 with electron-forge / vite and there's a lot of warnings and one actual error.

The actual error involves the signature of SetAccessor and what I assume has been thrash of its myriad overloads conflicting with what's currently specified in better-sqlite3, but as it appears that what's specified in better-sqlite3 is the defaults, simply removing them might be fine?

I don't feel like confirming this is the correct course for historical versions of NodeJS, which is why this is an issue and not a PR.

diff --git a/node_modules/better-sqlite3/src/better_sqlite3.cpp b/node_modules/better-sqlite3/src/better_sqlite3.cpp
index 0fe8ee2..220d15a 100644
--- a/node_modules/better-sqlite3/src/better_sqlite3.cpp
+++ b/node_modules/better-sqlite3/src/better_sqlite3.cpp
@@ -128,9 +128,7 @@ void SetPrototypeGetter (v8::Isolate * isolate, v8::Local <v8::External> data, v
                 InternalizedFromLatin1(isolate, name),
                 func,
                 0,
-                data,
-                v8::AccessControl::DEFAULT,
-                v8::PropertyAttribute::None
+                data
         );
 }
 #line 4 "./src/util/constants.lzz"

Hopefully I'm not doing something horribly wrong here: if that's evident, please let me know! 😉

This issue body was partially generated by patch-package.

sparecycles avatar Jan 12 '24 02:01 sparecycles

Thanks for taking the time to do this research and submit this!

@JoshuaWise can you check this out when you get a chance?

mceachen avatar Jan 16 '24 23:01 mceachen

Hi, I got the same bug with [email protected] and [email protected].

Your fix is working well, you save my day, thanks :)

LaGregance avatar Mar 16 '24 10:03 LaGregance

Seems like this is working for me too. Any chance to get a release of this ? I don't believe anyone can upgrade to electron 29 without this fix otherwise.

pelletier197 avatar Mar 25 '24 11:03 pelletier197

Fixed by https://github.com/WiseLibs/better-sqlite3/pull/1151

Sorry I missed this one!

JoshuaWise avatar Apr 03 '24 04:04 JoshuaWise

Thank you for releasing this!

Of course, I seem to not be able to fully enjoy it as python 3.12 implies node-gyp "^10" which seems to hang the electron build native dependencies step. (both [email protected] with my patch-package modification and 9.4.4 without).

Downgrading to python 3.11 and not pinning node-gyp with overrides in the package json still builds, (but now my build process "works on my machine" only... and I'd rather not have to dockerize my electron build at this point, it's slow enough as it is).

This could easily an electron-forge or node-gyp 10 issue, but if anyone has any guidance/ideas to share, I'd appreciate it!

Guessing: https://github.com/nodejs/node-gyp/releases/tag/v10.0.0 mentions

All internal functions have been coverted [sic] to return promises and no longer accept callbacks.

so forcing the version to ^10 is likely hanging forge because it's passing callbacks which are being ignored.

sparecycles avatar Apr 03 '24 18:04 sparecycles

@sparecycles what OS and version of node are you running? FWIW I'm using the latest version of electron-forge, electron, and better-sqlite3 and can build on ubuntu 22.04.4 LTS using node v20.11.1 and python 3.10.12 (I've probably run pip install setuptools as well)

mceachen avatar Apr 03 '24 20:04 mceachen

@mceachen OSX 14.4.1, node 20.11.1. The update to python 3.12 (base) specifically breaks node-gyp and I've tested and overriding the version of node-gyp to 10^ replaces the crash with a hang (with python reverted to 3.11 and everything else otherwise working).

Looks like https://github.com/electron/rebuild/pull/1128 is basically a one-line fix but it's stalled in the build process for whatever reasons. 99% sure this isn't specific to better-sqlite3, it just happens to be my only use-case for a native module and the stars aligned!

sparecycles avatar Apr 03 '24 21:04 sparecycles