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 ARM build issues #3602

Merged
merged 6 commits into from Jan 26, 2021
Merged

Fix ARM build issues #3602

merged 6 commits into from Jan 26, 2021

Conversation

uklotzde
Copy link
Contributor

Typo & detection

@uklotzde uklotzde added the build label Jan 26, 2021
@uklotzde uklotzde added this to the 2.3.0 milestone Jan 26, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Jan 26, 2021

Let's give it a try? 🤷

@uklotzde
Copy link
Contributor Author

Is the regex reasonable? Inspired from here: https://github.com/opencv/opencv/blob/8c221981bf7e2c8eb87302d70000aa246641487c/cmake/OpenCVDetectCXXCompiler.cmake#L91

The typo is a real bug.

@daschuer
Copy link
Member

Probably not.
I have just found this list:
https://stackoverflow.com/questions/45125516/possible-values-for-uname-m
There are no capital versions.

@uklotzde
Copy link
Contributor Author

Btw, we also use AMD64, but not amd64 which seems to be more common. Out of scope.

CMakeLists.txt Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor Author

The conditional processor/architecture dependent stuff is full of redundancies and old cruft. Needs to be rewritten at some time.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -201,9 +201,9 @@ elseif(GNU_GCC OR LLVM_CLANG)
# TODO(rryan): macOS can use SSE3, and possibly SSE 4.1 once
# we require macOS 10.12.
# https://stackoverflow.com/questions/45917280/mac-osx-minumum-support-sse-version
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL "arm")
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^arm.*")
Copy link
Member

Choose a reason for hiding this comment

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

This regex does not work now, because it matches a "armv8b" which is a arm64. This means it matches some arm64.
Please revert to the original and add a comment why this is correct.
Something like:
// CMAKE_SYSTEM_PROCESSOR is just "arm" for all 32 bit ARMs. These flags are not valid with AArch64 and ARMv8 targets where CMAKE_SYSTEM_PROCESSOR is one of ("aarch64_be (arm64)", "aarch64 (arm64)", "armv8b (arm64 compat)" and "armv8l (arm64 compat)".

@uklotzde
Copy link
Contributor Author

Someone else may take over.

@uklotzde uklotzde closed this Jan 26, 2021
@uklotzde uklotzde reopened this Jan 26, 2021
@uklotzde
Copy link
Contributor Author

Final proposal. I am not doing research on ARM architectures.

@uklotzde
Copy link
Contributor Author

You may add the comments you think are required later.

...just in case and for consistency
@uklotzde
Copy link
Contributor Author

Btw, STREQUAL "arm" never worked, because otherwise the bug would have been detected.

@daschuer
Copy link
Member

Thank you.
You are right, it also has not worked on our PPA. There is a "armv7l" reported.
I am curious if this really helps for your NaN bug.

@daschuer daschuer merged commit 06c8be4 into mixxxdj:2.3 Jan 26, 2021
@uklotzde
Copy link
Contributor Author

No, it won't fix the NaN issue. This was just a bycatch.

@uklotzde uklotzde deleted the arm-build branch January 28, 2021 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants