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

Use of CMAKE_<lang>_COMPILER_FORCED on host-builds flag breaks third-party libraries #36558

Closed
palchak-google opened this issue Jun 25, 2021 · 6 comments · Fixed by #39014
Closed
Assignees
Labels
area: Build System bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Stale
Milestone

Comments

@palchak-google
Copy link
Contributor

Describe the bug
When building for the "native_posix" target using the "host" toolchain, it's possible to use third-party libraries (e.g. unit test frameworks). Many such frameworks provide CMake build files, and some rely upon CMake's idiomatic, portable target_compile_features() function to ensure the compiler supports all the necessary language features.

Zephyr's build system unconditionally sets the CMAKE_<lang>_COMPILER_FORCED global variables. This has the effect of inhibiting CMake's automatic discovery of compiler features. Without compiler feature discovery CMake generates an error at list time if target_compile_features() is used anywhere in the build.

To Reproduce
Steps to reproduce the behavior:

  1. Add the following line to the end of zephyr/samples/basic/minimal/CMakeLists.txt:
    target_compile_features(app PRIVATE cxx_std_17)
  2. Attempt to build the samples/basic/minimal project with BOARD=native_posix and ZEPHYR_TOOLCHAIN_VARIANT=host:
$ ZEPHYR_TOOLCHAIN_VARIANT=host west build . --board=native_posix
  1. See the following error:
CMake Error at CMakeLists.txt:21 (target_compile_features):
  target_compile_features no known features for CXX compiler

  "GNU"

  version 10.2.1.


-- Configuring incomplete, errors occurred!

Expected behavior
Build should complete normally and a valid zephyr.elf should be produced.

Environment (please complete the following information):

  • OS: Linux
  • Toolchain: Host (gcc v10.2.1)

Proposed Fix*

Zephyr should not set the CMAKE_<lang>_COMPILER_FORCED flags when the 'host' toolchain is used for the target. In such circumstances, the particulars of the host toolchain may not be known in advance (unlike what is typical with target-specific toolchains). Also the build may include third-party libraries that rely upon CMake's automatic discovery of compiler features.

The straightforward fix is as follows:

  1. Remove the following lines from zephyr/cmake/boilerplate.cmake:
# Prevent CMake from testing the toolchain
set(CMAKE_C_COMPILER_FORCED   1)
set(CMAKE_CXX_COMPILER_FORCED 1)
  1. Insert the same lines into zephyr/cmake/target_toolchain.cmake as follows:
if(NOT (COMPILER STREQUAL "host-gcc"))
  # Prevent CMake from testing the toolchain
  set(CMAKE_C_COMPILER_FORCED   1)
  set(CMAKE_CXX_COMPILER_FORCED 1)
  include(${TOOLCHAIN_ROOT}/cmake/toolchain/${ZEPHYR_TOOLCHAIN_VARIANT}/target.cmake)
endif()
@palchak-google palchak-google added the bug The issue is a bug, or the PR is fixing a bug label Jun 25, 2021
@nashif nashif added the priority: medium Medium impact/importance bug label Jun 29, 2021
@github-actions
Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Aug 29, 2021
@tejlmand tejlmand removed the Stale label Sep 7, 2021
@tejlmand
Copy link
Collaborator

tejlmand commented Sep 7, 2021

The reason for this behavior is because Zephyr build infrastructure performs it's own toolchain testing regarding valid compiler flags.
On top of this, the testing result is cached. This significantly reduces CMake configure time, as the testing only needs to be carried out one time per each toolchain, and later CMake invocation in any project doesn't need to test the same toolchain again.

This is why CMake's own testing is disabled.

What we should probably do is to tell CMake about the known compile features when we are testing the compiler.
For example, if we know the toolchain supports cxx_std_17 we should set:

set(CMAKE_CXX_COMPILE_FEATURES cxx_std_17)

This should probably be done in the compiler_flags.cmake for the compiler.

@palchak-google before posting a fix, I would like to test it out on your use-case, so could you provide a small snippet and a reference to the testing framework you are using. Thanks in advance.

@yashi
Copy link
Collaborator

yashi commented Sep 29, 2021

might be better to link from #8827

BTW, is this really a bug? It sounds like an enhancement.

tejlmand added a commit to tejlmand/zephyr that referenced this issue Sep 30, 2021
Fixes: zephyrproject-rtos#36558 zephyrproject-rtos#32577

This commit introduces CMAKE_C_COMPILE_FEATURES and
CMAKE_CXX_COMPILE_FEATURES.

This allows users to use the `target_compile_features()` in their own
code.

In Zephyr, the CMAKE_C/CXX_COMPILE_FEATURES are defined based on the
compiler and the Kconfig / CSTD setting.
Doing so ensures that a user compiling Zephyr with c99 and specifies
`target_compile_features(<target> ... c_std_11)` will get an error.
And similar if building Zephyr with C++ support and c++11, but testing
for `target_compile_features(<target> ... cxx_std_17)`.

For example in the C++ case, the user must ensure that Zephyr is
compiled with C++17, that is: CPLUSPLUS=y and STD_CPP17=y, in which case
the CMAKE_CXX_COMPILE_FEATURES will contain support for C++17 and thus
the `target_compile_features(<target> ... cxx_std_17)` will succeed.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@tejlmand
Copy link
Collaborator

tejlmand commented Sep 30, 2021

BTW, is this really a bug? It sounds like an enhancement.

I would say it depends on the direction from where you see this.
The fact that we have our own toolchain testing and thus breaks the way standard CMake works could be argued as a bug.
We shouldn't break built-in features of CMake (even though Zephyr itself doesn't use this feature).

But better support for downstream users can be argued as an enhancement.

But definitely not a medium bug, so I changed to low.

But a fix is ready here: #39014

@tejlmand tejlmand added priority: low Low impact/importance bug has-pr and removed priority: medium Medium impact/importance bug labels Sep 30, 2021
cfriedt pushed a commit that referenced this issue Oct 1, 2021
Fixes: #36558 #32577

This commit introduces CMAKE_C_COMPILE_FEATURES and
CMAKE_CXX_COMPILE_FEATURES.

This allows users to use the `target_compile_features()` in their own
code.

In Zephyr, the CMAKE_C/CXX_COMPILE_FEATURES are defined based on the
compiler and the Kconfig / CSTD setting.
Doing so ensures that a user compiling Zephyr with c99 and specifies
`target_compile_features(<target> ... c_std_11)` will get an error.
And similar if building Zephyr with C++ support and c++11, but testing
for `target_compile_features(<target> ... cxx_std_17)`.

For example in the C++ case, the user must ensure that Zephyr is
compiled with C++17, that is: CPLUSPLUS=y and STD_CPP17=y, in which case
the CMAKE_CXX_COMPILE_FEATURES will contain support for C++17 and thus
the `target_compile_features(<target> ... cxx_std_17)` will succeed.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
zephyrbot pushed a commit that referenced this issue Oct 1, 2021
Fixes: #36558 #32577

This commit introduces CMAKE_C_COMPILE_FEATURES and
CMAKE_CXX_COMPILE_FEATURES.

This allows users to use the `target_compile_features()` in their own
code.

In Zephyr, the CMAKE_C/CXX_COMPILE_FEATURES are defined based on the
compiler and the Kconfig / CSTD setting.
Doing so ensures that a user compiling Zephyr with c99 and specifies
`target_compile_features(<target> ... c_std_11)` will get an error.
And similar if building Zephyr with C++ support and c++11, but testing
for `target_compile_features(<target> ... cxx_std_17)`.

For example in the C++ case, the user must ensure that Zephyr is
compiled with C++17, that is: CPLUSPLUS=y and STD_CPP17=y, in which case
the CMAKE_CXX_COMPILE_FEATURES will contain support for C++17 and thus
the `target_compile_features(<target> ... cxx_std_17)` will succeed.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@nashif nashif reopened this Oct 1, 2021
@nashif nashif added this to the v2.7.1 milestone Oct 1, 2021
nashif pushed a commit that referenced this issue Oct 19, 2021
Fixes: #36558 #32577

This commit introduces CMAKE_C_COMPILE_FEATURES and
CMAKE_CXX_COMPILE_FEATURES.

This allows users to use the `target_compile_features()` in their own
code.

In Zephyr, the CMAKE_C/CXX_COMPILE_FEATURES are defined based on the
compiler and the Kconfig / CSTD setting.
Doing so ensures that a user compiling Zephyr with c99 and specifies
`target_compile_features(<target> ... c_std_11)` will get an error.
And similar if building Zephyr with C++ support and c++11, but testing
for `target_compile_features(<target> ... cxx_std_17)`.

For example in the C++ case, the user must ensure that Zephyr is
compiled with C++17, that is: CPLUSPLUS=y and STD_CPP17=y, in which case
the CMAKE_CXX_COMPILE_FEATURES will contain support for C++17 and thus
the `target_compile_features(<target> ... cxx_std_17)` will succeed.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
Rushybrook pushed a commit to Rushybrook/zephyr that referenced this issue Oct 21, 2021
Fixes: zephyrproject-rtos#36558 zephyrproject-rtos#32577

This commit introduces CMAKE_C_COMPILE_FEATURES and
CMAKE_CXX_COMPILE_FEATURES.

This allows users to use the `target_compile_features()` in their own
code.

In Zephyr, the CMAKE_C/CXX_COMPILE_FEATURES are defined based on the
compiler and the Kconfig / CSTD setting.
Doing so ensures that a user compiling Zephyr with c99 and specifies
`target_compile_features(<target> ... c_std_11)` will get an error.
And similar if building Zephyr with C++ support and c++11, but testing
for `target_compile_features(<target> ... cxx_std_17)`.

For example in the C++ case, the user must ensure that Zephyr is
compiled with C++17, that is: CPLUSPLUS=y and STD_CPP17=y, in which case
the CMAKE_CXX_COMPILE_FEATURES will contain support for C++17 and thus
the `target_compile_features(<target> ... cxx_std_17)` will succeed.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@github-actions
Copy link

github-actions bot commented Dec 1, 2021

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Dec 1, 2021
@tejlmand
Copy link
Collaborator

tejlmand commented Dec 1, 2021

This is fixed.

@tejlmand tejlmand closed this as completed Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants