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

Add support for Apple Silicon / Universal build target for macOS #469

Closed
belthesar opened this issue Jul 26, 2023 · 10 comments · Fixed by #470
Closed

Add support for Apple Silicon / Universal build target for macOS #469

belthesar opened this issue Jul 26, 2023 · 10 comments · Fixed by #470
Labels
improvement Improvement on existing feature

Comments

@belthesar
Copy link
Contributor

Hi there! Ontime is a rather strikingly powerful project! I was looking to build something similar for a project I'm working on, and I was quite surprised to see such a well built tool that hits almost every use case I needed!

I use a M1 MacBook Pro as my laptop. While Ontime appears to run through Rosetta2 reasonably well, having a native version would improve the performance on that platform. I don't personally plan on using Ontime for live usage from my laptop on my next project, but there's a chance I feasibly would, and having maximum runtime and performance on the platform would be a boon to trusting using Ontime on the platform.

I'm not familiar enough with the stack to know what the level of effort of adding an Apple Silicon build target is, but given that it appears Ontime uses Github Actions for builds, I do at least know that Actions can be used to cross-compile for aarch64 on the x86 Mac runners. Thanks!

@belthesar
Copy link
Contributor Author

Did a little sleuthing on a fork to see if I could figure this out. It appears that electron-builder can use the --universal flag for macOS targets, producing a Universal binary (x64 and aarch64). I updated apps/electron/package.json to use this flag, and can successfully build a macOS Universal Binary package, however I don't have the means to test it. Using the build:local target generates a docker.cjs file that does not run under the app context (shotgunning a symlink from docker.cjs to index.cjs throws errors, I'm assuming due to other considerations for running an electron app vs. running the server as a node app). I'm unable to build a release target because I don't have your Sentry API key. I might try replacing the Sentry DSN and generate an API key to my test instance to get around this in the short term, but barring any other weirdness, I expect the build will run as is with the change. If I end up actually getting a reliable packaged build to test, I'll submit a PR with the appropriate change.

@cpvalente
Copy link
Owner

Hi @belthesar, this is a good observation, thank you for this.
As you found, we can pass the architecture in the electron builder config
electron-userland/electron-builder#5689 (comment)

It is unclear to me whether a universal build would be preferable to just dropping x64 support or even distributing two files.
I wonder whether the double file size could give the impression that the app is "bloated" in the macOS version

As for the build.
The build:local command is supposed to go around the lack of key, it simply sets a local environment variable which signals vite to build the app without sentry https://github.com/cpvalente/ontime/blob/master/apps/client/vite.config.js#L17

I wonder however if there is something wrong with the turbo repo scripts, you could try to recreate the steps manually:

  • install dependencies pnpm i
  • navigate to the react app apps/client
  • build app without sentry pnpm build:local
  • navigate to the backend apps/server
  • build app pnpm build
  • navigate to the electron app apps/electron
  • create package pnpm dist-mac

Let me know if this works for you

Since Ontime hits almost everything you need, you are welcome to contribute with code on what you feel the app does not achieve. It would be great to discuss this

Best

@cpvalente cpvalente added the improvement Improvement on existing feature label Jul 26, 2023
@belthesar
Copy link
Contributor Author

Hi @cpvalente, thanks for the response!

I agree, a build that hits almost 500 MB is less than ideal. I did see that it looks like electron-builder seems to use --arm64 for aarch64 targets, so setting up two separate builds for macOS x64 or macOS aarch64 would probably be best. Up to you, but I know there's still plenty of folks still running Intel Macs, and I have seen this pattern for other Electron OSS projects. Perhaps leveraging your Sentry telemetry to decide on a reasonable end of support date for macOS x64 is the best course of action.

I ended up modifying the dist-mac script to target aarch64, and by following the pnpm build order you suggested, was able to build a functional Apple Silicon native version of Ontime! I don't have time to put it through its paces right now, but startup time and overall responsiveness are significantly improved already.

I'm certainly in unfamiliar territory working with node, but with the way it appears the script works when executing build:local at root, my hunch is that it passes build:local at both the client and the server apps, which does not follow the steps you outlined.

As for what Ontime is missing, I'm honestly not sure if it's in-scope for the app, but we can certainly discuss that outside of the context of this issue.

Thanks again for the response!

@cpvalente
Copy link
Owner

Hi @belthesar,
This is great!

I think your assertion with build:local is correct and I will see to fix it.

If you want to talk about application scope I am all ears, get in touch at mail@getontime.no

@cpvalente
Copy link
Owner

HI @belthesar , would you mind taking a look at the artifacts from the new build?
I am unable to run the arm64 package in my M1 Pro while the other works fine.

Am I missing something?
https://github.com/cpvalente/ontime/releases/tag/v2.3.9

@belthesar
Copy link
Contributor Author

Hey there! Wanted to say that I've seen this but haven't been able to get back to this quite yet. Hopefully will do this week.

@cpvalente
Copy link
Owner

No worries, this is not visible to the users yet. Thank you again for your help

@belthesar
Copy link
Contributor Author

belthesar commented Aug 15, 2023

Can confirm that the build from CI doesn't work, but my local build does. Huh.

Edit: Looks like the download is flagging as quarantined, but doing a right click -> open to bypass quarantine is not unquaraning the bundle. Looking closer at the build, it seems like maybe there might be a codesigning issue?

┌─[ /Applications ]
└─▪ codesign -dv --verbose=4 ontime.app
Executable=/Applications/ontime.app/Contents/MacOS/ontime
Identifier=Electron
Format=app bundle with Mach-O thin (arm64)
CodeDirectory v=20400 size=513 flags=0x20002(adhoc,linker-signed) hashes=13+0 location=embedded
VersionPlatform=1
VersionMin=720896
VersionSDK=787200
Hash type=sha256 size=32
CandidateCDHash sha256=2b05eb90edf91092c00242056dc53b06e839640b
CandidateCDHashFull sha256=2b05eb90edf91092c00242056dc53b06e839640b1136b74c56c59b84ed5a145f
Hash choices=sha256
CMSDigest=2b05eb90edf91092c00242056dc53b06e839640b1136b74c56c59b84ed5a145f
CMSDigestType=2
Executable Segment base=0
Executable Segment limit=16384
Executable Segment flags=0x1
Page size=4096
Launch Constraints:
        None
CDHash=2b05eb90edf91092c00242056dc53b06e839640b
Signature=adhoc
Info.plist=not bound
TeamIdentifier=not set
Sealed Resources=none
Internal requirements=none
┌─[ /Applications ]
└─▪ codesign -dv --verbose=4 ontime\ 3.app
ontime 3.app: code object is not signed at all

@cpvalente
Copy link
Owner

Can confirm that the build from CI doesn't work, but my local build does. Huh.

Edit: Looks like the download is flagging as quarantined, but doing a right click -> open to bypass quarantine is not unquaraning the bundle. Looking closer at the build, it seems like maybe there might be a codesigning issue?

┌─[ /Applications ]
└─▪ codesign -dv --verbose=4 ontime.app
Executable=/Applications/ontime.app/Contents/MacOS/ontime
Identifier=Electron
Format=app bundle with Mach-O thin (arm64)
CodeDirectory v=20400 size=513 flags=0x20002(adhoc,linker-signed) hashes=13+0 location=embedded
VersionPlatform=1
VersionMin=720896
VersionSDK=787200
Hash type=sha256 size=32
CandidateCDHash sha256=2b05eb90edf91092c00242056dc53b06e839640b
CandidateCDHashFull sha256=2b05eb90edf91092c00242056dc53b06e839640b1136b74c56c59b84ed5a145f
Hash choices=sha256
CMSDigest=2b05eb90edf91092c00242056dc53b06e839640b1136b74c56c59b84ed5a145f
CMSDigestType=2
Executable Segment base=0
Executable Segment limit=16384
Executable Segment flags=0x1
Page size=4096
Launch Constraints:
        None
CDHash=2b05eb90edf91092c00242056dc53b06e839640b
Signature=adhoc
Info.plist=not bound
TeamIdentifier=not set
Sealed Resources=none
Internal requirements=none
┌─[ /Applications ]
└─▪ codesign -dv --verbose=4 ontime\ 3.app
ontime 3.app: code object is not signed at all

A-ha, that sounds about right. It might well be that Apple is pushing all the apps to be signed.
Ontime is not.

electron-userland/electron-builder#5850

As a free tool we are unable to support the costs for notarization, do you happen to know if there are ways around?

@belthesar
Copy link
Contributor Author

Once can remove the quarantine attribute from the app bundle by running the command xattr -d com.apple.quarantine /Applications/ontime.app in a terminal. This has to be done by the user and can't be done by CI, as the target system is what applies the com.apple.quarantine attribute to the files as they're being copied from the disk image. Users will also need to make sure the setting in Privacy and Security allows for applications downloaded from both the App Store and identified developers.
image

Once I removed the quarantine attributes from the bundle, the bundle launched fine. For other macOS apps, you can bypass Gatekeeper and remove the quarantine attribute by doing a Right/Two Finger/Option+Click on the bundle and click Open. However, I've noticed that some Electron apps don't work this way. There may be some guidance in Turbo and/or Electron's repos that might help with this, not sure.

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

Successfully merging a pull request may close this issue.

2 participants