Revert breaking change to CMAKE_SYSTEM_NAME and unnecessary introduction of cmake/modules/Platform/Zephyr.cmake #71468
Conversation
zephyrbot
requested review from
57300,
jeremybettis,
nashif,
nordicjm and
tejlmand
|
@nordicjm This is not reverting the whole of #67997 as that PR had 4 separate commits. It is in effect reverting c2b6016 but also providing an alternative place for the adjustment of TARGET_SUPPORTS_SHARED_LIBS which is still necessary for Xtensa support. The intention is not to undo #67997 but fix a breaking change it unnecessarily introduced. |
cmake/modules/kernel.cmake
Outdated
| # Enable dynamic library support required by CONFIG_XTENSA for CONFIG_LLEXT support. | ||
| if(CONFIG_XTENSA AND CONFIG_LLEXT) |
There was a problem hiding this comment.
64e7d85 added a Kconfig to specify the object type for the llext being built, so it's not tied to the arch anymore.
Thus, I believe this should now be:
| # Enable dynamic library support required by CONFIG_XTENSA for CONFIG_LLEXT support. | |
| if(CONFIG_XTENSA AND CONFIG_LLEXT) | |
| # Enable dynamic library support when required by LLEXT. | |
| if(CONFIG_LLEXT AND CONFIG_LLEXT_TYPE_SHAREDLIB) |
There was a problem hiding this comment.
Thx @pillo79
I amended the branch to include your suggestion.
There was a problem hiding this comment.
Only thing is there is now a compliance check failing due to CONFIG_LLEXT_TYPE_SHAREDLIB. So I'm confused.
EDIT: My bad. I had not done a proper rebase onto main. That should be good now.
EDIT2: ha now I see the actual option name is CONFIG_LLEXT_TYPE_ELF_SHAREDLIB. Fixed.
There was a problem hiding this comment.
Sorry for the misdirection - in fact it's CONFIG_LLEXT_TYPE_ELF_SHAREDLIB, my bad! 😉
nlebedenco
force-pushed
the
patch-4
branch
2 times, most recently
from
74e6bb8
to
be6b51e
Compare
This commit reverts a breaking change in CMAKE_SYSTEM_NAME introduced by "Zephyr" back to "Generic") and removes the file `cmake/modules/Platform/Zephyr`. Both changes in the aforementioned PR were only introduced to ultimately modify the value of the global CMake property TARGET_SUPPORTS_SHARED_LIBS for the special case of building for Xtensa with LLEXT. The modification of CMAKE_SYSTEM_NAME is considered a breaking change because it has the potential to alter the build of any non-trivial project that previously checked for the "Generic" system identifier as corresponding to Zephyr - for example by doing `if (CMAKE_SYSTEM_NAME STREQUAL "Generic")`. Such builds may now break in many ways including silently when there is no `else()` clause with a `message()` to alert the user that a whole configuration block had been skipped. In essence, that CMAKE_SYSTEM_NAME modification was only introduced in order to have CMake to load `cmake/modules/Platform/Zephyr.cmake` which in turn adjusted the value of TARGET_SUPPORTS_SHARED_LIBS. But the use of a CMake platform file like this is ineffective for non-trivial projects where one or more top level CMake `project()` calls may happen before the first call to `find_package(Zephyr)` because in such cases CMAKE_MODULE_PATH will not have been modified yet to contain the path to <Zephyr_ROOT>/cmake/modules and thus no platform file will be include by CMake. This patch moves the conditional override of TARGET_SUPPORTS_SHARED_LIBS needed by some archs (e.g. Xtensa) into the `kernel.cmake` module which is known to be the first call to `project()` that enables any language and thus the one that must come before any artifact target can be defined. Note commit 64e7d85 added a Kconfig to specify the object type of llext being built, so it's not tied to the arch anymore but to the CONFIG_LLEXT_TYPE_ELF_SHAREDLIB option. Signed-off-by: Nicolas Lebedenco <nicolas@lebedenco.net>
|
Thank you all for the reviews. |
This PR reverts a breaking change in CMAKE_SYSTEM_NAME introduced by #67997 (changing it from "Zephyr" back to "Generic") and removes the file
cmake/modules/Platform/Zephyr.Both changes in the aforementioned PR were only introduced to ultimately modify the value of the global CMake property TARGET_SUPPORTS_SHARED_LIBS for the special case of building for Xtensa with LLEXT.
The modification of CMAKE_SYSTEM_NAME is considered a breaking change because it has the potential to alter the build of any non-trivial project that previously checked for the "Generic" system identifier as corresponding to Zephyr - for example by doing
if (CMAKE_SYSTEM_NAME STREQUAL "Generic"). Such builds may now break in many ways including silently when there is no else clause with amessage(FATAL_ERROR)ormessage(WARNING)to alert the user that a wholeconfiguration block had been skipped.
In essence, that CMAKE_SYSTEM_NAME modification was only introduced in order to have CMake to load
cmake/modules/Platform/Zephyr.cmakewhich in turn adjusted the value of TARGET_SUPPORTS_SHARED_LIBS.But the use of a CMake platform file is ineffective for non-trivial projects where one or more top level CMake projects may be defined before the first call to
find_package(Zephyr)because in such cases CMAKE_MODULE_PATH will not have been modified yet to contain the path to<Zephyr_ROOT>/cmake/modules.This patch moves the conditional override of TARGET_SUPPORTS_SHARED_LIBS into the
kernel.cmakemodule which is known to be the first call toproject()that enables any language and thus the one that must comebefore any artifact target can be defined.
This is the command line one would use prior to #67997 (or with the
patched branch) followed by its respective output:
This is the command line one would use with the current main followed by its respective output:
Note how CMake is complaining that a platform cmake module could not be found for the target system. In fact, the file
<Zephyr_ROOT>/cmake/modules/Platform/Zephyr.cmake is never included. The configuration seems to succeed but
the stock
Generic.cmakeshipped with CMake was not included either.Note that omiting
-DCMAKE_SYSTEM_NAME=Zephyris not an option because without it CMake will set CMAKE_SYSTEM_NAME tothe value of CMAKE_HOST_SYSTEM_NAME resulting in this:
Again <Zephyr_ROOT>/cmake/modules/Platform/Zephyr.cmake is never included. This can be verified by temporarily adding
message(STATUS "Included ${CMAKE_CURRENT_LIST_FILE}")at the top of the file. The configuration seems to succeed butthe stock
Generic.cmakeshipped with CMake was not included either. Instead it wasWindows.cmakethat was included.Here is the example project that I used to produce the output listed.
SomeTopLevelProject.zip