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

CMake improvements #621

Open
2 tasks done
purpleKarrot opened this issue Oct 20, 2024 · 0 comments
Open
2 tasks done

CMake improvements #621

purpleKarrot opened this issue Oct 20, 2024 · 0 comments

Comments

@purpleKarrot
Copy link

purpleKarrot commented Oct 20, 2024

Checklist

  • I agree to follow the Code of Conduct that this project adheres to.
  • 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:

test_arg = "check";

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 (#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: #232 (comment)

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 (#15) or swift build (for https://github.com/flathub/org.freedesktop.Sdk.Extension.swift5).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant