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

Support for Apple M1 #479

Merged
merged 15 commits into from
May 23, 2022
Merged

Support for Apple M1 #479

merged 15 commits into from
May 23, 2022

Conversation

ilevyor
Copy link
Contributor

@ilevyor ilevyor commented Apr 29, 2022

Issue #, if available:

Description of changes:

Edited pom.xml to build for mac-arm chip and added corresponding build script.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pom.xml Outdated
</property>
</activation>
<properties>
<cmake.osx_arm64>-DCMAKE_OSX_ARCHITECTURES</cmake.osx_arm64>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird to have a variable where the value is set to something explicit, then the value is ignored later. You have here cmake.osx_arm64 = "-DCMAKE_OSX_ARCHITECTURES" but then later, if the variable is set at all (regardless of its value) you set cmake.architecture = "-DCMAKE_OSX_ARCHITECTURES=arm64"

Also, there's no way here to build the x86_64 binary from an M1 mac.

Suggestion 1: having an explicit <profile> for both mac-arm64 and mac-x64.

Suggestion 2: use the variable's value somehow, so that it's more flexible

maybe do like this?

<properties>
    ...
    <cmake.osx_arch>-DOSX_ARCH_DUMMY=1</xmake.osx_arch>

...
        <profile>
            <id>mac-arm64</id>
            ...
            <properties>
                <cmake.osx_arch>-DCMAKE_OSX_ARCHITECTURES=arm64</cmake.osx_arch>

...

                            <execution>
                                <id>cmake-configure</id>
                                ...
                                <configuration>
                                    <arguments>
                                        ...
                                        <argument>${cmake.osx_arch}</argument>

pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@ilevyor ilevyor requested a review from graebm May 16, 2022 15:01
@ilevyor ilevyor changed the title maven update and build script update maven to support universal builds for m1 support May 16, 2022
@graebm graebm changed the title update maven to support universal builds for m1 support Support for Apple M1 May 16, 2022
- pom file just uses 1 variable: cmake.osx_arch. Instead of cmake.osx_arch leading to the creation of cmake.architecture. Seems simpler this way.
- rename profiles: ~mac-x64-make~ -> mac-x64 and ~mac-arm-make~ -> mac-arm64. Because who cares that we're using makefiles
Copy link
Contributor

@graebm graebm left a comment

Choose a reason for hiding this comment

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

approved if we can run the pipeline one more time successfully
and check that our artifacts are working if you install them on M1

@graebm graebm merged commit a6505fb into main May 23, 2022
@graebm graebm deleted the universal-build branch May 23, 2022 17:38
@graebm
Copy link
Contributor

graebm commented May 23, 2022

the CodeBuild jobs are passing, but something's weird and Github isn't getting the notifications. Merging...

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.

2 participants