flatpak-builder icon indicating copy to clipboard operation
flatpak-builder copied to clipboard

CMake improvements

Open purpleKarrot opened this issue 4 months ago • 0 comments

Checklist

  • [X] I agree to follow the Code of Conduct that this project adheres to.
  • [X] I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Suggestion

Here, I am dumping a few ideas of how I think the current integration of CMake should be improved. If there is enough interest, I may split this into concrete tasks and prepare a few patches. I just want to make sure this has a chance of landing, before I spend more time. I saw that some PRs here are quite old, so I am not sure how actively maintained this project even is.

Configure Step

When a single directory is passed to cmake, that directory is interpreted as a source directory or build directory depending on the directory contents, which may lead to surprises and therefore should be avoided.

It is possible and recommended to specify the two directories explicitly:

cmake -S <source> -B <build> -G <generator> [<options>]

Build step

Instead of constructing a command line for the generated buildsystem, it is possible and recommended to use an abstraction provided by CMake:

cmake --build <dir> --parallel <jobs>

Test step

For Makefile generators (eg "Unix Makefiles", "Ninja", etc), CMake creates a build target named "test". For IDEs (eg "Visual Studio", "Xcode") the corresponding build target is called "RUN_TESTS" instead. CMake does not create a target called "check" anywhere. I have come across blog posts that recommend creating a target with that name, but it is not something that flatpak-builder should rely on, so this should be considered a bug:

https://github.com/flatpak/flatpak-builder/blob/f996a7949076ef1dc85c602da85b981b0a173a23/src/builder-module.c#L1889

Note that this build target effectively invokes ctest. What do you think is the effect of calling make test -j2? It tells the buildsystem to launch two processes in parallel. But the buildsystem only has one job: Invoking ctest, so it just launches one process. And since CTest is not instructed to use parallelization, it launches all tests sequencially. Maybe that was not the intention of passing -j2 (https://github.com/flatpak/flatpak-builder/issues/461).

A better approach is to launch CTest directly:

ctest --test-dir <dir> --parallel <jobs> --output-on-failure --stop-on-failure

Install step

Like the build target for running tests, the build target for installing a project also is named differently depending on the generator in use. Again, this always uses the same command under the hood. And again, calling it directly is more flexible:

cmake --install <dir> --prefix <prefix> --parallel <jobs>

Considerations:

All commands allow specifying the build directory as an option and do not depend on the current working directory. In the configure step, the directory is created if it does not exist.

The used generator does not affect any command line (apart from the -G option). Instead of having two supported buildsystems cmake and cmake-ninja, it would be sufficient to have just one, as long as there is a way to specify the generator:

buildsystem: cmake
config-opts:
  - -G Ninja

or:

buildsystem: cmake
cmake-generator: Ninja

Ideally, the used generator should be considered an implementation detail of flatpak-builder. Just because CMake uses "Unix Makefiles" as the default on Linux, does not mean that flatpak-builder has to use the same default. It could just as well default to Ninja, or change the default in the future. Clients specifying the generator should be the exception.

The exact filenames that CMake reads and writes should be considered implementation details of CMake. I do not agree that flatpak-builder should check the existence of CMakeLists.txt, Makefile, or build.ninja. If a necessary file is missing, the above commands report failures.

Out of source builds should be the default for this reason: https://github.com/flatpak/flatpak-builder/issues/232#issuecomment-353585099

The best way to use ccache is by setting the CMAKE_<LANG>_COMPILER_LAUNCHER variables. Aparently, supporting different buildsystems goes beyond constructing command line strings. it also involves environment variables and controlling the working directory. A higher level abstraction, like a buildsystem interface, may simplify adding support for other buildsystems like cargo (https://github.com/flatpak/flatpak-builder/issues/15) or swift build (for https://github.com/flathub/org.freedesktop.Sdk.Extension.swift5).

purpleKarrot avatar Oct 20 '24 21:10 purpleKarrot