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

Feature: set CMAKE_SYSTEM_PROCESSOR #8026

Closed
wants to merge 2 commits into from

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Nov 7, 2020

@KristianJerpetjon could you test this POC for your case, especially for libpng, libjpeg-turbo and others?

closes: #8025

Changelog: Feature: set CMAKE_SYSTEM_PROCESSOR
Docs: https://github.com/conan-io/docs/pull/XXXX

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@madebr
Copy link
Contributor

madebr commented Nov 24, 2020

Some extra data points.

I did some tests on Visual Studio 2019 using a little cmake script:

cmake_minimum_required(VERSION 3.15)
project(dummy)
message("CMAKE_SYSTEM_PROCESSOR: ${CMAKE_SYSTEM_PROCESSOR}")
message("CMAKE_SIZEOF_VOID_P: ${CMAKE_SIZEOF_VOID_P}")

The following command was executed:

cmake -G "Visual Studio 16 2019" -A <PLATFORM>
PLATFORM CMAKE_SYSTEM_PROCESSOR CMAKE_SIZEOF_VOID_P
Win32 AMD64 4
x64 AMD64 8
ARM ARM64 4
ARM64 ARM64 8

@memsharded
Copy link
Member

Thanks @madebr for the hints!

@SSE4 I think that for moving this forward we need to:

  • First wait to see if this is very necessary to be fixed in the current CMake build helper. If 1.31.4 stabilizes, we will not touch there anything at all. It is too risky.
  • We will move this to the new toolchains, in this case CMakeToolchain, which are experimental
  • I think that at first, the CMAKE_HOST_XXX variables are not needed, we can rely on CMake for it largely. Lets try to define only CMAKE_SYSTEM_PROCESSOR, and have at least a good mapping of most Conan architectures

Windows

Linux:

@madebr
Copy link
Contributor

madebr commented Nov 24, 2020

For arm support of my mpg123 cci recipe,
CMAKE_SYSTEM_PROCESSOR will be necessary for ARM support on Windows.
See https://sourceforge.net/p/mpg123/bugs/298/ for upstream discussion.

As long as CMAKE_SYSTEM_PROCESSOR is somehow defined 😄

@SSE4 SSE4 changed the title Feature: POC : set CMAKE_SYSTEM_PROCSSOR and friends Feature: POC : set CMAKE_SYSTEM_PROCSSOR Nov 25, 2020
Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4 SSE4 force-pushed the cmake_system_processor branch from ef1fd34 to f17f2a6 Compare November 25, 2020 10:17
@SSE4
Copy link
Contributor Author

SSE4 commented Nov 25, 2020

@memsharded updated:

  • set only CMAKE_SYSTEM_PROCESSOR
  • added more architectures to mapping for Windows and *nix

testing on real recipes and cross-building on real platforms is welcomed

@SSE4 SSE4 changed the title Feature: POC : set CMAKE_SYSTEM_PROCSSOR Feature: set CMAKE_SYSTEM_PROCSSOR Nov 25, 2020
@SSE4 SSE4 changed the title Feature: set CMAKE_SYSTEM_PROCSSOR Feature: set CMAKE_SYSTEM_PROCESSOR Nov 25, 2020

@staticmethod
def _cmake_system_processor(os_, arch):
if os_ in ["Windows", "WindowsStore", "WindowsCE"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these names are valid for Visual Studio, not for Windows systems.
mingw expects the gnu names instead of Microsoft tainted ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have a reference? the doc says:

On Windows, this variable is set to the value of the environment variable PROCESSOR_ARCHITECTURE

it doesn't mention compilers

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no other reference.
But the cmake documentation mentions GNU, and mingw is a GNU compiler.

Otoh, I searched for CMAKE_SYSTEM_PROCESSOR arm and CMAKE_SYSTEM_PROCESSOR aarch on github and it looks like there is no universal standard.
I think we have to face the reality that there is no universally-agreed-to-standard to use as CMAKE_SYSTEM_PROCESSOR variable.
Like everything in the c/c++ world (except for the stl standaard), there is no standard. CMake allows everything. CMAKE_SYSTEM_PROCESSOR is just a hint.

In reality, some project will either use one kind CMAKE_SYSTEM_PROCESSOR mapping from processor to cmake variable and not switch between a Microsoft convention or GNU convention. Or will not switch between Windows convention or Linux convention.

It's like the arch argument of MsBuild.build: there will always be a project having a different mapping (uppercase/lowercase/64 suffix).

Making this variable too hidden will make it harder for people wanting to modify it.
I'm just saying it would be nice to have an api that allows this:

self._cmake.cmake_host_processor = tools.cmake_host_processor(self.settings.arch, "gnu").tolower()

So people can hook it.
Cci recipe writers will thank you.

And conan providing a proper default: the Microsoft one if MSVC else GNU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think it's correct. CMake itself doesn't care much about CMAKE_SYSTEM_PROCESSOR and lists it as optional.
the main source of headache is libraries checking for CMAKE_SYSTEM_PROCESSOR, for instance, to include proper assembler sources, such as libjpeg-turbo case.
although CMake specifies CMAKE_SYSTEM_PROCESSOR to have values either of uname or PROCESSOR_ARCHITECTURE, I've seen in reality libraries checking for different values, e.g. for armv8 they may check for armv8, arm64 and aarch64.
examples:

Choose a reason for hiding this comment

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

On 1.33.0 we needed this patch to make the libjpeg-turbo cross compile for "ARM64/aarch64/armv8 etc." (not saying its not libjpeg turbos fault)

@madebr
Copy link
Contributor

madebr commented Oct 4, 2021

Looking at this, I think it's fine to set these variables, when cross building.
As long as a recipe creator can override these.

@sorny92
Copy link

sorny92 commented Oct 11, 2021

@memsharded updated:

  • set only CMAKE_SYSTEM_PROCESSOR
  • added more architectures to mapping for Windows and *nix

testing on real recipes and cross-building on real platforms is welcomed

I tested in linux x86_64 targeting armv8 and the fix works for me.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@memsharded
Copy link
Member

Closing this PR as outdated, it was targeting the legacy integrations, not the new CMakeToolchain

@memsharded memsharded closed this May 19, 2023
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.

[bug] CMAKE_SYSTEM_PROCESSOR isn't set
6 participants