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

cross build arch check in cmake build helper #7827

Merged
merged 11 commits into from Oct 8, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Oct 6, 2020

Changelog: Bugfix: Check comparing the host and the build architecture to decide if cross building and set CMAKE_SYSTEM_NAME in the CMake build helper.
Docs: Omit

#tags: slow

Close #7826

@memsharded memsharded added this to the 1.30.1 milestone Oct 6, 2020
@memsharded
Copy link
Member Author

memsharded commented Oct 6, 2020

Please @jasal82 have a look.

I have had to change the logic, and only change to CMAKE_SYSTEM_NAME=generic when the host os is not defined (embedded or the like), otherwise it will be breaking, almost for sure in some platforms as Windows that will get confused by the CMAKE_SYSTEM_NAME, and might potentially be changing users doing Linux 32bits from 64bits.

What is your host os?

Copy link
Contributor

@jgsogo jgsogo left a comment

I need more info, why not a more straightforward if/else as suggested?

definitions["CMAKE_SYSTEM_NAME"] = {"Macos": "Darwin",
"iOS": "Darwin",
"tvOS": "Darwin",
"watchOS": "Darwin",
"Neutrino": "QNX"}.get(os_, os_)
else:
definitions["CMAKE_SYSTEM_NAME"] = "Generic"
elif not os_:
Copy link
Contributor

@jgsogo jgsogo Oct 7, 2020

Choose a reason for hiding this comment

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

I would expect something like this here:

if cross_building(self._conanfile):  # We are cross building
    if os_:
        # If we are cross-building, it doesn't matter if it is because OS or ARCH
        definitions["CMAKE_SYSTEM_NAME"] = {...}.get(os_, os_)
    else:
        definitions["CMAKE_SYSTEM_NAME"] = "Generic"  # To avoid the CMake warning for missing *.cmake file

I'm sure there is a reason why we didn't have this more straightforward if/else conditions... maybe it is encoded in some test, but a comment here would be nice.


This implementation won't satisfy the ARM cross-compilation: os:host=Linux, os:build=Linux, arch:host=x86_64, arch:build=arm. According to CMake docs, here we should generate something with:

set(CMAKE_SYSTEM_NAME Linux)

Copy link
Member Author

@memsharded memsharded Oct 7, 2020

Choose a reason for hiding this comment

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

cross-building is returning True for Windows Visual building to x86 for x64, but seems that adding CMAKE_SYSTEM_NAME Windows for that case would be problematic, as cmake not really consider it cross-building, as far as I understood. Trying to do something like the proposed solution, I got some tests failing.

I am not saying that what we have is correct, certainly it is incomplete and fragile/buggy, but the risk of large breakings is important here.

Copy link
Contributor

@jgsogo jgsogo Oct 7, 2020

Choose a reason for hiding this comment

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

Ok, probably it is worth to add a CONAN_V2_MODE block with a more straightforward implementation (if we think it is better):

if cross_compiling:
    if CONAN_V2_MODE:
        if os_ == os_build and _os == 'Windows':
            # Nothing here?
        else:
            definitions["CMAKE_SYSTEM_NAME"] = {..., None, 'Generic'}.get(os_, os_)
    else:
        # Current logic plus patches.
        ...

I'll wait for answer here, I really want to know why setting it to Generic instead of the actual value.

Copy link
Member Author

@memsharded memsharded Oct 7, 2020

Choose a reason for hiding this comment

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

This functionality is going to be moved anyway to toolchains, so CONAN_V2_MODE would probably not be necessary there.

@jgsogo
Copy link
Contributor

jgsogo commented Oct 7, 2020

After the last answer in #7826 , I think we can add a test with ARM cross-compilation: os:host=Linux, os:build=Linux, arch:host=x86_64, arch:build=arm and check the resulting CMAKE_SYSTEM_NAME... and see if it doesn't break anything.

@memsharded memsharded requested a review from jgsogo Oct 7, 2020
Copy link
Contributor

@jgsogo jgsogo left a comment

I think this is the way to go (and we should remove the Windows exception too), but I'm afraid this can break existing solutions when cross compiling from Linux-x64 to Linux-x86.

Setting CMAKE_SYSTEM_NAME will make CMake set CMAKE_CROSSCOMPILING variable and it will have other implications: https://gitlab.kitware.com/search?utf8=%E2%9C%93&snippets=false&scope=&repository_ref=master&search=CMAKE_CROSSCOMPILING&group_id=415&project_id=541

I think that we should keep working the same for that scenario: Linux x64 to x86. Something like this:

skip_x64_x86 = conanfile.settings.get_safe('os') in ['Windows', 'Linux']
if cross_building(self._conanfile, skip_x64_x86=skip_x64_x86):
    ...
    definitions["CMAKE_SYSTEM_NAME"] = cmake_system_name_map.get(os_, os_)

I've realized about the implications after reading about CMAKE_CROSSCOMPILING 😞

@jgsogo jgsogo mentioned this pull request Oct 8, 2020
1 task
@memsharded memsharded requested a review from jgsogo Oct 8, 2020
jgsogo
jgsogo approved these changes Oct 8, 2020
Copy link
Contributor

@jgsogo jgsogo left a comment

I'll help you to pay the 🍔 if needed, I really think this is the desired behavior (keeping the exceptional one about Win/Linux exception)

conans/test/unittests/client/build/cmake_test.py Outdated Show resolved Hide resolved
jgsogo
jgsogo approved these changes Oct 8, 2020
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.

None yet

2 participants