conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.29k stars 982 forks source link

[bug] Auto-detection of CMAKE_SYSTEM_NAME not working for all cross-compilation scenarios #7826

Closed jasal82 closed 4 years ago

jasal82 commented 4 years ago

Conan has an auto-detection feature for CMAKE_SYSTEM_NAME which is enabled by default if a value is not set explicitly. I found the method _cmake_cross_build_defines() in CMakeDefinitionsBuilder which seems to perform that auto-detection:

        # System name and system version
        if cmake_system_name is not True:  # String not empty
            definitions["CMAKE_SYSTEM_NAME"] = cmake_system_name
        else:  # detect if we are cross building and the system name and version
            if cross_building(self._conanfile):  # We are cross building
                if os_ != os_build:
                    if os_:  # the_os is the host (regular setting)
                        definitions["CMAKE_SYSTEM_NAME"] = {"Macos": "Darwin",
                                                            "iOS": "Darwin",
                                                            "tvOS": "Darwin",
                                                            "watchOS": "Darwin",
                                                            "Neutrino": "QNX"}.get(os_, os_)
                    else:
                        definitions["CMAKE_SYSTEM_NAME"] = "Generic"

So if os_ == os_build the variable is never set which is clearly wrong. A typical cross-toolchain for arm would have os = Linux and os_build = Linux but arch and arch_build are different. My suggestion would be to add another else: block on the if os_ != os_build level that also sets CMAKE_SYSTEM_NAME to Generic.

memsharded commented 4 years ago

Just to be sure are you suggesting?

         if cross_building(self._conanfile):  # We are cross building
                if os_ != os_build:
                    if os_:  # the_os is the host (regular setting)
                        definitions["CMAKE_SYSTEM_NAME"] = {"Macos": "Darwin",
                                                            "iOS": "Darwin",
                                                            "tvOS": "Darwin",
                                                            "watchOS": "Darwin",
                                                            "Neutrino": "QNX"}.get(os_, os_)
                    else:
                        definitions["CMAKE_SYSTEM_NAME"] = "Generic"
          else:
                definitions["CMAKE_SYSTEM_NAME"] = "Generic"

Or why not something like:

         if cross_building(self._conanfile):  # We are cross building
                if os_ != os_build and arch != arch_build:
                    if os_:  # the_os is the host (regular setting)
                        definitions["CMAKE_SYSTEM_NAME"] = {"Macos": "Darwin",
                                                            "iOS": "Darwin",
                                                            "tvOS": "Darwin",
                                                            "watchOS": "Darwin",
                                                            "Neutrino": "QNX"}.get(os_, os_)
                    else:
                        definitions["CMAKE_SYSTEM_NAME"] = "Generic"

It seems that CMAKE_SYSTEM_NAME should not be set unless you are cross-building, so I would say the second.

jasal82 commented 4 years ago

Ah I'm sorry, that was wrong. What I meant was

         if cross_building(self._conanfile):  # We are cross building
            if os_ != os_build:
                if os_:  # the_os is the host (regular setting)
                    definitions["CMAKE_SYSTEM_NAME"] = {"Macos": "Darwin",
                                                        "iOS": "Darwin",
                                                        "tvOS": "Darwin",
                                                        "watchOS": "Darwin",
                                                        "Neutrino": "QNX"}.get(os_, os_)
                else:
                    definitions["CMAKE_SYSTEM_NAME"] = "Generic"
            else:
                definitions["CMAKE_SYSTEM_NAME"] = "Generic"

I'll fix the description. I'd say that the solution with if os_ != os_build or arch != arch_build would technically be the same because that's probably what cross_building() is checking for, isn't it?

memsharded commented 4 years ago

Ok, that makes more sense, indeed. Pushed review.

jgsogo commented 4 years ago

Hi, @jasal82

According to CMake docs, cross-compiling to Linux ARM should use set(CMAKE_SYSTEM_NAME Linux), why are you suggesting to assign Generic to it? According to this link, using Generic is basically a way to avoid a warning (or not to use the CMake distributed files).

jasal82 commented 4 years ago

@jgsogo If you can set Linux, even better, but it doesn't matter for my specific problem because any value in CMAKE_SYSTEM_NAME will enable the cross-compilation logic in CMake. In the current state it is completely broken, so I was suggesting the minimal viable fix. Feel free to adjust. The correct approach would probably be to map the value of os_ to the available CMake platform names.

jgsogo commented 4 years ago

Thanks for the answer, I was confused about it. 👍

jgsogo commented 4 years ago

7827 merged to 1.30.1. It will add the CMAKE_SYSTEM_NAME when cross-compiling to arm using a Linux machine to build.

Thanks for reporting the issue.

memsharded commented 4 years ago

Closed by https://github.com/conan-io/conan/pull/7827