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

chore: upgrade macos runner to macos-13 Ventura and xcode15 #13606

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

Swiftb0y
Copy link
Member

MacOS 14 is also already available. Updating from 12 to 13 should give us a nice boost in C++20 compatibility on MacOS.

@daschuer daschuer changed the title chore: upgrade macos runner to xcode15 chore: upgrade macos runner to macos-13 Ventura and xcode15 Aug 28, 2024
@JoergAtGithub
Copy link
Member

Will this increase the runtime requirements as well?

@daschuer
Copy link
Member

daschuer commented Aug 28, 2024

This virtually drops the still supported macOS Monterey (V 12) form 2021-08.
Only three month ago we have dropped macOS Big Sur (V 11) from 2020-11 because GitHub has dropped the runner support. It has fallen out of Apple's support 09.2023
#13296
Apple will release a new version once a year and usually maintains two older versions

Mixxx 2.6 is scheduled earliest at 2025.06 (after 2.5 + 60 day + beta) https://github.com/mixxxdj/mixxx/wiki/250_release_proposal

Conclusion:
Changing the runner now in main is fine, because Mixxx 2.6 will be released after macOS Monterey is dropped.

The other question is the XCode version. macOS 13.0 comes with XCode 14.1
XCode 15 requires macOS 13.5 with SDK macosx14.0 and has supports following deployment targets: macOS 10.13-14.2

Here is the readme describing the GitHub runner:
https://github.com/actions/runner-images/blob/main/images/macos/macos-13-Readme.md

So it looks like this will not limit the Mixxx binary support, but may be a barrier for contributors?
@fwcd what is your experience?

For now I think we should stick to the default pair macOS 13 + XCode 14.1
At least this would be an intermediate step and form there we may advance once we have a compelling reason

@daschuer
Copy link
Member

Since GitHub defaults to the max XCode version. We use now XCode 14.2 after the upgrade we will use XCode 15.2 unless we add:

DEVELOPER_DIR: /Applications/Xcode_14.3.1.app/Contents/Developer
/bin/bash -c "sudo xcode-select --switch /Applications/Xcode_14.3.1.app/Contents/Developer"

mixxxdj/vcpkg@e1076c9

@fwcd
Copy link
Member

fwcd commented Aug 28, 2024

Small note: It's capitalized "Xcode", not "XCode" :D

Will this increase the runtime requirements as well?

Not necessarily. In general, building with a newer SDK/Xcode without bumping the requirements should usually if not always be fine. For the App Store Apple even requires using the newest one IIRC. Due to ABI stability, the binary should still run on whichever macOS version our deployment target is set to, which in practice likely means we're bound to the oldest macOS version supported by Qt.

Building with a newer compiler also has the advantage that we get better C++20/23 support, so the only1 reason to build on an older macOS would be making sure that our build wasn't misconfigured somewhere (e.g. some dependency not built against the older deployment target).

My impression is that most devs on macOS are usually on the latest version anyway, so this likely doesn't really impact contributors much.

Footnotes

  1. Actually there was another reason, namely that there is a cpack bug, which occasionally crashes due to an hdiutil race condition on newer macOS versions, see https://github.com/actions/runner-images/issues/7522 and https://gitlab.kitware.com/cmake/cmake/-/issues/25671. Not sure what the status of this is and whether we still need to patch around this issue.

@daschuer
Copy link
Member

This is already in our mixxxdj/mixxx repository and notarization works. Does it mean we do not suffer the cited bug and we can just merge it?

@fwcd
Copy link
Member

fwcd commented Aug 28, 2024

Might be the case, I'm not really sure when this happens exactly, so we'll likely only find out after running the CI workflow on the new OS a bunch of times. We'll have to do it anyway eventually though, so I don't really see this as risky.

@Swiftb0y
Copy link
Member Author

I'm not fully familiar with the implications around the minimum deployment target implications, but I was under the impression that it should not depend on the runner OS. Thanks @fwcd for sharing your insight here. If it does not affect the target, I would really like to bump the macOS version as high as possible because macOS is usually the target with the oldest compiler anyway.

@fwcd
Copy link
Member

fwcd commented Aug 28, 2024

I think there is some merit to not using the latest runner, since we'll likely have few contributors using them and thus (if the build is misconfigured), we may unintentionally break backwards compatibility if we forget to set the deployment target somewhere. Maybe a more contrived option, but perhaps we could build on the newest macOS and then run the test suite on the older OS? Not sure if that would impose too much overhead though.

@daschuer
Copy link
Member

If, we have a runner to check backwards compatibility, we can IMHO use it right away, there is less risk that such a binary will crash after updating. That is the reason why we normally use the oldest runner. The bump here is OK, because the macos-12 runner will likely retired during 2.6-beta and this way we avoid a runner bump during beta. I remember some incidences where a runner update broke our CI.

Our CI is already quite "expensive" and has a certain CO² foodprint, so we should use it deliberately.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Ok, So it looks like an update is safe.

@daschuer daschuer merged commit f4785f4 into main Aug 29, 2024
23 checks passed
@Swiftb0y Swiftb0y deleted the chore/upgrade-macos13-xcode15.2 branch August 29, 2024 12:40
@Swiftb0y
Copy link
Member Author

Thanks

@daschuer
Copy link
Member

Sh... we need probably revert:

CPack Error: Error executing: /usr/bin/hdiutil create -ov -srcfolder "/Users/runner/work/mixxx/mixxx/build/_CPack_Packages/Darwin/DragNDrop/mixxx-2.6-alpha-77-gf4785f4720" -volname "mixxx-2.6-alpha-77-gf4785f4720" -fs "HFS+" -format UDRW "/Users/runner/work/mixxx/mixxx/build/_CPack_Packages/Darwin/DragNDrop/temp.dmg"
CPack Error: Error generating temporary disk image.
hdiutil: create failed - Resource busy

@fwcd
Copy link
Member

fwcd commented Aug 29, 2024

This is the bug I mentioned. We'll have to fix it anyway at some point, since the macos-12 runners likely won't be supported for that long either.

@fwcd
Copy link
Member

fwcd commented Aug 29, 2024

The issue seems to be that macOS's antivirus (XProtect) scans the disk image, which causes a race condition with CPack. The two main workarounds are killing the XProtect process (which is still a bit brittle, since it automatically relaunches) and simply rerunning CPack a few times until it succeeds. Neither of them are particularly elegant, but they seem to work "well enough" in practice, at least when used together.

If nothing else works, we could also package Mixxx as zip or tarball, but at least for the releases it would be nice to have a dmg, for consistency and to avoid having to hack around this in packages (e.g. Homebrew).

@daschuer
Copy link
Member

Both measures sounds like a workaround not a solution. Is there a ignore list or such that we can configure to not do the scan during Cpack?

In general we need an estimate how long a proper upstream solution will take and how stable our workaround is.

For now it seems to be more stable to revert main and test possible solutions on the https://github.com/mixxxdj/mixxx/tree/chore/upgrade-macos13-xcode15.2 branch. Is that OK?

@fwcd
Copy link
Member

fwcd commented Aug 29, 2024

Works for me! Admittedly I haven't looked into what upstream has been doing, but I do believe it's still a CPack bug

@Swiftb0y
Copy link
Member Author

For now it seems to be more stable to revert main and test possible solutions on the https://github.com/mixxxdj/mixxx/tree/chore/upgrade-macos13-xcode15.2 branch. Is that OK?

Sure... Though I wonder why this didn't happen before. explicitly pushed this branch upstream so the codesign CI would run... I guess the failure is spontaneous?

daschuer added a commit that referenced this pull request Aug 29, 2024
…xcode15.2"

This reverts commit f4785f4, reversing
changes made to 400ae46.
@daschuer
Copy link
Member

Done 1dc2119

@fwcd
Copy link
Member

fwcd commented Aug 29, 2024

Though I wonder why this didn't happen before. explicitly pushed this branch upstream so the codesign CI would run... I guess the failure is spontaneous?

It's a race condition, so it only happens sometimes. That's why retrying CPack is one of the workarounds.

@Swiftb0y
Copy link
Member Author

Ah right, yeah...

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Sep 22, 2024

image

We need to revive this soon!

@fwcd
Copy link
Member

fwcd commented Sep 22, 2024

This looks like a Homebrew message, not a deprecation from GitHub. It just means, they won't provide prebuilt packages (bottles) anymore, so our CI workflows will likely take longer as they have to build dependencies from scratch. They should still work though.

I agree that we'll have to revisit this, sooner or later, anyway though.

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.

4 participants