MCMi460 / NSO-RPC

Connect your Nintendo Switch playing status to Discord!
327 stars 32 forks source link

macOS specific build tweaks #150

Closed spotlightishere closed 5 months ago

spotlightishere commented 5 months ago

This includes two changes to how builds are approached for macOS:


These changes should permit for macos-latest to be used again for both our pure x86_64 and universal2 builds. (See further discussion within this pull request for how this was achieved.)

As an aside, I wonder if we should continue with pure x86_64 builds anymore. As macOS continues its migration towards arm64, the advantage of a smaller size may not be worth maintaining separately.

spotlightishere commented 5 months ago

While debugging this, I noticed that macOS tests have silently been failing for quite a while. I'm unclear on why invoking the NSO-RPC binary itself led to immediate termination (seemingly due to Python terminating itself, nothing AMFI?) but switching to the open command appears to function as desired.

HotaruBlaze commented 5 months ago

These changes should permit for macos-latest to be used again, but unfortunately the x86_64 builder cannot yet switch due to arm64 Python always being preferred via actions/setup-python.

We cant use latest unless we drop x64 completely, lowest we can do is macos-13, but macOS kept failing to bundle the app with "resource busy", the recommended workarounds didn't work: https://github.com/actions/runner-images/issues/7522 MacOS-14 IS arm64 only, officially. so I doubt we can workaround that

While debugging this, I noticed that macOS tests have silently been failing for quite a while. I'm unclear on why invoking the NSO-RPC binary itself led to immediate termination (seemingly due to Python terminating itself, nothing AMFI?) but switching to the open command appears to function as desired.

Reading this I noticed another flaw that we still aren't checking, we should modify the test that to check if the output.txt files exist, if not we should force fail the test, as it means it didn't execute, as the current test is pass if no match.

To continue from the test issue, it DID work to catch launch errors, so id assume theirs something wrong with python itself, because it worked before: https://github.com/HotaruBlaze/NSO-RPC/actions/runs/9088761689/job/24978856557

Above is from me making a bad change before noticing I could make it cleaner.


Apart from that, i don't see any massive issues with these changes.

Edit: Clarified more on why macos-latest isn't valid for our use case,

spotlightishere commented 5 months ago

We cant use latest unless we drop x64 completely [...] macOS kept failing to bundle the app with "resource busy"

I was unable to encounter that issue while testing, thankfully! However, it did prefer the arm64 PyQt6_Qt6 wheel while building, hence the arch -x86_64 changes. We'd have to manually install an x86_64-only version of Python (possibly copying what actions/setup-python does?) to keep the smaller size due to setup-python always pulling in the native runner architecture. Beyond that, it should be viable for our use. I'd be happy to investigate this before merging this PR.


Regarding tests, that is quite odd! I wasn't able to reproduce the immediate termination locally, but couldn't find a way to get good logs out of GitHub's runners to diagnose why Python was terminating itself. I'd be happy to modify that check as well.

HotaruBlaze commented 5 months ago

Regarding tests, that is quite odd! I wasn't able to reproduce the immediate termination locally, but couldn't find a way to get good logs out of GitHub's runners to diagnose why Python was terminating itself. I'd be happy to modify that check as well.

This may help you, Note it only works on runners on your personal fork, u cant enable it on MCMi460/NSO-RPC's actions tests: image

HotaruBlaze commented 5 months ago

We cant use latest unless we drop x64 completely [...] macOS kept failing to bundle the app with "resource busy"

This only happens on macos-13. macos-14 builds but the x64 build does not execute correctly, Suspected to be due to it doing a mix of universal2 and x64 causing a broken build. Your latest changes may have fixed it however It was why I actually initially added the test as a way to catch compile errors.

spotlightishere commented 5 months ago

We'd have to manually install an x86_64-only version of Python [...] due to setup-python always pulling in the native runner architecture.

Upon further review, there's an architecture option available! This seems to produce a functional x86_64 only build as of 6f1b13c7eb9225033f16eaa8b0093787663627f0. Ironically, the x86_64 only build is currently larger than the universal2 build - seemingly due to it including all Qt frameworks. Would it be worth debloating it as well, or possibly removing the x86_64 build entirely?

HotaruBlaze commented 5 months ago

We'd have to manually install an x86_64-only version of Python [...] due to setup-python always pulling in the native runner architecture.

Upon further review, there's an architecture option available! This seems to produce a functional x86_64 only build as of 6f1b13c. Ironically, the x86_64 only build is currently larger than the universal2 build - seemingly due to it including all Qt frameworks. Would it be worth debloating it as well?

Yeah, we should probably debloat it, or atleast check the size differences. I remember it making a big difference on universal2

HotaruBlaze commented 5 months ago

Im kinda eh on making a universal2 build actually have both arch's in it, but it may just be for the best.

Maybe we should do a multi-arch debloated build as a test? since my main concern is it actually working and if we can cut down on the total size, may be worth it.

spotlightishere commented 5 months ago

The current universal build contains both architectures - that's what universal2 is intended to be used for! I'm able to launch it under both arm64 and x86_64 and have its presence function correctly. Are any components currently not functional under an architecture for you?

HotaruBlaze commented 5 months ago

universal used to have a quirk where Qt didnt work at all for me, also our current universal2 build will only work on universal2/arm, i dont believe it actually bundles any x64 stuff inside it currently.

spotlightishere commented 5 months ago

It seems that CI was erroneously using Python 3.11 as installed via GitHub's built-in packages, who only installs for arm64/x86_64 and not universal2. 2722564d497e1bbd4f3fcef92402ee95b49bcf5f should have us directly use Python as installed via the package and appears to function under both architectures. (Thank you for noticing this, @HotaruBlaze!)

MCMi460 commented 5 months ago

If universal works on x86_64 machines as well, now, then we might as well remove x86_64 builds since they'll just clutter and confuse.

HotaruBlaze commented 5 months ago

yeah, the new universal2 builds work on x64, I'm fine with getting rid of it

HotaruBlaze commented 5 months ago

Do we want to disable the dedicated x64 builds here, or do we want to do it in another PR?

spotlightishere commented 5 months ago

Apologies - have been quite busy recently, sorry for not following up on this sooner! We should probably merge this as-is and remove in a second pull request so that the x86_64 fixes are available in commit history.

HotaruBlaze commented 5 months ago

Alright, I'll deal with it once this is merged into development, As I plan to split up the actions and prevent the PR tests being as spammy.

MCMi460 commented 5 months ago

Thank you! :)