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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for build failure: electron 29.0.0-alpha.7 #1127

Closed
sparecycles opened this issue Jan 12, 2024 · 7 comments
Closed

fix for build failure: electron 29.0.0-alpha.7 #1127

sparecycles opened this issue Jan 12, 2024 · 7 comments

Comments

@sparecycles
Copy link

Hi! 馃憢

Firstly, thanks for your work on this project! 馃檪

Today I used patch-package to patch better-sqlite3@9.2.2 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.

@mceachen
Copy link
Member

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

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

@LaGregance
Copy link

Hi,
I got the same bug with electron@29.1.4 and better-sqlite3@9.4.3.

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

@pelletier197
Copy link

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.

@JoshuaWise
Copy link
Member

Fixed by #1151

Sorry I missed this one!

@sparecycles
Copy link
Author

sparecycles commented Apr 3, 2024

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 better-sqlite3@9.4.3 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.

@mceachen
Copy link
Member

mceachen commented Apr 3, 2024

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

@sparecycles
Copy link
Author

@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 electron/rebuild#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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants