Comment 1 Alexander Zeijlon 2024-02-01 10:12:37 CET

I have looked a bit at what we need to have in place before we can build a functioning CMake in cenbuild.

We apply two patches to the CMake source code before we build the package:

One that adds our own function, called find_tool(), that wraps the built-in function find_program() such that it makes sure to look for prefixed and suffixed tool chain versions of programs before it looks for base versions. This needs to be patched when upgrading, if the patch has not been committed upstream.

The other patch fixes an incompatibility between CMake 3.10 (the old version in cenbuild) and libuv 1.21.0. This patch is only needed until CMake < 3.12. And since we require CMake >= 3.17, the patch can be skipped when upgrading.

Comment 2 Alexander Zeijlon 2024-02-01 13:55:12 CET

It seems that we don't have to change much in our patch to retain the patched functionality when upgrading to the latest version (3.28.2).

Patching of Modules/CMakeFindBinUtils.cmake
###########################################

The original code has been modified since v 3.10 to take into account more variants of programs as input to find_program(). It is now checking for (in order, as long as both prefix and suffix are defined):

1. <toolchain_prefix><name><toolchain_suffix>
2. <toolchain_prefix><name>
3. <name><toolchain_suffix>
4. <name>

This behavior can be moved to / updated in our FindTool.cmake.

Patching of Modules/FindPkgConfig.cmake
#######################################

The original code has been modified since v. 3.10 such that it uses an additional argument, NAMES_PER_DIR, which tells find_program() to look for any of the input program names, one directory at a time. This is not something we want to do, since it means that find_program() will be more likely to choose the wrong version of a program instead of the tool chain version we are looking for.

This was added as a workaround for an issue when running on Windows [1], and does not affect us. Hence, we can remove the argument and apply the same change as the patch previously did for v. 3.10.

[1] https://gitlab.kitware.com/cmake/cmake/-/commit/c7bd2d0d

Comment 3 Alexander Zeijlon 2024-02-06 09:31:15 CET

find_program() basically performs a DFS, which means that it performs an exhaustive search for the first program name in the list in all provided paths before moving on to the next program name.

Hence, when generating the list of tool chain programs to look for, we want decorated names [1] to always occur before base names, such that we make sure that decorated alternatives are always found first.

[1] Prefixed and suffixed names.

Comment 4 Alexander Zeijlon 2024-02-06 09:33:59 CET

We also need to take into account that either one of prefix and suffix may be undefined, such that base names *accidentaly * end up before decorated names when generating the list.

The updated find_tool() takes this into account when building the list input to find_program().

Comment 5 Alexander Zeijlon 2024-02-06 12:55:47 CET

To test this behavior, I created a minimal CMake-project that basically only uses find_tool() to look for fake binaries in a predefined folder structure:
> cmake_minimum_required(VERSION 3.10)
> project(Hello LANGUAGES CXX)
> set(hints 
>   "~/workbench/cmake/src/"
>   "~/workbench/cmake/src/sub/"
>   "~/workbench/cmake/src/sub/subsub/"
>   )
> set(names "a++" "acc" "acc.bat" "acc.exe")
> set(prog )
> find_tool(prog NAMES ${names} HINTS ${hints}
>   NO_CMAKE_ENVIRONMENT_PATH NO_CMAKE_PATH)
> message(prog="${prog}")
I then tested the behavior of find_tool() by verifying that decorated files are always found if such files exist in the folder structure, according to the list ordering mentioned in comment 3 and comment 4.

Each of the paths in the hints list were populated with base name files. Decorated files were added to only some paths. E.g. only one decorated file "prefix-acc.exe" in the last folder in the hints list, which should result in the file being found after an exhaustive search for all other variants of all other base names.

Comment 6 Alexander Zeijlon 2024-02-06 14:31:56 CET

I also built new packages for FLTK using the upgraded CMake, and then built ThinLinc clients for windows and macOS. These were then run on their corresponding platforms without any apparent issues.

This should hopefully ensure that find_tool() works as expected, since it is used to find the correct (cross-)compiler when building packages for certain platforms.

Comment 8 Alexander Zeijlon 2024-02-07 13:32:33 CET

Note that both the old and new version of CMake were built with a bundled version of jsoncpp.

Ideally, we want to build all our packages from scratch without using bundled/precompiled dependencies. But in this case we opted for using the bundled version of jsoncpp since that library itself requires CMake to build, hence, a circular dependency. See bug 7777.

What's new in CMake version 3.28.2, is that it also requires us to compile with bundled cppdap if we compile with bundled jsoncpp.

We could have looked at patching the bootstrap script to allow for a non bundled version of cppdap. But for now we use the bundled one, since it is not clear what potential side effects this could introduce to the build.

Comment 9 Alexander Zeijlon 2024-02-07 13:39:36 CET

> MUST:
> * We should provide a version of CMake that fulfills the build
>   requirements for all packages we need in cenbuild.
This bug was broken out from Bug 8172, where the requirement is CMake >= 3.17. We are now providing the newest version, which is 3.28.

> * The upgraded version should still be able to properly identify tool
>   chain programs when it is used for cross compilation in cenbuild.
find_tool() has been updated to match the behavior in the new CMake version, And it's been verified to properly find the tool chain programs we require for cross compilation to Windows and macOS.

> SHOULD:
> * Workarounds that are in place for the previous version of CMake should
>   be removed if they are no longer needed.
The workaround for libuv has been removed since it is no longer needed in CMake >= 3.12.0.

> COULD:
> * We should provide a modern version of CMake in cenbuild such that we are
>   likely to comply with future package requirements.
CMake has been upgraded to the newest version.

Comment 11 Linn 2024-02-09 15:40:24 CET

> MUST:
> * We should provide a version of CMake that fulfills the build
>   requirements for all packages we need in cenbuild.
Only 2 packages have a build requirement on CMake - FLTK and libjepg. Tested that both these packages could be built.

> * The upgraded version should still be able to properly identify tool
> chain programs when it is used for cross compilation in cenbuild.
Tested this by installing the previously built FLTK package on my workstation, where I built the Windows and macOS clients. Tested running the client on Windows 10 and macOS 14 and looked around in the client options, did not notice anything out of place.

> SHOULD:
> * Workarounds that are in place for the previous version of CMake should be
> removed if they are no longer needed.
Yes, the libuv workaround has been removed. The other patch with find_tool() has been tweaked to fit into the updated cmake version.

> COULD:
> 
> * We should provide a modern version of CMake in cenbuild such that we are
> likely to comply with future package requirements.
Yes, as mentioned CMake was updated to the latest version, 3.28.2.


Also looked through the commits, looks good. Looked extra at commit r40752, and can confirm that we now handle HINTS the same way as before. Additionally, when updating the cendio-build packages, the CMake package is correctly upgraded.

Comment 12 Alexander Zeijlon 2024-02-26 08:24:37 CET

I have submitted a merge request for our find_tool() to upstream CMake, https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9289