-
Notifications
You must be signed in to change notification settings - Fork 411
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
base: master
Are you sure you want to change the base?
Conversation
Hi @codeinred,
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]>
Hi @ammarfaizi2, thank you so much for letting me know! I redid it as a single commit with the appropriate formatting! |
There is also #297 by the way.
I'm not sure what this sentence means? cmake only requires one file to support out of source builds? |
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. |
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". |
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 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. |
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
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 If you would like, I could add an option titled something like |
(I'm extremely sorry - I closed the pull request as a mistake.) |
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 Lines 83 to 89 in ca9252f
The output of failing test must be shown right after:
If you run them in parallel, we will have races in kernel log (printing the "running" and problem from warn, bug, oops, etc.). |
Tests won't be run in parallel by default; I ran them in parallel in my example. If you run |
…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]>
Honestly, it's not because of the Meson, and it's just this implementation. I can do everything in one file same as CMake. |
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.)
Thank you for pointing out that ctest does the same basic thing that 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.
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...
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. |
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:
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:
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:
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)test/
. All .c files in thetest/
directory are added as tests, except forhelper.c
.Please let me know if you have any questions or feedback, and I'll do my best to address them!