-
-
Notifications
You must be signed in to change notification settings - Fork 161
Remove hard-coded better-sqlite3 binaries #294
Conversation
88ea25a
to
5295d95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had a chance to test yet, but overall I'm a big fan of not hard-coding and not relying on custom build scripts so this is looking great. Just had a couple small questions in the review.
This looks great! it irked me a bit when I had to add the mac arm .node file to get that build working but I didn't see an alternative when OG trilium uses better-sqlite3 8.4.0 with electron 25 (for which there is no precompiled binary in the better-sqlite3 upstream.) Now that dependencies are updated and available as blobs, this approach seems much simpler. Some notes about the mac builds
|
Thanks for checking the mac builds! I have fixed them, you can check whether they are fixed. For the arch config, I think it won't work to config in |
I just ran the updated commits and CI scripts locally on mac and it works great except for one thing that's actually my fault...
I typoed, it should be '$buildPath/${appName}.app/Contents/Resources' (with an s after Content) Other than that it works flawlessly. |
I'll merge this and I can push a fix directly on @JYC333 , thank you for your contributions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Using
electron-forge
to pack the installer for all platforms expect for linux server.Couldn't test with MacOS since I don't have Mac and ARM device, maybe someone could help with testing the mac installer and arm64 build.
Webpack warning fixed could also be updated later if this is merged. TypeStrong/ts-node#2073
Related issue TriliumNext/trilium#5077 TriliumNext/trilium#4970