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.

Sign up for GitHub

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

Jump to bottom

[FIXUP] cmake: Do not trigger cross-compiling unnecessarily #125

Merged
merged 3 commits into from
May 20, 2024
Merged

[FIXUP] cmake: Do not trigger cross-compiling unnecessarily #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a856151
[FIXUP] cmake: Do not trigger cross-compiling unnecessarily
hebasto May 20, 2024
6ff697a
depends: Rename `cmake_system` > `cmake_system_name`
hebasto May 15, 2024
b231e61
depends: Add `CMAKE_SYSTEM_VERSION` to CMake toolchain file
hebasto May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,22 +556,20 @@ else()
)
endif()

if(CMAKE_CROSSCOMPILING)
target_compile_definitions(core_base_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS})
target_compile_definitions(core_depends_release_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS_RELWITHDEBINFO})
target_compile_definitions(core_depends_debug_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS_DEBUG})

# If {C,CXX}FLAGS variables are defined during building depends and
# configuring this build system, their content might be duplicated.
if(DEFINED ENV{CFLAGS})
deduplicate_flags(CMAKE_C_FLAGS)
endif()
if(DEFINED ENV{CXXFLAGS})
deduplicate_flags(CMAKE_CXX_FLAGS)
endif()
if(DEFINED ENV{LDFLAGS})
deduplicate_flags(CMAKE_EXE_LINKER_FLAGS)
endif()
target_compile_definitions(core_base_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS})
target_compile_definitions(core_depends_release_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS_RELWITHDEBINFO})
target_compile_definitions(core_depends_debug_interface INTERFACE ${DEPENDS_COMPILE_DEFINITIONS_DEBUG})

# If {C,CXX,LD}FLAGS variables are defined during building depends and
# configuring this build system, their content might be duplicated.
if(DEFINED ENV{CFLAGS})
deduplicate_flags(CMAKE_C_FLAGS)
endif()
if(DEFINED ENV{CXXFLAGS})
deduplicate_flags(CMAKE_CXX_FLAGS)
endif()
if(DEFINED ENV{LDFLAGS})
deduplicate_flags(CMAKE_EXE_LINKER_FLAGS)
endif()

add_subdirectory(src)
Expand Down
42 changes: 23 additions & 19 deletions cmake/module/Maintenance.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,15 @@ function(add_maintenance_targets)
else()
set(exe_format ELF)
endif()
if(CMAKE_CROSSCOMPILING)
list(JOIN DEPENDS_C_COMPILER_WITH_LAUNCHER " " c_compiler_command)
else()
set(c_compiler_command ${CMAKE_C_COMPILER})
endif()

# In CMake, the components of the compiler invocation are separated into three distinct variables:
# - CMAKE_C_COMPILER_LAUNCHER: a semicolon-separated list of launchers or tools to precede the compiler (e.g., env or ccache).
Copy link

Choose a reason for hiding this comment

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

Does this mean that ccache and env can't be used simultaneously? Basically meaning that ccache can't be used for darwin builds?

Copy link
Owner Author

@hebasto hebasto May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does this mean that ccache and env can't be used simultaneously?

No, it does not.

The current implementation works flawlessly for ccache + darwin. It appends ccache command to the CMAKE_{C.CXX}_COMPILER_LAUNCHER variables after env.

UPD. The resulted compiler invocation string looks like env ... ccache ... clang ...

# - CMAKE_C_COMPILER: the full path to the compiler binary itself (e.g., /usr/bin/clang).
# - CMAKE_C_COMPILER_ARG1: a string containing initial compiler options (e.g., --target=x86_64-apple-darwin -nostdlibinc).
# By concatenating these variables, we form the complete command line to be passed to a Python script via the CC environment variable.
list(JOIN CMAKE_C_COMPILER_LAUNCHER " " c_compiler_command)
Copy link

Choose a reason for hiding this comment

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

Looks like this has just taken on the depends macos ccache hack behaviour for everything, and I still don't understand why?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It is not only about "ccache hack".

Consider the following example:

$ cat CMakeLists.txt 
cmake_minimum_required(VERSION 3.22)
set(CMAKE_CXX_COMPILER env -u A_VAR clang++)
project(compiler_demo CXX)
message("CMAKE_CXX_COMPILER: ${CMAKE_CXX_COMPILER}")
$ cmake -B build
-- The CXX compiler identification is Clang 18.1.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/env - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMAKE_CXX_COMPILER: /usr/bin/env
-- Configuring done (0.2s)
-- Generating done (0.0s)
-- Build files have been written to: /home/hebasto/CMAKE_COMPILER_DEMO/build

The output CMAKE_CXX_COMPILER: /usr/bin/env is quite misleading.

Copy link

Choose a reason for hiding this comment

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

set(CMAKE_CXX_COMPILER env -u A_VAR clang++)

Where are we doing this?

Copy link

Choose a reason for hiding this comment

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

I still don't understand why we need to do anything other than set(c_compiler_command ${CMAKE_C_COMPILER}), like we have been doing this entire time. If this was working perfectly fine before, why does it not work now.

Copy link
Owner Author

Choose a reason for hiding this comment

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

set(CMAKE_CXX_COMPILER env -u A_VAR clang++)

Where are we doing this?

bitcoin/depends/hosts/darwin.mk

Lines 87 to 94 in 7973a67

darwin_CXX=env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH \
-u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH \
-u LIBRARY_PATH \
$(clangxx_prog) --target=$(host) \
-B$(build_prefix)/bin \
-isysroot$(OSX_SDK) -nostdlibinc \
-iwithsysroot/usr/include/c++/v1 \
-iwithsysroot/usr/include -iframeworkwithsysroot/System/Library/Frameworks

Copy link
Owner Author

@hebasto hebasto May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The config output from CMake itself also isn't relevant for those scripts?

It demonstrates that the value of the CMAKE_CXX_COMPILER variable could be useless to convey it to an external python script that expects a compiler invocation string.

Copy link

Choose a reason for hiding this comment

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

Can we get some documentation.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Can we get some documentation.

Sure. That's what I can suggest to consider:

  1. The CMAKE_<LANG>_COMPILER variable docs:

The full path to the compiler for LANG.

If a non-full path value is supplied then CMake will resolve the full path of the compiler.

  1. From the source code:
    # if CMAKE_${lang}_COMPILER is a list, use the first item as
    # CMAKE_${lang}_COMPILER and the rest as CMAKE_${lang}_COMPILER_ARG1
  1. The <LANG>_COMPILER_LAUNCHER property docs:

Specify a semicolon-separated list containing a command line for a compiler launching tool. ...
Some example tools are distcc and ccache.

The env command is a similar wrapper tool.

Copy link

Choose a reason for hiding this comment

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

I mean a comment in our code.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sure. A comment has been added.

string(STRIP "${c_compiler_command} ${CMAKE_C_COMPILER}" c_compiler_command)
string(STRIP "${c_compiler_command} ${CMAKE_C_COMPILER_ARG1}" c_compiler_command)
add_custom_target(test-security-check
COMMAND ${CMAKE_COMMAND} -E env CC=${c_compiler_command} CFLAGS=${CMAKE_C_FLAGS} LDFLAGS=${CMAKE_EXE_LINKER_FLAGS} ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/devtools/test-security-check.py TestSecurityChecks.test_${exe_format}
COMMAND ${CMAKE_COMMAND} -E env CC=${c_compiler_command} CFLAGS=${CMAKE_C_FLAGS} LDFLAGS=${CMAKE_EXE_LINKER_FLAGS} ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/devtools/test-symbol-check.py TestSymbolChecks.test_${exe_format}
Expand Down Expand Up @@ -111,7 +115,20 @@ function(add_macos_deploy_target)
)

string(REPLACE " " "-" osx_volname ${PACKAGE_NAME})
if(CMAKE_CROSSCOMPILING)
if(CMAKE_HOST_APPLE)
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/${osx_volname}.zip
COMMAND ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/macdeploy/macdeployqtplus ${macos_app} ${osx_volname} -translations-dir=${QT_TRANSLATIONS_DIR} -zip
DEPENDS ${CMAKE_BINARY_DIR}/${macos_app}/Contents/MacOS/Bitcoin-Qt
VERBATIM
)
add_custom_target(deploydir
DEPENDS ${CMAKE_BINARY_DIR}/${osx_volname}.zip
)
add_custom_target(deploy
DEPENDS ${CMAKE_BINARY_DIR}/${osx_volname}.zip
)
else()
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/dist/${macos_app}/Contents/MacOS/Bitcoin-Qt
COMMAND OTOOL=${OTOOL} ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/macdeploy/macdeployqtplus ${macos_app} ${osx_volname} -translations-dir=${QT_TRANSLATIONS_DIR}
Expand All @@ -133,19 +150,6 @@ function(add_macos_deploy_target)
DEPENDS ${CMAKE_BINARY_DIR}/dist/${osx_volname}.zip
)
add_dependencies(deploy deploydir)
else()
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/${osx_volname}.zip
COMMAND ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/macdeploy/macdeployqtplus ${macos_app} ${osx_volname} -translations-dir=${QT_TRANSLATIONS_DIR} -zip
DEPENDS ${CMAKE_BINARY_DIR}/${macos_app}/Contents/MacOS/Bitcoin-Qt
VERBATIM
)
add_custom_target(deploydir
DEPENDS ${CMAKE_BINARY_DIR}/${osx_volname}.zip
)
add_custom_target(deploy
DEPENDS ${CMAKE_BINARY_DIR}/${osx_volname}.zip
)
endif()
endif()
endfunction()
10 changes: 9 additions & 1 deletion depends/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,17 @@ $(host_prefix)/share/config.site : config.site.in $(host_prefix)/.stamp_$(final_
$< > $@
touch $@

ifeq ($(host),$(build))
crosscompiling=FALSE
else
crosscompiling=TRUE
endif

$(host_prefix)/toolchain.cmake : toolchain.cmake.in $(host_prefix)/.stamp_$(final_build_id)
@mkdir -p $(@D)
sed -e 's|@host_system@|$($(host_os)_cmake_system)|' \
sed -e 's|@depends_crosscompiling@|$(crosscompiling)|' \
-e 's|@host_system_name@|$($(host_os)_cmake_system_name)|' \
-e 's|@host_system_version@|$($(host_os)_cmake_system_version)|' \
-e 's|@host_arch@|$(host_arch)|' \
-e 's|@CC@|$(host_CC)|' \
-e 's|@CXX@|$(host_CXX)|' \
Expand Down
2 changes: 1 addition & 1 deletion depends/funcs.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ ifeq ($($(1)_type),build)
$(1)_cmake += -DCMAKE_INSTALL_RPATH:PATH="$$($($(1)_type)_prefix)/lib"
else
ifneq ($(host),$(build))
$(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system)
$(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system_name)
$(1)_cmake += -DCMAKE_C_COMPILER_TARGET=$(host)
$(1)_cmake += -DCMAKE_CXX_COMPILER_TARGET=$(host)
endif
Expand Down
2 changes: 1 addition & 1 deletion depends/hosts/android.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ android_CXXFLAGS=-std=$(CXX_STANDARD)
android_AR=$(ANDROID_TOOLCHAIN_BIN)/llvm-ar
android_RANLIB=$(ANDROID_TOOLCHAIN_BIN)/llvm-ranlib

android_cmake_system=Android
android_cmake_system_name=Android
5 changes: 4 additions & 1 deletion depends/hosts/darwin.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,7 @@ darwin_release_CXXFLAGS=$(darwin_release_CFLAGS)
darwin_debug_CFLAGS=-O1 -g
darwin_debug_CXXFLAGS=$(darwin_debug_CFLAGS)

darwin_cmake_system=Darwin
darwin_cmake_system_name=Darwin
# Darwin version, which corresponds to OSX_MIN_VERSION.
# See https://en.wikipedia.org/wiki/Darwin_(operating_system)
darwin_cmake_system_version=20.1
2 changes: 1 addition & 1 deletion depends/hosts/freebsd.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@ x86_64_freebsd_CC=$(default_host_CC) -m64
x86_64_freebsd_CXX=$(default_host_CXX) -m64
endif

freebsd_cmake_system=FreeBSD
freebsd_cmake_system_name=FreeBSD
5 changes: 4 additions & 1 deletion depends/hosts/linux.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,7 @@ i686_linux_CXX=$(default_host_CXX) -m32
x86_64_linux_CC=$(default_host_CC) -m64
x86_64_linux_CXX=$(default_host_CXX) -m64
endif
linux_cmake_system=Linux

linux_cmake_system_name=Linux
# Refer to doc/dependencies.md for the minimum required kernel.
linux_cmake_system_version=3.17.0
4 changes: 3 additions & 1 deletion depends/hosts/mingw32.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@ mingw32_debug_CXXFLAGS=$(mingw32_debug_CFLAGS)

mingw32_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC

mingw32_cmake_system=Windows
mingw32_cmake_system_name=Windows
# Windows 7 (NT 6.1).
mingw32_cmake_system_version=6.1
2 changes: 1 addition & 1 deletion depends/hosts/netbsd.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ x86_64_netbsd_CC=$(default_host_CC) -m64
x86_64_netbsd_CXX=$(default_host_CXX) -m64
endif

netbsd_cmake_system=NetBSD
netbsd_cmake_system_name=NetBSD
2 changes: 1 addition & 1 deletion depends/hosts/openbsd.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@ x86_64_openbsd_CC=$(default_host_CC) -m64
x86_64_openbsd_CXX=$(default_host_CXX) -m64
endif

openbsd_cmake_system=OpenBSD
openbsd_cmake_system_name=OpenBSD
12 changes: 10 additions & 2 deletions depends/toolchain.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@

# This file is expected to be highly volatile and may still change substantially.

set(CMAKE_SYSTEM_NAME @host_system@)
set(CMAKE_SYSTEM_PROCESSOR @host_arch@)
# If CMAKE_SYSTEM_NAME is set within a toolchain file, CMake will also
# set CMAKE_CROSSCOMPILING to TRUE, even if CMAKE_SYSTEM_NAME matches
# CMAKE_HOST_SYSTEM_NAME. To avoid potential misconfiguration of CMake,
# it is best not to touch CMAKE_SYSTEM_NAME unless cross-compiling is
# intended.
if(@depends_crosscompiling@)
Copy link

Choose a reason for hiding this comment

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

I really dislike that we still have to have this logic (and the depends counterpart), and I still don't really understand why it's needed. Now that the depends hacks are being removed, shouldn't we just be able to set CMAKE_SYSTEM_NAME, because the build system isn't going to operate specially for depends, and we've removed the CMAKE_CROSSCOMPILING usage?

In any case, according to the docs, if we are setting CMAKE_SYSTEM_NAME, we should also be setting CMAKE_SYSTEM_VERSION, or is that not needed?

CMAKE_SYSTEM_NAME may be set explicitly when first configuring a new build tree in order to enable cross compiling. In this case the CMAKE_SYSTEM_VERSION variable must also be set explicitly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

we've removed the CMAKE_CROSSCOMPILING usage

Right. But CMake internally still uses it, which might affect its behaviour (feature tests, compiler invocation string, testing framework etc).

Copy link
Owner Author

@hebasto hebasto May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In any case, according to the docs, if we are setting CMAKE_SYSTEM_NAME, we should also be setting CMAKE_SYSTEM_VERSION

FWIW, we are not doing this on the master branch:

bitcoin/depends/funcs.mk

Lines 193 to 197 in 7fcf4e9

ifneq ($(host),$(build))
$(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system)
$(1)_cmake += -DCMAKE_C_COMPILER_TARGET=$(host)
$(1)_cmake += -DCMAKE_CXX_COMPILER_TARGET=$(host)
endif

or is that not needed?

From the CMake source code, it follows that it is used for Android and Darwin platforms. For example, on Darwin, it affects on which flags will be used by default (-Wl,-search_paths_first).

Copy link

Choose a reason for hiding this comment

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

For example, on Darwin, it affects on which flags will be used by default (-Wl,-search_paths_first).

This seems like a more correct thing to do, so that's fine, but apparently it will have no effect with lld.

Copy link

Choose a reason for hiding this comment

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

I really dislike that we still have to have this logic (and the depends counterpart), and I still don't really understand why it's needed. Now that the depends hacks are being removed, shouldn't we just be able to set CMAKE_SYSTEM_NAME, because the build system isn't going to operate specially for depends, and we've removed the CMAKE_CROSSCOMPILING usage?

FWIW, in depends we do:

cross_compiling=maybe
host_alias="@HOST@"

Which I believe pretty much has the same effect for autotools that @hebasto is describing here for CMake.

Copy link

Choose a reason for hiding this comment

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

Then we should at least be documenting why we are avoiding this behaviour (besides calling something that isn't cross-compiling, cross-compiling, which is just confusing).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sorry, but I don't understand this point. We are not trying to avoid something, rather following the CMake docs.

When we want to enable cross-compiling, we set the CMAKE_SYSTEM_NAME variable, which sets the CMAKE_CROSSCOMPILING one explicitly.

When we don't want to enable cross-compiling, we skip touching the CMAKE_SYSTEM_NAME variable.

Anyway, I'll be happy to add a comment to this code. Do you mind suggesting a good wording for it?

Copy link

Choose a reason for hiding this comment

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

When we don't want to enable cross-compiling, we skip touching the CMAKE_SYSTEM_NAME variable.

This is what we are avoiding. Otherwise CMake decides things are cross-compilation, even when they are not. Otherwise we'd be free to set this all the time.

Copy link

Choose a reason for hiding this comment

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

@hebasto, for the wording, if @fanquake allows me here and agrees with it, perhaps something like:

# Setting CMAKE_SYSTEM_NAME only when cross-compiling is intended.
# This prevents CMake from misinterpreting the build as a cross-compilation,
# which could lead to incorrect configurations. If we are not cross-compiling,
# leave CMAKE_SYSTEM_NAME unset to allow CMake to correctly infer the build system.

If it's too long, I'd just get the first 2 lines and add the link to the CMAKE_CROSSCOMPILING documentation.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Then we should at least be documenting why we are avoiding this behaviour (besides calling something that isn't cross-compiling, cross-compiling, which is just confusing).

An explanatory comment has been added.

set(CMAKE_SYSTEM_NAME @host_system_name@)
set(CMAKE_SYSTEM_VERSION @host_system_version@)
set(CMAKE_SYSTEM_PROCESSOR @host_arch@)
endif()

function(split_compiler_launcher env_compiler launcher compiler)
set(${launcher})
Expand Down
Loading