For the execution of builds within the oemake project, the build will source the environment setup and then execute the underlying build tool. As cmake has some direct support for toolchain builds, a toolchain file is provided to configure the toolchain for cmake. This file root/meta/recipes-devtools/cmake/cmake/OEToolchainConfig.cmake is provided largely unchanged through a long string of yocto versions and produces some problems. Code in question: ``` set( CMAKE_C_FLAGS $ENV{CFLAGS} CACHE STRING "" FORCE ) set( CMAKE_CXX_FLAGS $ENV{CXXFLAGS} CACHE STRING "" FORCE ) set( CMAKE_ASM_FLAGS ${CMAKE_C_FLAGS} CACHE STRING "" FORCE ) set( CMAKE_LDFLAGS_FLAGS ${CMAKE_CXX_FLAGS} CACHE STRING "" FORCE ) ``` 1. CMake already reads the environment variables to initialize CMAKE_C_FLAGS from CFLAGS, CMAKE_CXX_FLAGS with CXX_FLAGS and CMAKE_EXE_LINKER_FLAGS from LDFLAGS. This mechanism has extension Points to override the default settings with CMAKE_C_FLAGS_INIT variables and others. Doing this manually disables all these extension points and makes it significantly harder to control the build. 2. This code FORCEs the variable to their predefined values, taking away the option to add further command line parameters in the cmake project. A lot of cmake projects rely on this capability to control (enable or suppress) warnings. All of this will be silently overridden, which warnings or other compiler options suddenly and inexplicably then don't work in toolchain builds. As the toolchain file already has the highest priority in the cmake system (loaded first), there is no need to force these things. 3. Build mode in cmake is controlled by choosing CMAKE_BUILD_TYPE to Debug, Release, MinsizeRel or others. Instead of indirectly forcing Build flags like -O2 -g or others in here, it would be much more suitable to choose an appropriate build mode. The problem with that is that openembedded does a weird combination of a debug and release build in one. CMake does not have a suitable mode for that at all. openembedded basically says that a release build is a debug build with the debug symbols stripped, which is completely at odds with modern build configuration tools and compiler work. (as there are -Og, -O2/3 and -Os optimization modes which configure for a certain purpose, and combinations like -O2 and -g degrade build compiler performance significantly). Here it could be useful to add a CMAKE_BUILD_TYPE, but many projects do not support any additional build mode. The best option available would perhaps be to set this mode to Release and add the -g flag as a generic option for the Release and Minsizerel modes to always build with debugging symbols. 4. The ASM_FLAGS are set from the C Flags. This is not necessarily the correct option and just letting CMake pick up the asm flags from the shell environment instead (i.e. do nothing) would result in the correct flags. 5. The linker flags are set to the C++ compiler flags. This does not make any sense at all, as the compiler flags and linker flags are different things. Also for c projects, this is definitively the wrong flags. This would not have worked at all if it wasn't for point 6. 6. The Linker Flags are spelt wrong, so the last line has zero effect. CMAKE_LDFLAGS_FLAGS is not a cmake variable. I believe the author meant CMAKE_LD_FLAGS, which is not a valid cmake variable either. CMake controls the linker flags through the variables CMAKE_SHARED_LINKER_FLAGS, CMAKE_STATIC_LINKER_FLAGS, CMAKE_EXE_LINKER_FLAGS, CMAKE_MODULE_LINKER_FLAGS. CMake already takes the LDFLAGS from the environment variable and populates them accordingly, so as this misspelling does nothing, as a result, everything works fine, as there was never the need to do anything in first place. Furthermore, the method to extract the processor is quite interesting and begs the question of whether it would be better to have a direct variable in the environment setup instead of parsing it out from the sysroot tree, which can break.
These all sound like good observations and some of them sound worrying. If something is invalid syntax entirely (as in 6), it can probably be removed. This would also mean the issue in 5 isn't an issue as it is ignored (and can be removed). For 3, we use the debug mode as it then generates files in the form closest to what OE needs and allows us to use our debug splitting code. I don't think cmake worked well in release mode going from memory of previous discussions of this. 4 and 1 are basically the same issue. In the past we've had issues with the values being overwritten and not uses from the environment correctly as cmake thinks it knows best. OE is unusual in that we are quite specific about the flags we want used and tend to know what we're targetting. I'd imagine we could test removing these but it may cause other issues, they were likely added for a reason. cmake has also changed so it make longer me the case, I don't know. This one does give me worry about regressions though. 2 is basically related to my worries about 4 and 1. Whilst some projects may use it to control warnings, other do things which break our builds. I'm not sure how we tell the diffrence between them or control this. For issues 5/6, please send a patch, that should be straight forward. For 1/4 we could test a patch removing this and see how much breaks. Most of the project core maintainers are not cmake experts so we rely on the advice of others to keep this all working.
Martin, Thanks for the report. Are you able to send patches for 5 and 6 to the oe-core list? Let me know if you need help.
If I did everything correctly, a patch for 5 and 6 should be available at https://lists.openembedded.org/g/openembedded-core/topic/patch_cmake_remove_bogus/89070351
Thanks Martin, I see you patch on the list and in the master-next branch: https://git.openembedded.org/openembedded-core/log/?h=master-next d4569a142d cmake: remove bogus CMAKE_LDFLAGS_FLAGS definition from toolchain file so once/if it gets merged to master, that one issue will be resolved. Please copy the commit link here. I assume that this one patch is not all that needs to be done. How do you want to handle the rest of the issues listed in this bug?
I sent a patch to fix issue 4, it is https://lists.openembedded.org/g/openembedded-core/topic/patch_cmake_read_asm_flags/89138781. The problem currently is, that if the library cmake code sets the build mode to Release (which a lot of libraries do) cmake will append a -O3 -DNDEBUG to the call. This will override the previously specified -O2 or -Os from the CXXFLAGS and produce libraries with a different optimization level than what the toolchains want. (the -g luckily goes trough) So I have serious doubts if the cmake recipes in openembedded are currently built with the configured optimization level at all, as a lot of cmake projects set a default a build type (it is considered good practice). An IMHO better option would be to FORCE a build mode (like this: SET(CMAKE_BUILD_TYPE RelWithDebInfo CACHE STRING "" FORCE) and then force set the CMAKE_CXX_FLAGS_RELWITHDEBINFO and CMAKE_C_FLAGS_RELWITHDEBINFO and CMAKE_ASM_FLAGS_RELWITHDEBINFO to the desired "-O2 -g -DNDEBUG" parameters (which are the cmake default anyway but forcing them would prevent them from being changed in projects). Projects then set CMAKE_C_FLAGS and CMAKE_CXX_FLAGS as they wish, but debug symbols will be turned on and the atomization level will be corrected to -O2, no matter what the libraries cmake configuration does. The other thing that worries me is the -DNDEBUG flag. Currently it is set for all projects setting a Release/RelWithDebInfo mode in their cmake config but not for the others, which is inconsistent and a bit arbitrary. Having assertions enabled usually degrades performance from a few percent to tenfold for a lot of higher-level libraries, (like Eigen, etc). But it enables a lot of bounds checking and other useful sanity checks in debug mode which help immensely when debugging problems. The current situation of having it inconsistently half-enabled does not really serve anyone. There is little possibility to consistently disable it in CMake (although we could try), but we could at least consistently enable it, and this is what the above definition of flags would do. Here the fact that Debug and Release are flavours and not separate builds limits the options, but I would say when forced to choose I care about decent performance in the Release flavour more than about debugging experience in the debug flavour.
Thanks to Martina and others several commits/fixes for cmake have been merged to master: https://git.openembedded.org/openembedded-core/log/?qt=grep&q=cmake I think there are still issues listed in Martin's original report that should be dealt with so the bug should stay opened. Alternatvely, new defects could be opened for the remaining issues; it's really up to you Martin, what do you want to do?
Martin, Please summarize what remains to be done. Are you willing and able to work on this issue in this release?
I sent a new v2 patch for issue 4 in this discussion, building on the feedback from different reviewers. As I am confident the variable is also misspelt and does not have a real effect, I proposed its removal. It can be found here: https://lists.openembedded.org/g/openembedded-core/message/168243. Remaining are the issues 1 ( to a lesser degree), 2 and 3. I would assume that forcing the cmake build mode to RelWithDebInfo would be worth a try to solve the problem. Then the cmake cxx flags do no need to be forced at all, also the build mode flags will be appended later and always ensure to turn optimizations on and add debug symbols. This will require patches for libraries which also force the build mode, which is considered bad style in modern cmake code for libraries. RelWithDebInfo translates to -O2 -g -DNDEBUG. This would disable assertions on top of current code, which would have huge positive performance implications for libraries not specifying a Release build as default. It would take the assertions away from the debug flavor also, which is sad, but in one compilation, a reasonable tradeoff in my eyes. I can provide a patch which forces the build mode instead, but I have no clue how to perform reasonable testing on that patch, and I am almost certain that there is a set of small fixes required in recipes using cmake for some of the modules. In these failure cases, the resulting libraries would not contain debug symbols, although we want them to do so. Any ideas how to that?
Hi Martin, Re (4) I see the patch hasn't yet been merged. Do you still think that is the right behaviour? Re (3), a variable for the build type that defaults to RelWithDebug sounds like the right thing to do, as we expect normal builds to have debug symbols (which are stripped later) but be optimised. If you can verify that eg core-image-sato doesn't break, feel free to send a patch and we can run it through the autobuilder. Re (1), if cmake actually does the right thing with the environment, then the toolchain can be simplified a lot, yes. The class has been like that "forever" (since 2009) so no doubt cmake has changed since then. If you understand how cmake works then a cleanup is encouraged, we can run any patches through the autobuilder to verify they work. Remember that there are two toolchains involved: the toolchain generated by meta/classes/cmake.bbclass which is used in normal builds by bitbake, and the toolchains in meta/recipes-devtools/cmake/cmake/ which are used in a SDK. They're both needed in different contexts, but any changes should be synchronised as changes in one should be applicable to the other. Finally, what is oemake?
Martin, The YP bug reivew (RP really) doesn't think we're done yet. Can we finish up or create sub-issues to make smaller tasks?