Fix cross-build vars in CMakeToolchain #10856
Conversation
There was a problem hiding this comment.
One of the major advantages of the AppleBlock is that it is esier to maintain, reason about, or customize, if necessary, it is easy to replace the AppleBlock without touching the rest of cross building. More modular than a very large _get_cross_build method with all the cross-build logic for all platforms.
Just a balance, I am not saying this is worse than the current situation, I need to wrap my head around it first.
|
czoido
changed the title
| return arch_host | ||
|
|
||
| def _get_cross_build(self): | ||
| user_toolchain = self._conanfile.conf.get("tools.cmake.cmaketoolchain:user_toolchain") |
There was a problem hiding this comment.
Maybe (not related to this PR, just a possible improvement) it is worth honoring the other confs, and letting users passing both the user_toolchain and the other system_xxxx```confs. This can be done by swapping place and doing a return system_name, system_version, system_processor``.
The thing is that it would be weird to define tools.cmake.cmaketoolchain:system_name and that being completely rejected if you have tools.cmake.cmaketoolchain:user_toolchain defined?
|
|
||
| def _system_version(self): | ||
| os_host = self._conanfile.settings.get_safe("os") | ||
| if self._is_apple_cross_building(): |
There was a problem hiding this comment.
This if self._is_apple_cross_building(): is the possible factorization. If basically every method that does something starts with if self._is_apple_cross_building():, then that means that it could be separated earlier?
|
|
||
| if system_name is not None and system_version is None: | ||
| system_version = _system_version | ||
| if system_name is not None and system_processor is None: |
There was a problem hiding this comment.
the logic changed a bit here, before we were only setting system_processor in the case that if arch != arch_build: but I think we should set this variable anyway if we activated the CMake cross-compiling setting CMAKE_SYSTEM_NAME manually.
Changelog: Fix: Setting
CMAKE_SYSTEM_PROCESSORfor Apple cross-compiling including M1.Changelog: Fix: Do not overwrite values for
CMAKE_SYSTEM_NAME,CMAKE_SYSTEM_VERSIONandCMAKE_SYSTEM_PROCESSORset from the [conf] or the user_toolchain.Changelog: Fix: Fix architecture translation from Conan syntax to build system in CMakeToolchain.
Docs: conan-io/docs#2472
I tried to maintain all the logic like it was before for the “Generic” part and fix setting
CMAKE_SYSTEM_NAME,CMAKE_SYSTEM_VERSIONandCMAKE_SYSTEM_PROCESSORfor Apple.A bit of background on these variables:
They are typically set when we cross-compile, just setting the
CMAKE_SYSTEM_NAMEvariable manually will make that CMake setsCMAKE_CROSSCOMPILING=True. If you setCMAKE_SYSTEM_NAME, you should also setCMAKE_SYSTEM_VERSIONandCMAKE_SYSTEM_PROCESSOR.The Apple case:
From CMake docs:
That means for some cases we could not set
CMAKE_SYSTEM_NAMEand as we are settingCMAKE_OSX_SYSROOTeverything should be ok. Seems that this holds true for the M1 case, in fact in that case we were not settingCMAKE_SYSTEM_NAMEbefore.Also from CMake docs:
Maybe we should not set these variables for the Apple case but we may affect someone using
CMAKE_CROSSCOMPILING(although it’s not recommended).I have also checked the popular ios-cmake toolchain, and they seem to set those vars for the Apple case (with the same values that we do now)
#TAGS: svn, slow