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

CMake: Use CMAKE_SYSTEM_PROCESSOR instead of ARCH_TYPE variable #1608

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

AMS21
Copy link
Contributor

@AMS21 AMS21 commented Feb 13, 2024

This was caused by using the CMAKE_SIZEOF_VOID_P variable before it is actually set by cmake (after a call to project).

https://cmake.org/cmake/help/latest/variable/CMAKE_SIZEOF_VOID_P.html

This is set to the size of a pointer on the target machine, and is
determined when a compiled language is enabled. (...)

Also the else(...) construct in CMake works a bit different than the original author expected since the expression is not evaluated and thus we always set ARCH_TYPE to x86 which is not correct.

Also took the liberty to properly output the CMAKE_PROJECT_VERSION and the ARCH_TYPE variables.

AMS21 and others added 2 commits February 13, 2024 14:50
This was caused by using the `CMAKE_SIZEOF_VOID_P` variable before it
is actually set by cmake (after a call to project).

https://cmake.org/cmake/help/latest/variable/CMAKE_SIZEOF_VOID_P.html

> This is set to the size of a pointer on the target machine, and is
> determined when a compiled language is enabled. (...)

Also the `else(...)` construct in CMake works a bit different than the
original author expected since the expression is not evaluated and thus
we always set `ARCH_TYPE` to `x86` which is not correct.

Also took the liberty to properly output the `CMAKE_PROJECT_VERSION` and
the `ARCH_TYPE` variables.
Copy link
Member

@Xottab-DUTY Xottab-DUTY left a comment

Choose a reason for hiding this comment

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

Much thanks!!!

When I was refactoring that part of the code during the creation of 52c3e2e commit, I noticed that ARCH_TYPE was used to set the binaries output folder and CMAKE_SYSTEM_PROCESSOR is better fit there.
But I decided to not touch it yet.

I don't remember why ARCH_TYPE was introduced, but we surely can just remove it.

@Xottab-DUTY Xottab-DUTY added Code Quality Build issue The issue in the build-time. labels Feb 13, 2024
@Xottab-DUTY Xottab-DUTY changed the title CMake: Fix determination of ARCH_TYPE variable CMake: Use CMAKE_SYSTEM_PROCESSOR instead of ARCH_TYPE variable Feb 13, 2024
@Xottab-DUTY Xottab-DUTY merged commit f3ce61e into OpenXRay:dev Feb 13, 2024
18 of 30 checks passed
@AMS21 AMS21 deleted the cmake_arch branch February 13, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build issue The issue in the build-time. Code Quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants