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 Apple Silicon Build #969

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tomadamatkinson
Copy link
Collaborator

@tomadamatkinson tomadamatkinson commented Mar 8, 2024

Description

Adds an Apple Silicon runner to help reduce the chances of #965. There are some considerations with this approach. macos-14 runners are in beta and so it may take considerable time to acquire a runner. Due to the beta and the possible slow provisioning times this might be more hassle than good.

It can't harm to add the runner on a trial basis. If we run into any significant issues we can comment out the newer runner until it is out of beta later in the year

gary-sweet
gary-sweet previously approved these changes Mar 11, 2024
gpx1000
gpx1000 previously approved these changes Mar 11, 2024
@SaschaWillems
Copy link
Collaborator

My only concern with this: It seems to take ages for the CI to acquire runners for Apple silicon. Dunno how to find out once CI/CD has finished, but on your initial run it took roughly half a day to get runner for the new targets. I kinda fear that this could heavily increase CI/CD for all PRs.

Is there a way to improve this?

@tomadamatkinson
Copy link
Collaborator Author

Completely agree @SaschaWillems. This is also my concern. Im thinking that we should run the macos-14 build only if the branch is main. That way the provisioning times shouldn't affect PRs

@tomadamatkinson tomadamatkinson dismissed stale reviews from gpx1000 and gary-sweet via d7bef0d March 11, 2024 20:06
@SaschaWillems
Copy link
Collaborator

Completely agree @SaschaWillems. This is also my concern. Im thinking that we should run the macos-14 build only if the branch is main. That way the provisioning times shouldn't affect PRs

That sounds like a good idea. I also don't think we need to run four different MacOS builds, like the initial PR did.

@tomadamatkinson
Copy link
Collaborator Author

That sounds like a good idea. I also don't think we need to run four different MacOS builds, like the initial PR did.

One is Macos amd64 and the other is Macos arm64. Theres still some value there until macos-14 is in general release. We could merge the Build and Test V2 into the build job which save us some runners. I kept this separate to try and encourage moving more of the framework into modules (not quite there yet :) )

On this PR we can see this job is now skipped https://github.com/KhronosGroup/Vulkan-Samples/actions/runs/8239111840?pr=969

On the main of my fork we can see it runs the apple silicon job https://github.com/tomadamatkinson/Vulkan-Samples/actions/runs/8239110561

Should this PR tackle getting rid of duplicate builds? I dont mind doing that, just trying to keep the scope small

- uses: actions/checkout@v3
with:
submodules: "recursive"
- if: ${{ matrix.platform.os == 'ubuntu' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

If I get it right, this part is only used with platform = { macos, macos-14 }?
That is, all those matrix.platform.os == 'ubuntu' or 'windows' parts should not be needed here?

strategy:
matrix:
platform: [windows, ubuntu, macos]
platform: [{ os: windows, runtime: windows-latest }, { os: ubuntu, runtime: ubuntu-latest }, { os: macos, runtime: macos-12 }]
build_type: [Debug, Release]
env:
PARALLEL: -j 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, instead of duplicating most of this file, you could add macos-14 to the matrix and just have a condition here like
if: ${{ github.ref == 'refs/heads/main' || matrix.platform.runtime != 'macos-14' }}

@gpx1000
Copy link
Contributor

gpx1000 commented Mar 19, 2024

I've been thinking about this. I believe iOS is arm64, could we count the CI for that as evidence that macos-14 should work? There's very little differences between the two from a perspective of what gets built.

@jeroenbakker-atmind
Copy link
Contributor

jeroenbakker-atmind commented Apr 10, 2024

My main dev setup for VulkanSamples related development is M1 Studio + M2 Air which I compile 3 times a week on both systems. I can keep track and responsibility for these kind of issues. The M1 can be setup as PR builder and I could script that it runs and report problems to the PR.

I read that I could also set it up as a github runner for this repo specific. Availability would be 16 hours during working days.

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.

Update astc version (#951) errors on linking on a M1 Mac
6 participants