TriliumNext / Notes

Build your personal knowledge base with TriliumNext Notes
GNU Affero General Public License v3.0
740 stars 37 forks source link

Remove hard-coded better-sqlite3 binaries #294

Closed JYC333 closed 1 month ago

JYC333 commented 1 month ago

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. https://github.com/TypeStrong/ts-node/pull/2073

Related issue #229 #80

gigamonster256 commented 1 month ago

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 commented 1 month ago

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.

gigamonster256 commented 1 month ago

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 commented 1 month ago

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!