Closed pfultz2 closed 7 years ago
Thank you, I'll have a look at this soon
I decided to revert the Exports installation for the time being due to the added complexity to the CMakeLists.txt file. Of course, I guess I would normally discuss these changes in the review instead of reverting them, but I decided to do it in this way since this PR (and its commits) basically was 3 PR's/commits in one.
Feel free to open another PR regarding adding Exports installation, however, and we can discuss these changes there. Hopefully one can accomplish this with a few lines in the CMakeLists.txt and without adding an additional cmake file.
There is also the question if cmake installing Exports is needed for such a small library (with only one library file).
I don't think size of the library matters. The point is to provide easy package configuration so when I want to use the library I can just do:
find_package(tiny-process-library)
add_library(foo foo.cpp)
target_link_libraries(foo tiny-process-library)
Otherwise, I would need to do the following:
find_library(TINY_PROCESS_LIBRARY tiny-process-library)
find_path(TINY_PROCESS_INCLUDE process.hpp)
find_package(Threads)
add_library(foo foo.cpp)
target_link_libraries(foo ${TINY_PROCESS_LIBRARY} ${CMAKE_THREAD_LIBS_INIT})
target_include_directories(foo ${TINY_PROCESS_INCLUDE})
This suffers from several problems:
There is boilerplate to write with cmake(and I agree it is painful). This is why I wrote bcm_package
to simplify this, but I assume you don't want to bring in an additional dependency.
Hopefully one can accomplish this with a few lines in the CMakeLists.txt and without adding an additional cmake file.
Alternatively, you could set the export file to 'tiny-process-library-config.cmake` file instead of configuring a file and then including the exports(that is what is done here). However, if you would like to add additional cmake variables or you might use imported targets from dependencies in the future, then the extra file needs to be configured.
Thank you, I'll look into this some more in a week or so and get back to you.
My main worry is the added complexity of making it convenient for users to use the library when using some build management system X. CMake is not the only build management system, Meson for instance, is getting increasingly more popular. And then you also have Conan, and see my final response in #18, where the final solution was to create a non-intrusive Conan package. Maybe this is how the problem will be solved for other build management systems as well in the future?
Now, this project makes use of CMake to compile the examples and tests, and I think it's important that the users can look at the CMakeLists.txt and understand what is going on, so that they may implement the same in the build management system they are using. Supporting all the build management systems out there, with all the extra files added to the project, would make it quite confusing for new users I think, even though we have added a convenient setup for the build management system they are using.
edit: typo
CMake is not the only build management system, Meson for instance, is getting increasingly more popular.
This is not about build systems nor package managers. This is about package configuration. There is only two package configuration systems: cmake's and pkgconfig. Pkgconfig is cross-platform and build-independent. Non-cmake build systems like meson, waf, makefiles, or autotools use pkgconfig. Furthermore, cmake can use pkgconfig as well, however, using cmake's native package configuration is much nicer. I can send a PR with cmake and pkgconfig support, so it can easily support all build systems.
where the final solution was to create a non-intrusive Conan package. Maybe this is how the problem will be solved for other build management systems as well in the future?
This can be a solution for package managers, but not for package configuration. This should always be part of the build since it describes to downstream libraries how to use the library.
Now, this project makes use of CMake to compile the examples and tests, and I think it's important that the users can look at the CMakeLists.txt and understand what is going on, so that they may implement the same in the build management system they are using.
There should be no need to write another build script. First, cmake provides a portable build system with a descriptive toolchain, which means any package manager can bootstrap proper cmake build scripts(this is what cget does). Secondly, with support for pkgconfig the user can install the library with cmake and then easily use it in meson, makefiles, or whatever build system they choose to use.
Thank you for correcting me and for your excellent explanation, I'll have to do some additional reading on this it seems. But from what you say, it sounds like pkgconfig is the way to go here, since it is build independent? At least in the first run maybe? I'm thinking now that cmake isn't necessarily considered the "standard" build management system anymore for C++.
But from what you say, it sounds like pkgconfig is the way to go here, since it is build independent?
It is build independent. Its also nice to have native cmake as well, since the project is built with cmake and windows users would need to install pkgconfig. I can easily send a PR that supports both, especially since the work has already been done for cmake.
I'm thinking now that cmake isn't necessarily considered the "standard" build management system anymore for C++.
I still think cmake is the defacto standard for open-source projects. Plus, many open-source projects are moving away from autotools to cmake as well. There are some projects that use non-cmake build systems, but from my experience they generally suffer problems with portability and finding dependencies, and that is where cmake's maturity does much better.
Ok, but can we put the new code in a cmake directory? Maybe also separate the pkgconfig and cmake specific configuration source into two files. In that way we can keep the current CMakeLists.txt readable for those not experienced with package configuration.
Yea, I can see what I can do to move most of the code to a separate cmake directory, so that the top-level cmake is simpler.
find_package(tiny-process-library)
and then use thetiny-process-library
target.BUILD_TESTING
variable to enable tests as that is the variable used by ctest to enable tests.