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

Add CMakeLists.txt to project as build method #438

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

codeinred
Copy link

@codeinred codeinred commented Sep 23, 2021

Hi! I created a CMakeLists.txt file for the project. It can build both the library itself and also the tests. This PR also enables running tests with ctest. Tests can now be built and run via:

cmake -B build
cmake --build build
cd build && ctest

Aside from support for ctest, including a CMakeLists.txt in the library allows it to be added as a dependency via the FetchContent interface, so that other projects build and link against liburing on supported kernel versions even if no system installation is present:

# Example code illustrating how another project could automatically 
# link against liburing
FetchContent_Declare(
  liburing
  GIT_REPOSITORY https://github.com/axboe/liburing.git
  GIT_TAG        master)
FetchContent_MakeAvailable(liburing)

Having this can be useful on HPC systems or other shared clusters where getting a library approved for system-wide install is a difficult process. It also makes it easier for users to test and compare more recent versions of the library to an existing system installation.

Other features include:

  • Out of source builds (like meson, although it's significantly less complex to support CMake, requiring only one file)
  • Automatic generation of compile_commands.json, which is used by language servers like clangd, cquery, and ccls to enable automatic linting as well as to provide support for other features of modern IDEs. (This is placed in the build directory when generated, and isn't included in the project)
  • The CMakeLists.txt file does not have to be updated if additional tests are added in test/. All .c files in the test/ directory are added as tests, except for helper.c.

Please let me know if you have any questions or feedback, and I'll do my best to address them!

@ammarfaizi2
Copy link
Contributor

Hi @codeinred,
Jens and some areas on Linux kernel require the commit to contain Signed-off-by tag.
The format for commit message is:

  • First line is title
  • Empty line
  • Description (may be omited for trivial changes)
  • Empty line again (if it has description)
  • Then a Signed-off-by with your name and email.

Description should be word wrapped 72 chars. Some things should not be word-wrapped, they may be some kind of quoted text - long compiler error messages, oops reports, or whatever things that have a certain specific format.

See the past commit for example: 72a9439

Created CMakeLists.txt and updated .gitignore accordingly. Having
support for CMake enables liburing  to be added as a dependency via the
FetchContent interface, so that other projects build and link against
liburing on supported kernel versions even if no system installation is
present. This can be useful on HPC systems or other shared clusters
where the approval process for system-wide installation is slow. It also
makes it easier for users to test and compare more recent versions of
the library to an existing system installation.

Signed-off-by: Alecto Perez <[email protected]>
@codeinred
Copy link
Author

Hi @ammarfaizi2, thank you so much for letting me know! I redid it as a single commit with the appropriate formatting!

@eli-schwartz
Copy link
Contributor

There is also #297 by the way.

Out of source builds (like meson, although it's significantly less complex to support CMake, requiring only one file)

I'm not sure what this sentence means? cmake only requires one file to support out of source builds?

@codeinred
Copy link
Author

codeinred commented Sep 24, 2021

Sorry about that! I meant that, like the meson build system, CMake supports out of source builds. But supporting CMake adds less complexity than supporting Meson, because CMake only requires the one file, whereas an analogous pull request to support Meson required 7 files and 384 lines of code.

If you prefer, it is possible to split the CMakeLists.txt into multiple files, but as it stands it’s a pretty straightforward build, and so that isn’t really necessary.

@eli-schwartz
Copy link
Contributor

As far as I can tell, this PR includes no install targets and definitely doesn't install the man pages or pkg-config files.

It also runs the configure shell script (440 lines) to generate an important header file, while the meson project files implement that internally at the cost of approximately a hundred lines.

It also implements the testsuite by running each executable on its own, but the official Makefile uses a shell script wrapper which processes the output of the test programs, and so does the contributed meson.build.

i.e. the meson files are complex and large precisely because they are a complete port and fully reimplement the entire build, test, and distribution lifecycle of the project.

Given the... severe constraints... which the CMakeLists.txt is operating under, I feel like it's not entirely honest to claim that cmake itself is easier and simpler to write and only takes one file.

"If you prefer, it is possible to remove most functionality from the meson.build and consolidate it into one file, but as it stands the meson.build lets you delete 967 lines of previous build system rather than supporting both Make and CMake".

@eli-schwartz
Copy link
Contributor

Anyway, it's not super urgent for liburing to add meson support, since that PR is languishing and it's now being maintained downstream via mesonbuild/wrapdb#127

Not completely convenient from a maintenance perspective to have it under active third-party maintenance instead of first-party maintenance, but the important thing is that users of liburing who need it as a dependency in their project can use liburing_dep = dependency('liburing') to find a system version, and copy this file https://wrapdb.mesonbuild.com/v2/liburing_2.0-1/liburing.wrap to subprojects/liburing.wrap in order to have automatic dependency lookups fall back on building the subproject. Tests are automatically run by default, even as a subproject, but you can filter that out via meson test arguments.

Personally, I'd advise making the testsuite available even when building it as a dependency of another project, because even a dependency can potentially have issues on your system setup.

@codeinred
Copy link
Author

The CMakeLists.txt doesn't run any of the tests. It registers them with CTest, which runs and processes the output of the test programs. CTest reports which tests passed; which failed; it allows you to specify a timeout (the io-cancel test was failing to exit, both when built with cmake, and when built from the existing Makefile), and it even allows you to run multiple tests in parallel.

This is the output of running cd build && ctest -j 8 --timeout 10, which runs ctest with 8 jobs in parallel, and a timeout of 10 seconds. The output has been shortened to only show the last few tests, and the summary at the end.

105/112 Test   #8: test-a4c0b3decb33-test ...........   Passed    5.91 sec
106/112 Test #104: test-submit-reuse ................   Passed    0.60 sec
107/112 Test  #97: test-sqpoll-cancel-hang ..........   Passed    1.00 sec
108/112 Test #112: test-wakeup-hang .................   Passed    2.00 sec
109/112 Test #110: test-timeout .....................***Failed    4.00 sec
110/112 Test  #45: test-io-cancel ...................***Timeout  10.01 sec
111/112 Test  #57: test-multicqes_drain .............***Timeout  10.02 sec
112/112 Test  #91: test-splice ......................***Timeout  10.02 sec

88% tests passed, 13 tests failed out of 112

Total Test time (real) =  14.24 sec

The following tests FAILED:
	  1 - test-232c93d07b74-test (Subprocess aborted)
	 11 - test-accept-test (Subprocess aborted)
	 27 - test-double-poll-crash (SEGFAULT)
	 39 - test-file-verify (Failed)
	 45 - test-io-cancel (Timeout)
	 57 - test-multicqes_drain (Timeout)
	 74 - test-read-write (Failed)
	 91 - test-splice (Timeout)
	 93 - test-sq-poll-dup (Failed)
	 94 - test-sq-poll-kthread (Failed)
	 95 - test-sq-poll-share (Failed)
	103 - test-submit-link-fail (Failed)
	110 - test-timeout (Failed)
Errors while running CTest
Output from these tests are in: /home/alecto/cowork/liburing/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

In this case, CMake isn't intended to replace the install functionality provided by the Makefile, but rather to allow out-of-source builds; to enable the use of CTest; to enable generation of compile_commands.json; and to allow the library to be fetched remotely so that it can automatically be downloaded as a dependency of other projects.

If you would like, I could add an option titled something like LIBURING_ALWAYS_BUILD_TESTS that can be passed to cmake so that tests are built even when it's a dependency of another project. Or we could make building tests the default, and have a LIBURING_DONT_BUILD_TESTS option, for when the user has already verified that it works on their system.

@codeinred codeinred closed this Sep 24, 2021
@codeinred codeinred reopened this Sep 24, 2021
@codeinred
Copy link
Author

codeinred commented Sep 24, 2021

(I'm extremely sorry - I closed the pull request as a mistake.)

@louvian
Copy link

louvian commented Sep 24, 2021

This is the output of running cd build && ctest -j 8 --timeout 10, which runs ctest with 8 jobs in parallel, and a timeout of 10 seconds. The output has been shortened to only show the last few tests, and the summary at the end.

I don't think these tests should be run in parallel. Building them in parallel does make sense, but not for running them.

Recall that the tests of liburing are mostly kernel test. Running them in parallel may confuse the reader when examining the kernel log (dmesg). Consider the output from this test script

# Log start of the test
if [ "$DO_KMSG" -eq 1 ]; then
local dmesg_marker="Running test $test_string:"
echo "$dmesg_marker" > /dev/kmsg
else
local dmesg_marker=""
fi

The output of failing test must be shown right after:

"Running test $test_string:"

If you run them in parallel, we will have races in kernel log (printing the "running" and problem from warn, bug, oops, etc.).

@codeinred
Copy link
Author

Tests won't be run in parallel by default; I ran them in parallel in my example. If you run ctest --timeout 10 in the build directory it will run the tests sequentially with a timeout of 10 seconds.

ammarfaizi2 added a commit to ammarfaizi2/liburing that referenced this pull request Oct 1, 2021
…odeinred-master

* 'master' of https://github.com/codeinred/liburing:
  Added support for CMake build system

Link: axboe#438
Signed-off-by: Ammar Faizi <[email protected]>
@stalkerg
Copy link

whereas an analogous pull request to support Meson required 7 files and 384 lines of code.

Honestly, it's not because of the Meson, and it's just this implementation. I can do everything in one file same as CMake.
PS I know very well CMake and Meson. Meson is better if it's enough.

@eli-schwartz
Copy link
Contributor

The CMakeLists.txt doesn't run any of the tests. It registers them with CTest, which runs and processes the output of the test programs.

That's a bit like saying a Makefile doesn't compile any executables, it just registers executables with the Make program. It completely misses the point of my statement, which is that the CMakeLists.txt contains instructions such that using this PR to run the tests, will run a bunch of ELF binaries even though those are not the tests. The tests are a bunch of shell scripts, the compiled executables are merely resource programs which the tests run.

You're missing part of the test, because in fact CTest does NOT process the output, only the return code.

Running programs that don't sufficiently test anything, because they are not parsed by the shell test harness, seems... insufficient.

(As it happens, the test harness doesn't look at the command output, but rather looks at dmesg, assuming it has permission to. This is not something cmake can do, even with PASS_REGULAR_EXPRESSION.)

CTest reports which tests passed; which failed; it allows you to specify a timeout (the io-cancel test was failing to exit, both when built with cmake, and when built from the existing Makefile), and it even allows you to run multiple tests in parallel.

This is the output of running cd build && ctest -j 8 --timeout 10, which runs ctest with 8 jobs in parallel, and a timeout of 10 seconds. The output has been shortened to only show the last few tests, and the summary at the end.

Thank you for pointing out that ctest does the same basic thing that meson test and autotools make check do as well. As it happens, I know all this already.

Also, none of this has anything whatsoever to do with my comment, so I'm not sure why you're trying to tell me about it.

Allow me to reiterate: your PR does not correctly test the project, because it doesn't attempt to operate the existing test machinery (when running the tests as root). This is a shortcoming in your PR, which has nothing to do with ctest.

In this case, CMake isn't intended to replace the install functionality provided by the Makefile

This makes it much less useful if it's not suitable for general use... why refrain from fully implementing the build? A fully operational build is more likely to be used by people, and thus be kept around and avoid bitrot...

but rather to allow out-of-source builds; to enable the use of CTest; to enable generation of compile_commands.json; and to allow the library to be fetched remotely so that it can automatically be downloaded as a dependency of other projects.

These are compelling arguments in favor of the benefits of using some form of widely used build system, but they are also not really cmake-specific points. All these are features of meson as well, and half of them are supported by autotools as well. (I am interpreting "enable CTest" as "enable a standardized testing framework".)

It's true that none of these are supported by highly customized build systems.

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

Successfully merging this pull request may close these issues.

5 participants