Add CMake as a supported build system #82
Conversation
There was a problem hiding this comment.
I don't have a problem with including CMake support, I understand that a lot of people either have not choice or prefer CMake, and unfortunately the duplication is unavoidable. I have a few comments on things, and I opened #83 to try to ease a little bit of the duplication.
I would note that I have gone out of my way to make sure that the Meson works with Muon, which is a pure C99 implementation of Meson, so there is another option for people who are concerned about Python.
51b3704 to
7fe10d1
Compare
|
@dcbaker Thanks a lot for your really helpful reviews! I have fixed some of them and will get back to the others soon |
31e704a to
9cf4c66
Compare
|
@dcbaker I'm not quite sure I have understood this part correctly. The way I'm reading it, it's checking for the existence of I have made the CMake version check for |
|
@bretbrownjr Would appreciate review for some CMake best practices :) |
|
@lunacd Meson's In this case we're looking for |
|
Oh I see. Thanks for explaining how our current meson setup works! I checked docs all the way back to gcc 4 and clang 4. They both support |
|
Since clang also has had support for a long time, I think that's a fine solution. |
There was a problem hiding this comment.
There are some small things to clean up, but I like this PR. @dcbaker's support for muon is appreciated, but the build environment I'm most interested in doesn't have muon in the base system image and it would be a very nontrivial dependency to add at this time.
Aside: We don't have the analogous ability to download and build dependencies from source as needed with this PR. That's probably fine. We can add that later if we miss it. If someone wants a hint on how that might be possible, check out the Dependency Providers feature, which I'm aware of but haven't had reason to use personally.
src/CMakeLists.txt
Outdated
| find_package(tl-expected 1.0 REQUIRED) | ||
| target_link_libraries(libcps PRIVATE tl::expected) | ||
|
|
||
| find_package(fmt 8 REQUIRED) | ||
| target_link_libraries(libcps PRIVATE fmt::fmt) | ||
|
|
||
| find_package(jsoncpp 1.9 REQUIRED) | ||
| target_link_libraries(libcps PRIVATE JsonCpp::JsonCpp) | ||
|
|
||
| find_package(CLI11 2.1 REQUIRED) | ||
| target_link_libraries(cps-config PRIVATE CLI11::CLI11) |
There was a problem hiding this comment.
| find_package(tl-expected 1.0 REQUIRED) | |
| target_link_libraries(libcps PRIVATE tl::expected) | |
| find_package(fmt 8 REQUIRED) | |
| target_link_libraries(libcps PRIVATE fmt::fmt) | |
| find_package(jsoncpp 1.9 REQUIRED) | |
| target_link_libraries(libcps PRIVATE JsonCpp::JsonCpp) | |
| find_package(CLI11 2.1 REQUIRED) | |
| target_link_libraries(cps-config PRIVATE CLI11::CLI11) | |
| find_package(tl-expected REQUIRED) | |
| target_link_libraries(libcps PRIVATE tl::expected) | |
| find_package(fmt REQUIRED) | |
| target_link_libraries(libcps PRIVATE fmt::fmt) | |
| find_package(jsoncpp REQUIRED) | |
| target_link_libraries(libcps PRIVATE JsonCpp::JsonCpp) | |
| find_package(CLI11 REQUIRED) | |
| target_link_libraries(cps-config PRIVATE CLI11::CLI11) |
Unless we have a specific problem we're trying to work around, it's best to not put specific version information in a build configuration. That info really goes in dependency management logic, for instance in the ubuntu build environment setup logic.
There was a problem hiding this comment.
I would also move the add_executable down to line 34 or so. It's good to keep all the declaration and configuration for a target all together. It's good to have all of the context for how a target is configured all on the same "page" of code, or as close to that as you can get. Just for clarity reasons.
There was a problem hiding this comment.
Those are the minimum supported versions we use. Meson does have those wired, and I would rather shift left, ie, cmake/meson should fail to configure if you try to use fmt 7 rather than failing at build time, or worse, compiling and then failing in some weird way at runtime.
With my packager hat on, this kind of information helps downstreams too, because ubuntu (for example) can use this information to help decide if they want to take an update, or if the versions they're putting together actually work, or whether they might want to patch cps-config because they want it work with a dependency version they ship we don't support.
There was a problem hiding this comment.
I only care that the cmake and meson does similar things. I'd be happy to either keep version check in both or neither.
There was a problem hiding this comment.
Well, the dependency management system (which could be CMake but specifically in FetchContent logic), is more "left" than the build step, and it's the part of the development process responsible for finding versions of dependencies that match, broadly.
Not overspecifying in the build system makes it easy to perform development activities at that layer. One doesn't have to fiddle with CMake and Meson in order to upgrade or downgrade dependencies, especially as a bulk operation.
There was a problem hiding this comment.
Maybe I'm missing something, but specifying versions as == 1.2.1 is bad practice, 100% agreed. But saying >= 1.2 when you in fact rely on features in 1.2 is good practice, since it helps users building your software to get things configured correctly immediately which lowers useless bug reports, and it documents for vendors what you expect. If a software vendor wants to use 1.1 for some reason (lets say Ubuntu in this case), they have mechanisms in place to carry the patch to Meson or CMake that does that as part of their packaging process. And that patch isn't even the hard part, they also have to patch out whatever 1.2 features you're using.
There was a problem hiding this comment.
If I understand both of you correctly, I think we can agree a minimum version requirement that is not over-specified is beneficial. An over-specified version requirement is not as great. The question at hand then is: are we over-specifying? Does using a lower version of the library actually break anything?
There was a problem hiding this comment.
In the case of fmt I think it does. I had to do work to make fmt < 9 work (which is why my distro was shipping), and so
I'd be shocked if 6 or 7 worked as is. I think CLI11 also had issues with 2.0, which is why we have 2.1 there, but I might be misremembering. I know that the library we were using before CLI11 definitely had issues unless you were using specific versions.
There was a problem hiding this comment.
I believe this remains as the only unresolved thread. @bretbrownjr @dcbaker What do you guys think?
There was a problem hiding this comment.
Let's leave the version qualifiers in for now so we can move ahead with CMake support. I'm still concerned that they're overspecified inside CMakeLists.txt, especially since analogous information will be in the configurations for meson and relevant dependency management systems, but maybe we can follow up on that later.
5eef56c to
fdd37e4
Compare
8f2f8a2 to
b37d262
Compare
I have no idea this exists. Looks interesting so I took a shot at it. See the very last commit of this series. Let me know if I'm using something the wrong way. One thing that confuses me is jsoncpp's cmake target name. Their namespaced target name is, quite surprisingly to me, provided in a separate cmake file. The consequence is that when jsoncpp is downloaded and built from source, cmake does not see the namespaced target. Any idiomatic way to make it available? And what confuses me even further is the two non-namespaced target it provides: |
As far as I know, not especially. One of the things I'm hoping CPS can help with is for projects to have declarative ways (i.e., CPS files!) to explicitly define their interfaces. Barring that, some procedural code detecting that the As to the static versus shared question, looks like |
# Changes Currently, source code copied into docker images and the built binaries are also stored there. This commit extracts that process from docker build into the CI pipeline. # Benefits - Docker image layers are cached. Moving source code and binaries out of docker image reduces unnecessary cache, which are never going to be a cache-hit for CI purposes anyway. - This also adds flexibility to support other build systems other than meson.
Makes sense to me. I made it link against lib or static depending on |
This commit adds CMake as a supported build system alongside meson. For a potentially low-level project like cps-config that many other C++ projects would have a reason to depend on, using CMake instead of meson removes python as a transitive dependency for those C++ projects that choose to leverage cps-config. CMake is also just a very widely used build system for C++. Providing CMake as an option may introduce a more familiar workflow for those that would like to use, contribute to, or package cps-config. This introduces a fair amount of repetition between the cmake and meson GitHub workflow files. Ideally this can be extracted into an action that the two can share. I'll leave figuring out exactly how that works for another day.
Adding CMake support introduces `CMakePresets.json`. This causes clang-format trying to format JSON files, but failing to do so because no styles are specified for JSON files. This commit takes LLVM style as a reasonable default to start with.
When the switch was made from cxxopts to CLI11, this dependency was not fixed. CI has been passing because meson is falling back to wraps. CMake in this project is not set up this way and requires CLI11 to be properly installed in those containers.
This commit enables integration tests in CMake. Because CMake tests expect failure to be communicated via return code rather than TAP, the integration test harness is modified to return 1 when there is at least one failure.
This enables sharing of version between meson and CMake
There was a problem hiding this comment.
I'm ready to move ahead on this, once the CI is working.
@lunacd is this related to this PR or did the CI in the main branch start failing somehow?
Looks like system failures on GitHub Actions side. Retrying |
|
Can't figure out how to on the UI. I guess I'll just force push tonight |
This is another step in reaching feature parity between CMake and meson. When using the `fetch-deps` preset, or when setting `CMAKE_PROJECT_TOP_LEVEL_INCLUDES` to `subprojects/provider.cmake`, the dependencies will be downloaded and built from source.
Currently, CI is failing weirdly because of ubuntu repos returning 404. I suspect that is because the package index layer, `apt-get update`, is a cache-hit but the next layer that installs packages are not. This causes `apt` to work with outdated indices. Further, it's just good container hygiene to keep those in a single layer to make sure package index doesn't end up in a layer of the image.
|
Looks like a docker layer caching problem. I pushed another commit to fix that. This probably doesn't belong here but it wouldn't pass without. So just don't squash the commits. Merge or rebase instead |
Closes #50. Supersedes #78.
Changes
This commit adds CMake as a supported build system alongside meson.
Benefits
For a potentially low-level project like cps-config that many other C++
projects would have a reason to depend on, using CMake instead of meson
removes python as a transitive dependency for those C++ projects that
choose to leverage cps-config.
CMake is also just a very widely used build system for C++. Providing
CMake as an option may introduce a more familiar workflow for those that
would like to use, contribute to, or package cps-config.
Future work
This introduces a fair amount of repetition between the cmake and meson
GitHub workflow files. Ideally this can be extracted into an action that
the two can share. I'll leave figuring out exactly how that works for
another day.
This PR also refactors the CI process a bit to introduce the flexibility necessary for this change.
Perform build outside docker build process
Changes
Currently, source code copied into docker images and the built binaries
are also stored there.
This commit extracts that process from docker build into the CI
pipeline.
Benefits
docker image reduces unnecessary cache, which are never going to be a
cache-hit for CI purposes anyway.
meson.