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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hard-coded better-sqlite3 binaries #294

Merged
merged 11 commits into from
Aug 10, 2024
Merged

Conversation

JYC333
Copy link

@JYC333 JYC333 commented Aug 5, 2024

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 #229 #80

@JYC333 JYC333 requested a review from eliandoran August 5, 2024 18:33
@eliandoran eliandoran force-pushed the develop branch 2 times, most recently from 88ea25a to 5295d95 Compare August 7, 2024 19:25
Copy link

@zerebos zerebos left a 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.

.vscode/settings.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@gigamonster256
Copy link

gigamonster256 commented Aug 8, 2024

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

  1. the afterComplete function assumes that the extra resources are located at '$buildPath/resources' when on mac builds it's located at '$buildPath/${appName}.app/Content/Resources'. This causes the build to error since the path is wrong. However, the files being copied correctly don't even make it into the dmg or zip at the root level (although they are present inside of the .app file) and might as well be ignored.

  2. The forge maker-dmg doesn't respect the arch in the forge.config.js (as far as I can tell) so running npm run make-electron only generates an arm64 build on my machine (so I suspect the CI script will not compile both architectures)

  3. The dmg created is located at out/make/${appName}-${packageJSON.version}-${targetArch}.dmg not out/make/dmg/${targetArch}/*.dmg like the CI script specifies

@JYC333
Copy link
Author

JYC333 commented Aug 9, 2024

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

  1. the afterComplete function assumes that the extra resources are located at '$buildPath/resources' when on mac builds it's located at '$buildPath/${appName}.app/Content/Resources'. This causes the build to error since the path is wrong. However, the files being copied correctly don't even make it into the dmg or zip at the root level (although they are present inside of the .app file) and might as well be ignored.
  2. The forge maker-dmg doesn't respect the arch in the forge.config.js (as far as I can tell) so running npm run make-electron only generates an arm64 build on my machine (so I suspect the CI script will not compile both architectures)
  3. The dmg created is located at out/make/${appName}-${packageJSON.version}-${targetArch}.dmg not out/make/dmg/${targetArch}/*.dmg like the CI script specifies

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 forge.config.js. It has to pass when you run the command like npm run make-electron -- --arch="arm64", so I update the scripts in CI for build both arches.

@JYC333 JYC333 requested a review from zerebos August 9, 2024 20:31
@gigamonster256
Copy link

gigamonster256 commented Aug 9, 2024

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...

'$buildPath/${appName}.app/Content/Resources'

I typoed, it should be '$buildPath/${appName}.app/Contents/Resources' (with an s after Content)

Other than that it works flawlessly.

@eliandoran
Copy link
Contributor

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...

'$buildPath/${appName}.app/Content/Resources'

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 develop.

@JYC333 , thank you for your contributions!

Copy link
Contributor

@eliandoran eliandoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@eliandoran eliandoran merged commit 061b0c9 into TriliumNext:develop Aug 10, 2024
@JYC333 JYC333 deleted the sql branch August 10, 2024 08:24
@eliandoran eliandoran added this to the v0.90.4 milestone Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants