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

Fix the build for M1 MacBooks #211

Closed
wants to merge 1 commit into from
Closed

Fix the build for M1 MacBooks #211

wants to merge 1 commit into from

Conversation

jwatte
Copy link

@jwatte jwatte commented Dec 22, 2021

It turns out, MACOS is a perfectly fine OS to support aarch64 (through its name
arm64.)

The only thing missing was the appropriate check in impl_aarch64, plus renaming
the file to match that it's really for any OS that has the CPU.

jwatte@Jons-MBP build % make test
Running tests...
Test project /Users/jwatte/cpu_features/build
Start 1: bit_utils_test
1/4 Test #1: bit_utils_test ................... Passed 0.06 sec
Start 2: string_view_test
2/4 Test #2: string_view_test ................. Passed 0.04 sec
Start 3: stack_line_reader_test
3/4 Test #3: stack_line_reader_test ........... Passed 0.04 sec
Start 4: cpuinfo_aarch64_test
4/4 Test #4: cpuinfo_aarch64_test ............. Passed 0.12 sec

It turns out, MACOS is a perfectly fine OS to support aarch64 (through its name
arm64.)

The only thing missing was the appropriate check in impl_aarch64, plus renaming
the file to match that it's really for any OS that has the CPU.

jwatte@Jons-MBP build % make test
Running tests...
Test project /Users/jwatte/cpu_features/build
    Start 1: bit_utils_test
1/4 Test google#1: bit_utils_test ...................   Passed    0.06 sec
    Start 2: string_view_test
2/4 Test google#2: string_view_test .................   Passed    0.04 sec
    Start 3: stack_line_reader_test
3/4 Test google#3: stack_line_reader_test ...........   Passed    0.04 sec
    Start 4: cpuinfo_aarch64_test
4/4 Test google#4: cpuinfo_aarch64_test .............   Passed    0.12 sec
@jwatte
Copy link
Author

jwatte commented Dec 22, 2021

Also, I have signed the CLA, and trying to re-sign it gives me an error saying it's already signed.

@@ -15,7 +15,7 @@
#include "cpu_features_macros.h"

#ifdef CPU_FEATURES_ARCH_AARCH64
#if defined(CPU_FEATURES_OS_LINUX) || defined(CPU_FEATURES_OS_ANDROID)
#if defined(CPU_FEATURES_OS_LINUX) || defined(CPU_FEATURES_OS_ANDROID) || defined(CPU_FEATURES_OS_MACOS)
Copy link
Contributor

@toor1245 toor1245 Dec 22, 2021

Choose a reason for hiding this comment

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

@jwatte, recently added new code layout #194 to separate platform code, current file src/impl_linux_or_android_aarch64.c works with /proc/cpuinfo and /proc/self/auxv which is only suitable for Linux, so this defined(CPU_FEATURES_OS_MACOS) should be in other file src/impl_aarch64_macos.c, also a PR #204 has already been created for this, the only thing is that the PR is big and we will merge it with small PRs

сс: #210

@toor1245
Copy link
Contributor

toor1245 commented Feb 1, 2022

@gchatelet, can we close this PR, since we will be adding changes to fix the build for M1 in the future?

@testn
Copy link

testn commented Mar 7, 2022

Can we get this fixed, please? It has been broken for a long time.

@technicalpickles
Copy link

I think this can be closed now that #209 merged which makes a similar fix

@Mizux
Copy link
Collaborator

Mizux commented Aug 18, 2022

closing it as duplicate of the on going PR:

@Mizux Mizux closed this Aug 18, 2022
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.

5 participants