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

Fix freebsd cross build with cmake 3.25 #78534

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Nov 18, 2022

In cmake 3.25, the toolchain file is called with context containing LINUX variable set. This causes problem for non-Linux platform. Fix is to unset platform variables which we use in the toolchain file.

Upstream PR: dotnet/arcade#11672
Fixes: #78528

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 18, 2022
@ghost
Copy link

ghost commented Nov 18, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

In cmake 3.25, the toolchain file is called with context containing LINUX variable set. This causes problem for non-Linux platform. Fix is to unset platform variables which we use in the toolchain file.

Upstream PR: dotnet/arcade#11672
Fixes: #78528

Author: am11
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@am11 am11 force-pushed the feature/build/freebsd-cmake-3.25 branch from 9b92609 to 9ff3fc2 Compare November 18, 2022 03:24
@am11
Copy link
Member Author

am11 commented Nov 18, 2022

cc @jkotas, reverted pinned container reference. FreeBSD legs are green. It was this change in cmake 3.25 which sets LINUX to 1: Kitware/CMake@62cd390.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@akoeplinger
Copy link
Member

akoeplinger commented Nov 18, 2022

We have similar code in eng/native/tryrun.cmake. This sounds like a cmake bug to me, could you please report it to them?

The changelog only mentions this so I don't think it should be set on freebsd:

The LINUX and CMAKE_HOST_LINUX variables are now set to true when the target or host system is Linux, respectively.

Maybe we should also stop relying on these variables and introduce our own (e.g. similar to the CLR_CMAKE_HOST_FREEBSD)

@akoeplinger
Copy link
Member

Ah this is in the cross-build docker images so the host system is actually Linux right? That makes more sense then and makes me wish we'd stop using these variables even more.

@am11
Copy link
Member Author

am11 commented Nov 18, 2022

It can refactored separately, I don't feel strongly that we should; because:

  1. The toolchain file and try_compile is a cmake built-in mechanism and cmake invokes these files at different times with different contexts in cross-build mode. We should not mix the variable names which will be set later by configureplatform.cmake (e.g. CLR_CMAKE_HOST_FREEBSD).

  2. The root-cause of why Linux.cmake is even included in the toolchain file (which is supposed to act like a seed file in cross-compilation mode to locate usable toolchain) is because we make decision about the target platform in this file, rather than before invoking cmake. We can easily move these conditions:

    set(TARGET_ARCH_NAME $ENV{TARGET_BUILD_ARCH})
    if(EXISTS ${CROSS_ROOTFS}/bin/freebsd-version)
    set(CMAKE_SYSTEM_NAME FreeBSD)
    set(FREEBSD 1)
    elseif(EXISTS ${CROSS_ROOTFS}/usr/platform/i86pc)
    set(CMAKE_SYSTEM_NAME SunOS)
    set(ILLUMOS 1)
    elseif(EXISTS ${CROSS_ROOTFS}/boot/system/develop/headers/config/HaikuConfig.h)
    set(CMAKE_SYSTEM_NAME Haiku)
    else()
    set(CMAKE_SYSTEM_NAME Linux)
    set(LINUX 1)
    endif()
    set(CMAKE_SYSTEM_VERSION 1)
    to gen-buildsys.sh and set CMAKE_SYSTEM_NAME to the desired target platform before invoking cmake (as we already do for Darwin in gen-buildsys.sh..). But we haven't done that intentionally from the get go because we wanted to keep build-rootfs.sh and toolchain.cmake pair tightly coupled and independent; those hint paths to detect platform come from (or cause by) build-rootfs.sh.

@akoeplinger
Copy link
Member

Yeah I'm mostly worried that we're basically overriding CMake-internal variables and who knows what impact that has.

@am11
Copy link
Member Author

am11 commented Nov 18, 2022

It is a predefined variable. We overwrite predefined variables all over the place. All CMAKE_* are predefined variables, for example. I don't think there is anything to worry about here.

@jkotas
Copy link
Member

jkotas commented Nov 18, 2022

I agree with @am11 that overwriting built-in CMake variables during cross-compilation seems to be pretty standard pattern. It is even suggested in official CMake docs.

@jkotas
Copy link
Member

jkotas commented Nov 18, 2022

All failures are known issues according to the Build Analysis.

@jkotas jkotas merged commit 82054d0 into dotnet:main Nov 18, 2022
@akoeplinger
Copy link
Member

akoeplinger commented Nov 18, 2022

Right but we're apparently using these variables not in the way that cmake intended (i.e. for cross compilation targets rather than the host OS). I think refactoring to use e.g. a CROSS_TARGET_ prefix would make it clearer.

@akoeplinger
Copy link
Member

We also need to backport this or some other fix to all servicing branches.

@jkotas
Copy link
Member

jkotas commented Nov 18, 2022

we're apparently using these variables not in the way that cmake intended (i.e. for cross compilation targets rather than the host OS).

CMake documentation says that it is meant to represent target system: Kitware/CMake@62cd390#diff-186bb1bd28bd5ab49edaeb9820e3a5218c73bf02fec15bd71d221da5a130608bR4

@akoeplinger
Copy link
Member

Ah ok, yeah then this approach looks good to me.

@jkotas
Copy link
Member

jkotas commented Nov 18, 2022

We also need to backport this or some other fix to all servicing branches.

I have triggered the backports in arcade repo. We can let the codeflow to take care of the rest in the servicing trees.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FreeBSD build break with cmake version 3.25.0
3 participants