[FIXUP] cmake: Do not trigger cross-compiling unnecessarily#125
Conversation
|
Friendly ping @fanquake, @TheCharlatan, @m3dwards, @theuni :) |
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
tACK 5c50fe9
Nice catch! All this testing and didn't notice it 🤦🏻♂️🙏🏼.
Tested on Ubuntu 22.04, building both first without specifying HOST and then cross for Windows.
Before (cmake-staging) building for linux host in linux triggers cross-compilation which is wrong.
cmake -B build --toolchain depends/x86_64-pc-linux-gnu/share/toolchain.cmake
...
Configure summary
=================
...
Cross compiling ....................... TRUE, for Linux, x86_64
...
That's wrong.
cmake -B build --toolchain depends/x86_64-w64-mingw32/share/toolchain.cmake
...
Configure summary
=================
...
Cross compiling ....................... TRUE, for Windows, x86_64
...
That's correct.
This PR fixes the issue.
cmake -B build --toolchain depends/x86_64-pc-linux-gnu/share/toolchain.cmake
...
Configure summary
=================
...
Cross compiling ....................... FALSE
...
Now it's correct.
cmake -B build --toolchain depends/x86_64-w64-mingw32/share/toolchain.cmake
...
Configure summary
=================
...
Cross compiling ....................... TRUE, for Windows, x86_64
...
That's still correct.
hebasto
force-pushed
the
240321-cmake-CP
branch
from
5c50fe9 to
e351106
Compare
|
Rebased. |
hebasto
force-pushed
the
240321-cmake-CP
branch
2 times, most recently
from
6546686 to
e7e67d2
Compare
| else() | ||
| set(c_compiler_command ${CMAKE_C_COMPILER}) | ||
| endif() | ||
| set(c_compiler_command "${CMAKE_C_COMPILER} ${CMAKE_C_COMPILER_ARG1}") |
There was a problem hiding this comment.
Shouldn't this just be CMAKE_C_COMPILER? Or why has it now changed from that?
There was a problem hiding this comment.
Please consider the following example:
$ cat CMakeLists.txt
cmake_minimum_required(VERSION 3.22)
set(CMAKE_CXX_COMPILER clang++ -stdlib=libc++ -std=c++20)
project(compiler_demo CXX)
message("CMAKE_CXX_COMPILER: ${CMAKE_CXX_COMPILER}")
message("CMAKE_CXX_COMPILER_ARG1: ${CMAKE_CXX_COMPILER_ARG1}")
$ 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/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMAKE_CXX_COMPILER: /usr/bin/clang++
CMAKE_CXX_COMPILER_ARG1: -stdlib=libc++ -std=c++20
-- Configuring done (0.2s)
-- Generating done (0.0s)
-- Build files have been written to: /home/hebasto/CMAKE_COMPILER_DEMO/build
When a language being initialized, CMake splits the compiler command into two parts, i.e., a binary and its options.
|
|
||
| set(CMAKE_SYSTEM_NAME @host_system@) | ||
| set(CMAKE_SYSTEM_PROCESSOR @host_arch@) | ||
| if(@depends_crosscompiling@) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we've removed the
CMAKE_CROSSCOMPILINGusage
Right. But CMake internally still uses it, which might affect its behaviour (feature tests, compiler invocation string, testing framework etc).
There was a problem hiding this comment.
In any case, according to the docs, if we are setting
CMAKE_SYSTEM_NAME, we should also be settingCMAKE_SYSTEM_VERSION
FWIW, we are not doing this on the master branch:
Lines 193 to 197 in 7fcf4e9
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theCMAKE_CROSSCOMPILINGusage?
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
hebasto
force-pushed
the
240321-cmake-CP
branch
4 times, most recently
from
a8b35ef to
9babd89
Compare
|
My Guix build: |
| else() | ||
| set(c_compiler_command ${CMAKE_C_COMPILER}) | ||
| endif() | ||
| list(JOIN CMAKE_C_COMPILER_LAUNCHER " " c_compiler_command) |
There was a problem hiding this comment.
Looks like this has just taken on the depends macos ccache hack behaviour for everything, and I still don't understand why?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
set(CMAKE_CXX_COMPILER env -u A_VAR clang++)
Where are we doing this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we get some documentation.
Sure. That's what I can suggest to consider:
- The
CMAKE_<LANG>_COMPILERvariable 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.
- 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- The
<LANG>_COMPILER_LAUNCHERproperty 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.
There was a problem hiding this comment.
Sure. A comment has been added.
hebasto
force-pushed
the
240321-cmake-CP
branch
2 times, most recently
from
ad6d890 to
9babd89
Compare
hebasto
force-pushed
the
240321-cmake-CP
branch
2 times, most recently
from
ebb89af to
a818bf5
Compare
729632c fixup! ci: Test CMake edge cases (Hennadii Stepanov) 107bdf8 fixup! ci: Test CMake edge cases (Hennadii Stepanov) Pull request description: 1. The first commit forces the build step in the native Windows jobs to fail if build fails. Otherwise, the build step might falsely succeed because the last command exit code determines the return status of the step. I've noticed such wrong behaviour while working on #182. 2. The second commit pulled from #125. ACKs for top commit: m3dwards: utACK 729632c Tree-SHA512: 3530f8c2d60896665adff52caaf88460e18bf1087f9350337a4f191558023ee3484b0639673f90c48d6c24074338b640d5cfde067bb90344dfebaa2d6173f1cc
|
Rebased. |
|
Rebased on top of the #188. |
hebasto
force-pushed
the
240321-cmake-CP
branch
2 times, most recently
from
aefa673 to
eb51d1b
Compare
|
Rebased. It's one commit only now :) |
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
reACK eb51d1b
Validated the changes from my first tACK and I've been following discussions with @fanquake.
Re-tested eb51d1b, the issue is still fixed with this version.
This point is still valid and needs to be addressed. |
Considering #125 (comment) and the fact that we are defining cross-compiling toolchain options explicitly, I see no point in adding |
Why not? That comment just says we aren't doing it in master (but should we be?) and that it's needed for Darwin. So I don't really follow in why there's no point in doing it now? |
The |
|
My Guix build: |
|
Guix build (aarch64): |
| 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). |
There was a problem hiding this comment.
Does this mean that ccache and env can't be used simultaneously? Basically meaning that ccache can't be used for darwin builds?
There was a problem hiding this comment.
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 ...
|
Tested this on macos natively, with and without ccache, and seems to work correctly. |
5 participants
As it was pointed out during today's call, the toolchain file, which is generated by the depends build system, unnecessarily triggers CMake's cross-compiling mode by setting the
CMAKE_SYSTEM_NAMEvariable even the host and build systems coincides.That behavior deviates from the current master branch and potentially might have unwanted/unexpected side effects.
This PR fixes this issue.
All Guix builds remain cross-compiling.