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

fixes platform identification in setup.py to work correctly with M1/M2 Mac #114

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

ab-gh
Copy link
Contributor

@ab-gh ab-gh commented Oct 4, 2022

line 42 in setup.py was:

IS_ARM = get_platform().startswith('linux-aarch')

which only returns true for linux arm64 machines. After testing on a MacBook Air M2, setting IS_ARM = True enabled successful install. This commit enables the IS_ARM flag to function with Apple Silicon architecture Macs (M1/M2/)

@coldfix
Copy link
Member

coldfix commented Oct 4, 2022

Hi, thanks for looking into it. Did you check whether it builds locally? I'll let the workflows run, but this will not build mac arm64 wheels. For that the .github/workflows may have to be extended to add a M1 machine.

@ab-gh
Copy link
Contributor Author

ab-gh commented Oct 4, 2022

This correctly builds locally. MAD-X was installed locally following the instructions as here but with the following changes:

Change cmake to arm64 architecture

replace

cmake .. \
    -DCMAKE_POLICY_DEFAULT_CMP0077=NEW \
    -DCMAKE_POLICY_DEFAULT_CMP0042=NEW \
    -DCMAKE_OSX_ARCHITECTURES=x86_64 \
    -DCMAKE_C_COMPILER=gcc \
    -DCMAKE_CXX_COMPILER=g++ \
    -DCMAKE_Fortran_COMPILER=gfortran \
    -DBUILD_SHARED_LIBS=OFF \
    -DMADX_STATIC=OFF \
    -DCMAKE_INSTALL_PREFIX=../dist \
    -DCMAKE_BUILD_TYPE=Release \
    -DMADX_INSTALL_DOC=OFF \
    -DMADX_ONLINE=OFF \
    -DMADX_FORCE_32=OFF \
    -DMADX_X11=OFF

with

    -DCMAKE_OSX_ARCHITECTURES=arm64 \

Set Openblas directory

set -x DYLD_FALLBACK_LIBRARY_PATH /opt/homebrew/opt/openblas/lib

Disable ASM optimisation

CFLAGS="--disable-asm-optimizations" cmake --build . --target install

Build with blas/lapack linked

python setup.py build_ext -lblas -llapack

Screenshot 2022-10-04 at 1 53 22 pm

image

@coldfix
Copy link
Member

coldfix commented Oct 4, 2022

Sounds good. We can potentially also add an upstream wheel using these changes.

set -x DYLD_FALLBACK_LIBRARY_PATH /opt/homebrew/opt/openblas/lib

Of course, setting DYLD path would not be ideal in the case of an upstream wheel.

CFLAGS="--disable-asm-optimizations" cmake --build . --target install

What was the reason for this? Did you experience build or runtime errors?

The style check complains about line length. Can you change it like this:

- IS_WIN = get_platform().startswith('win')
- IS_ARM = get_platform().startswith('linux-aarch')
+ platform = get_platform()
+ IS_WIN = platform.startswith('win')
+ IS_ARM = platform.startswith('linux-aarch') or platform.endswith('arm64')

@ab-gh
Copy link
Contributor Author

ab-gh commented Oct 4, 2022

CFLAGS="--disable-asm-optimizations" cmake --build . --target install

What was the reason for this? Did you experience build or runtime errors?

Apologies, this seems as though it is not necessary.

@coldfix
Copy link
Member

coldfix commented Oct 4, 2022

Are you also planning to work on building an upstream wheel using the github actions, or should I merge as is?

Build error on Mac might be due to py3.6 being EOLed some months ago, will have to remove that build.

@ab-gh
Copy link
Contributor Author

ab-gh commented Oct 4, 2022

Are you also planning to work on building an upstream wheel using the github actions, or should I merge as is?

I can certainly give this a try.

@ab-gh
Copy link
Contributor Author

ab-gh commented Oct 4, 2022

actually it probably makes sense for an upstream wheel to be a separate PR. easiest to merge now and I'll open a new one.

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