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

macOS specific build tweaks #150

Merged
merged 7 commits into from
Jun 2, 2024

Conversation

spotlightishere
Copy link
Collaborator

@spotlightishere spotlightishere commented May 16, 2024

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

  • Under our universal2 builds, we prepare our universal PyQt6_Qt6 wheel first, and then install PyQt6 (the architecture-agnostic wrapper) and subsequent packages beyond. This avoids some architectural mismatch when building underneath x86_64.
  • Under our pure x86_64 builds we leverage arch -x86_64 to ensure that, regardless of host architecture, the version of Python run is x86_64. Under native x86_64 hosts this does nothing, but it ensures Rosetta is leveraged under arm64 hosts, preventing architectural mismatch.

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.

This additionally skips building the wheel if it has already been built (e.g. for rapid development).
This ensures that x86_64 is leveraged for our pure x86_64 builds even on arm64 runners.
@spotlightishere
Copy link
Collaborator Author

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.

steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: 3.11.4
- name: Install Python 3.11.4 and build NSO-RPC
Copy link
Contributor

Choose a reason for hiding this comment

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

May be a good idea to remove "and build NSO-RPC" from this, as you have a dedicated build step below.

@HotaruBlaze
Copy link
Contributor

HotaruBlaze commented May 16, 2024

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: actions/runner-images#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
Copy link
Collaborator Author

spotlightishere commented May 17, 2024

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
Copy link
Contributor

HotaruBlaze commented May 17, 2024

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
Copy link
Contributor

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
Copy link
Collaborator Author

spotlightishere commented May 17, 2024

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, or possibly removing the x86_64 build entirely?

@HotaruBlaze
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

spotlightishere commented May 17, 2024

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
Copy link
Contributor

HotaruBlaze commented May 17, 2024

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
Copy link
Collaborator Author

spotlightishere commented May 17, 2024

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. 2722564 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
Copy link
Owner

MCMi460 commented May 19, 2024

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
Copy link
Contributor

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

@HotaruBlaze
Copy link
Contributor

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

@spotlightishere
Copy link
Collaborator Author

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
Copy link
Contributor

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 MCMi460 merged commit 149d240 into MCMi460:development Jun 2, 2024
6 checks passed
@MCMi460
Copy link
Owner

MCMi460 commented Jun 2, 2024

Thank you! :)

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.

3 participants