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 aarch64 Mac runners to test_python, restore PSP_ARCH dev behavior #2802

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

tomjakubowski
Copy link
Contributor

@tomjakubowski tomjakubowski commented Oct 23, 2024

This PR follows up #2798. We weren't previously testing the aarch64 wheel in CI. This adds a macos-14 runner, which is aarch64 native.

I also restored the previous dev environment behavior when PSP_ARCH is not set: instead of a fatal build error, the build just doesn't set CMAKE_OSX_ARCHITECTURES (et al).

Pull Request Checklist

  • Description which clearly states what problems the PR solves.
  • Description contains a link to the Github Issue, and any relevent Discussions, this PR applies to.
  • Include new tests that fail without this PR but passes with it.
  • the new tests (new test runner, really) would have failed before arrow: Fix cross-compiling errors on macos #2798
  • Include any relevent Documentation changes related to this change.
  • Verify all commits have been signed in accordance with the DCO policy.
  • Reviewed PR commit history to remove unnecessary changes.
  • Make sure your PR passes build, test and lint steps completely.

this matches the behavior pre-toolchain patch -- if PSP_ARCH is not set,
then no toolchain file is defined, and the build proceeds without
setting `CMAKE_OSX_ARCHITECTURES`

Signed-off-by: Tom Jakubowski <tom@prospective.dev>
@tomjakubowski
Copy link
Contributor Author

tomjakubowski commented Oct 23, 2024

Workflow run from my fork on current HEAD of this branch (3282a8c) with a test release tag: https://github.com/tomjakubowski/perspective/actions/runs/11471445415

@@ -623,7 +633,8 @@ jobs:

- uses: actions/download-artifact@v4
with:
name: perspective-python-dist-${{ matrix.arch }}-${{ matrix.os }}-${{ matrix.python-version }}
# the macos-14 runner tests artifacts built on macos-13
name: perspective-python-dist-${{ matrix.arch }}-${{ matrix.os == 'macos-14' && 'macos-13' || matrix.os }}-${{ matrix.python-version }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this shell-looking code is how ternaries are done in github actions expressions

macos-14 is aarch64 with Apple silicon.  This lets us test the aarch64
wheel in CI

Will try a follow up to see if the builds are any faster on macos-14.
Will want to double check we still have a good value for
`CMAKE_OSX_DEPLOYMENT_TARGET`/`MACOSX_DEPLOYMENT_TARGET` after upgrading
the builders

Signed-off-by: Tom Jakubowski <tom@prospective.dev>

new crack at build matrix

verified this one with a model of the behavior in github's docs.  would
be cool if I knew how to get github to print the job configurations
before the job actually runs
@tomjakubowski
Copy link
Contributor Author

OK, I think I've fixed the issue where bogus test_python job configurations were being created. Open for review

@tomjakubowski tomjakubowski marked this pull request as ready for review October 23, 2024 20:19
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good!

@texodus texodus merged commit b903fa0 into finos:master Oct 25, 2024
32 of 33 checks passed
@texodus texodus added the bug Concrete, reproducible bugs label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants