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

feat: kubo 0.29 and native apple silicon #1856

Merged
merged 13 commits into from
Jun 12, 2024
Merged

Conversation

lidel
Copy link
Member

@lidel lidel commented Jun 23, 2021

This is exploration into better support of Apple Silicon.

electron-builder supports producing either universal package or one for each architecture.

In case of Desktop, things are more complicated: we also bundle go-ipfs binary.
This means that unless the go-ipfs binary is also universal or per-arch, we can't improve much.

Due to the way we depends on go-ipfs via npm-go-ipfs universal binaries all the way sound like way easier route (downside being the disk size), however dist.ipfs.io already has amd64 and arm64 for darwin.

Universal binary test

Main cost of universal package is size, even with out go-ipfs (still being amd64) it adds ~75 MB:

  • 195M IPFS Desktop-0.16.0-universal.dmg (electron amd64&arm64 + go-ipfs arm64)
  • 119 MB IPFS-Desktop-0.16.0.dmg (amd64, M1 will use Roseta)

TODO

Closes #2813

powershell.exe -NoProfile -NonInteractive thing should be also fixed by electron-userland/electron-builder#7127 (comment), so:

Closes #2335
Closes #2636
Closes #2752
Closes #2778
Closes #2779
Closes #2780
Closes #2784

@lidel lidel added the area/macos MacOS label Jun 23, 2021
@BigLep BigLep added this to the go-ipfs 0.10 milestone Jul 8, 2021
@AlexxNica
Copy link

@lidel, do we know where the difference in size comes from, specifically? We could find where they are and try something to minimize the impact of shipping the universal package.

@unbeatable-101
Copy link

@lidel, do we know where the difference in size comes from, specifically? We could find where they are and try something to minimize the impact of shipping the universal package.

Not a dev, but I'm pretty sure the size increase is just the binaries about doubling in size due to basically containing two apps.

@jenkijo
Copy link

jenkijo commented Sep 22, 2021

Hello, any update?
I've committed fix for go-ipfs ipfs/npm-kubo#44

@dvcrn
Copy link

dvcrn commented Sep 28, 2021

Thanks for the PR! I wanted to build this locally but ran into issues with the build step trying to download a arm ipfs binary that obviously didn't exist. How did you build this?

@jenkijo
Copy link

jenkijo commented Oct 3, 2021

It work fine with me, what error that you got?

@hacdias
Copy link
Member

hacdias commented Mar 24, 2022

I think that Universal would be easier for the end users despite the storage size. In general, storage is cheap and 75 MB won't probably affect many people.

Iff storage size is very important, we could either build universal + download go-ipfs on first run, or ship different binaries.

@SgtPooki
Copy link
Member

SgtPooki commented May 6, 2022

@lidel Have we looked into https://github.com/tauri-apps/tauri at all? It claims significant improvements over electron across all platforms, though I haven't investigated more deeply. I also haven't found any apps that have migrated from electron to tauri

@SgtPooki
Copy link
Member

SgtPooki commented May 6, 2022

Regarding universal app vs rosetta on M1.. the speed increases you get by running M1 native is significant in every benchmark I've seen. However, it would be great to see a breakdown of the risks & benefits for our specific application.

I don't think we should decide to just force rosetta forevermore. If the decision is to wait for some period until we have more bandwidth, I think sticking with rosetta and the smaller bundle for now is fine. Ultimately, we should be moving to the type of app bundle the platform we're targeting recommends.

For Apple/macOS, that is the universal build.

@hacdias
Copy link
Member

hacdias commented May 6, 2022

@SgtPooki an issue you may want to look at too is #867

@lidel
Copy link
Member Author

lidel commented May 16, 2022

Have we looked into https://github.com/tauri-apps/tauri

I think we looked at it briefly, but it was very alpha back then. IIRC the main downside was lack of tooling for end-to-end autoupdate solution, which is raison d'être of ipfs-desktop. It seems they work on it, but it is still undocumented? tauri-apps/tauri#2776

Long term, we want to move away from fetching updates from centralized HTTP server like github and leverage IPFS+IPNS instead (see old notes in #789), so if we have working setup for that, we could move away from Electron (literally the only value it brings is https://www.electron.build/auto-update working on all three platforms: macOS+Windows+Linux(AppImage))

75 MB won't probably affect many people.

Usually I would be against doubling the size of download, but we talk Apple here – it is fair to assume that if someone uses mac they most likely don't care about additional download cost over the mobile carrier (just being realistic here).

Ultimately, we should be moving to the type of app bundle the platform we're targeting recommends.
For Apple/macOS, that is the universal build.

Yeah, sounds like universal binary is the way to go. The caveat here is that we don't have "universal go-ipfs binary" – iiuc this PR (in current form) still bundles the intel version of go-ipfs.

Figuring out how to include both binaries (arm64 and amd64) for darwin is the key to unblocking this.
Someone familiar with Apple hardware and ecosystem needs to investigate if ipfs-desktop should fetch independent binaries during the build of DMG, or if dist.ipfs.io website should add universal binary upstream (if it is even possible for standalone CLI binaries).

@lidel
Copy link
Member Author

lidel commented Nov 28, 2022

I wrote a plan for unblocking this in ipfs/distributions#770 (comment).
Not a macOS user myself, but if someone wants to work on this, I'm around to advise.

@ipfs ipfs deleted a comment from Whitedragon2772 Nov 28, 2022
@lidel lidel added the status/blocked Unable to be worked further until needs are met label Nov 28, 2022
@SgtPooki
Copy link
Member

SgtPooki commented Dec 7, 2022

I wrote a plan for unblocking this in ipfs/distributions#770 (comment).

Not a macOS user myself, but if someone wants to work on this, I'm around to advise.

I read that comment and the approach seems reasonable and clear. Thanks for the backlink!

@whizzzkid
Copy link
Contributor

@lidel should this be closed as ipfs/distributions#770 (comment) is closed.

I was also wondering if this could be solved as part of #2441 where instead of "shipping" a binary, we download and validate checksums and then use that?

@lidel
Copy link
Member Author

lidel commented Jul 7, 2023

There are two separate topics at play (mind I am not macOS user, below is my rough understanding on Apple Silicon situation)

  • architecture of go binary (kubo daemon)
  • architecture of electron app (ipfs desktop)
    • this feels unresolved, but maybe not a big deal, since the majority of work happens in kubo?
    • iiuc what happens on macOS is that we produce DMG that is amd64 and when run on Apple Silicon it runs emulation

I agree we should close this, as universal DMG does not solve it end-to-end.
If we want to use native arch on macOS, need to fill new issue and make a new plan for both topics above.

@SgtPooki
Copy link
Member

please see #2681 for a continuation of this work

@lidel lidel deleted the feat/apple-silicon-native-build branch October 18, 2023 21:13
@lidel lidel restored the feat/apple-silicon-native-build branch May 16, 2024 14:59
@lidel
Copy link
Member Author

lidel commented May 16, 2024

Reopening because of #2813

I will prototype Universal DMG that includes Universal Kubo binary as well.

@lidel lidel reopened this May 16, 2024
@lidel lidel mentioned this pull request May 27, 2024
9 tasks
modern linux image at github includes it out of the box
and we use xvfb-maybe internally to use it when appropriate
@lidel lidel force-pushed the feat/apple-silicon-native-build branch from ac7b752 to 507baa5 Compare June 7, 2024 17:09
if this still passes on github, means they set development mode
on their images, which allows for making symlinks without admin
on windows-latest
@lidel lidel marked this pull request as ready for review June 7, 2024 18:47
@lidel lidel requested review from SgtPooki, whizzzkid and a team as code owners June 7, 2024 18:48
@lidel lidel removed the request for review from whizzzkid June 7, 2024 18:48
@lidel lidel force-pushed the feat/apple-silicon-native-build branch from bf87528 to 1217b7e Compare June 7, 2024 19:01
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

Should this be version 0.36.0? code changes look fine. app shows universal. but I tried running the app (from latest CI job) and it crashes immediately.

image
System Integrity Protection: enabled

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGBUS)
Exception Codes:       KERN_PROTECTION_FAILURE at 0x0000007483f72aa9
Exception Codes:       0x0000000000000002, 0x0000007483f72aa9

Termination Reason:    Namespace SIGNAL, Code 10 Bus error: 10
Terminating Process:   exc handler [24263]

VM Region Info: 0x7483f72aa9 is in 0x74005c0000-0x7500000000;  bytes after start: 2207984297  bytes before end: 2080953686
      REGION TYPE                    START - END         [ VSIZE] PRT/MAX SHRMOD  REGION DETAIL
      Memory Tag 255             7400540000-74005c0000   [  512K] rw-/rwx SM=ZER  
--->  Memory Tag 255             74005c0000-7500000000   [  4.0G] ---/rwx SM=ZER  
      Memory Tag 255             7500000000-7500004000   [   16K] rw-/rwx SM=ZER  

.github/CODEOWNERS Outdated Show resolved Hide resolved
@lidel
Copy link
Member Author

lidel commented Jun 10, 2024

@SgtPooki yes, this will be released as 0.36.0 (2e891f3 for release-please, it will open release PR once this one is merged).

But the crash is concerning.

  • Are you on Intel or Apple Silicon machine?
  • What is the version of macOS?
  • Is it crashing every time?
  • Was it crashing with RC 2 (.zip of build here)?

I can get my hands on M1 macMini tomorrow, but having second data point would be useful.

@SgtPooki
Copy link
Member

SgtPooki commented Jun 10, 2024

  • Are you on Intel or Apple Silicon machine?

Apple silicon

  • What is the version of macOS?

Sonoma 14.5 (23F79)

  • Is it crashing every time?

Yep.

I can get my hands on M1 macMini tomorrow, but having second data point would be useful.

that version is also crashing for me immediately. I remember a previous version you asked me to test a month ago or so worked fine. If that's the same one, it's not now and the problem is my system.


It seems like it might be related to me using nix-shell for nodejs and it's not letting the kubo binary download

@@ -77,7 +78,7 @@
"countly-sdk-nodejs": "^20.11.0",
"electron-serve": "^1.1.0",
"electron-store": "^8.1.0",
"electron-updater": "^5.3.0",
"electron-updater": "^6.2.1",
Copy link
Member Author

@lidel lidel Jun 12, 2024

Choose a reason for hiding this comment

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

ℹ️ This was done mostly to ensure macos update works fine on ARM, but also includes fix for powershell.exe -NoProfile -NonInteractive errors:
electron-userland/electron-builder#7127 (comment)

@lidel
Copy link
Member Author

lidel commented Jun 12, 2024

Thanks, sounds like it.

FWIW I've just tested both on M1 macMini and both work ok. Let's merge and ship.

If crashes of IPFS Desktop binary (electron) rather than kubo itself become common, then the first thing we should do is to upgrade to supported version of Electron (#2775)

@lidel lidel merged commit 0cd87ab into main Jun 12, 2024
7 checks passed
@lidel lidel deleted the feat/apple-silicon-native-build branch June 12, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment