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: set CMAKE_SYSTEM_PROCESSOR when cross-compiling #5215

Merged
merged 9 commits into from
Jun 17, 2024

Conversation

SirLynix
Copy link
Member

Some projects use CMAKE_SYSTEM_PROCESSOR to identify target archs (see jrouwe/JoltPhysics#1133 and xmake-io/xmake-repo#4330).
When CMAKE_SYSTEM_NAME is set, cmake switches to cross-compiling mode and won't set CMAKE_SYSTEM_PROCESSOR by itself.

@waruqi waruqi added this to the v2.9.3 milestone Jun 13, 2024
envs.CMAKE_SYSTEM_NAME = "Darwin"
envs.CMAKE_SYSTEM_PROCESSOR = package:targetarch()
Copy link
Member

Choose a reason for hiding this comment

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

Is there an official standard for the value definition of CMAKE_SYSTEM_PROCESSOR? This may break some packages if each package requires a different value specification.

see https://github.com/search?q=repo%3Axmake-io%2Fxmake-repo%20CMAKE_SYSTEM_PROCESSOR&type=code

maybe arm64, aarch64, ARM64 ...

I see that in CMakelist.txt for each package, it has a different value. Even if they are all arm64.

If there is no standardized specification, I would prefer to set it up in each package.

Copy link
Member

Choose a reason for hiding this comment

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

a related issue. #3174

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better that xmake sets a default value because CMake default values is empty when CMAKE_SYSTEM_NAME is set (meaning nothing matches), and xmake already sets CMAKE_SYSTEM_PROCESSOR by passing toolchain file on Android and emscripten (toolchain files set CMAKE_SYSTEM_PROCESSOR).

This value seems to be generated from uname -m, giving this possible list of archs on system supporting it: https://unix.stackexchange.com/questions/711432/uname-m-valid-values

We could set a translation table, to set AMD64 instead of x86_64 and more.

Copy link
Member

Choose a reason for hiding this comment

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

and xmake already sets CMAKE_SYSTEM_PROCESSOR by passing toolchain file on Android and emscripten (toolchain files set CMAKE_SYSTEM_PROCESSOR).

But xmake doesn't set them directly. the official toolchain for Android and emscripten provides this value, so it's usually not a problem.

And for the cross-compilation toolchain, we can't be sure how the correct values should be set.

We could set a translation table, to set AMD64 instead of x86_64 and more.

But some packages use x86_64 instead of AMD64. https://github.com/xmake-io/xmake-repo/blob/7c3c922d9978d28e02ca093bc5804af359736341/packages/t/turbobase64/port/CMakeLists.txt#L21

As a result, we are still unsure how to set a uniform value for all packages.

Copy link
Member

Choose a reason for hiding this comment

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

Therefore, even if xmake sets a default value, there is no guarantee that it will be correct, and this can lead to some uncertainty. If a new version of xmake changes the default value, it can break some existing packages.

Whereas if we set it in the package, then it is always guaranteed that the correct value is configured.

Copy link
Member Author

Choose a reason for hiding this comment

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

and xmake already sets CMAKE_SYSTEM_PROCESSOR by passing toolchain file on Android and emscripten (toolchain files set CMAKE_SYSTEM_PROCESSOR).

But xmake doesn't set them directly. the official toolchain for Android and emscripten provides this value, so it's usually not a problem.

And for the cross-compilation toolchain, we can't be sure how the correct values should be set.

We could set a translation table, to set AMD64 instead of x86_64 and more.

But some packages use x86_64 instead of AMD64. https://github.com/xmake-io/xmake-repo/blob/7c3c922d9978d28e02ca093bc5804af359736341/packages/t/turbobase64/port/CMakeLists.txt#L21

As a result, we are still unsure how to set a uniform value for all packages.

I understand, but trying to set a good default would be better than not setting anything, an empty CMAKE_SYSTEM_PROCESSOR is never correct, a CMAKE_SYSTEM_PROCESSOR set to the expected values (AMD64 on Windows, x86_64 on Linux, etc.) would be better and allow some package to work with less work.

Copy link
Member

Choose a reason for hiding this comment

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

an empty CMAKE_SYSTEM_PROCESSOR is never correct, a CMAKE_SYSTEM_PROCESSOR set to the expected values (AMD64 on Windows, x86_64 on Linux, etc.) would be better and allow some package to work with less work.

There are very few such cases, and of the 1300+ packages in xmake-repo, only 9 need to have them explicitly set.

When we encounter these cases, we simply just need to go ahead and configure them explicitly in the package.

https://github.com/search?q=repo%3Axmake-io%2Fxmake-repo%20CMAKE_SYSTEM_PROCESSOR&type=code

Also, in the CMakelist.txt of some packages, they will also probe and hard-code the configuration of CMAKE_SYSTEM_PROCESSOR by themselves based on the platform, which may lead to conflicts and redefinitions if we set the default value instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you consider it if we could ensure the CMAKE_SYSTEM_PROCESSOR values is AMD64 on Windows x86_64, x86_64 on Linux 64bits, etc.? By checking how CMake gets this value

Copy link
Member

Choose a reason for hiding this comment

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

You can try add get_cmake_system_processor(package) to get and map these archs. But the user must be able to override it in the package configuration.

if configs_str and configs_str:find(k, 1, true) then

@waruqi
Copy link
Member

waruqi commented Jun 15, 2024

And, if -DCMAKE_SYSTEM_PROCESSOR= has been configured in the package configuration, it should be used first and the internal settings ignored.

@waruqi waruqi requested a review from xq114 June 16, 2024 13:43
@waruqi waruqi merged commit e411d9b into xmake-io:dev Jun 17, 2024
19 checks passed
@SirLynix SirLynix deleted the patch-17 branch June 17, 2024 07:38
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