Convert Makefiles to cmake
#44
Conversation
|
If you are interested in making the change, please feel free to merge. Passes all the tests on Arch Linux 64-bit. |
|
This is actually quite nice. |
|
Glad that you liked it :) I needed to make |
|
So are there plans for this to be merged? |
|
@imneme, do you think we can merge this PR anytime soon? |
|
@imneme and @lemire, I have spent some time today, and I have done the following:
If the PR gets accepted, @imneme needs to enable Travis builds on her account, and the badge should work properly. Here is the valid badge on my Travis account (enabled on my fork): |
|
Why move |
|
I think I'd echo @talisein here; happy to hear others chime in though. |
|
Given that these are public headers, I would leave them in include. |
|
TL;DR. I have moved back to
@talisein I wanted to separate the tests and sample programs from the actual library. Imagine you have a super-project directory structure as follows: Then, with the following cmake_minimum_required(VERSION 3.8.0)
add_subdirectory(external/pcg/src)
# ^---------- if external/pcg,
# then it becomes a super-build, and the super-project needs to build the tests
# and samples, which is not necessarily needed in the super-project
add_executable(main main.cpp)
target_link_libraries(main PRIVATE pcg::pcg)and #include <iostream>
#include <vector>
using namespace std;
#include "pcg_random.hpp"
int main(int argc, char *argv[]) {
pcg32 gen;
vector<int> x(10);
for (auto &elem : x)
cout << (elem = gen(10)) << '\n';
return 0;
}I can easily use the header files of PCG and build my Moreover, if developers of PCG later on decide to have a C-API for PCG for predefined types and/or generators, then they can easily put the implementation files and the C-API header under, e.g., As for
I could not understand the problem. If we are switching from
@lemire, they are already in |
|
What you are describing sounds reasonable but why does it require moving the include file to the src directory? Are you saying that CMake cannot achieve this subdirectory build without having to build the tests, unless you move the directory? I would expect CMake to support for header-only library... it is the easiest kind of library, after all. (It is 'trivial' library with no work to do.)
This is a header-only library. I would expect users to just pull in the files and then add "include" to their "include path". The only purpose of CMake is to build the tests. End users do not need to use CMake, the could use the build system of their choice. It is very common, in the case of a header-only library, to have the directory structure of the pcg project. I think that @imneme just followed the established practice (that goes back decades). It would be sad and surprising if modern IDEs could not deal with it. All CMake needs to do is adjust the include path. I can look at the problem if you'd like. I am convinced that moving the directory is unnecessary, and if it is not, it indicates that CMake is inflexible and needs to be improved. Let me know, I'm willing to invest a few minutes trying things out. |
Yes, sort of. If you would like to separate testing and building (the samples) from the library (the headers, in this case), different CMake projects (i.e., As it is now, there are 4 CMake projects in this repo: If the user is willing to run the tests and do everything the previous This is what the CI is doing right now on Travis. If the user would like to skip testing and the binaries, then would be enough. However, now I have polluted the
It is supporting it, and my solution works just as well (be it
This is where we depart from each other, I suppose. I believe there are two different use cases. In the first one, I am a privileged user and I am lazy. I install However, in this setup, I always need to remember that This time, CMake installs the necessary configuration files, which contain information about not only the In the second use case, I am a developer who is working on a super-project with a lot of dependencies. Basically, in this latter use case, I do not agree with you on the statement: "The only purpose of CMake is to build the tests." The statement "End users do not need to use CMake, the could use the build system of their choice." is correct, but the problem is that the only build system provided in
I am not arguing at all about this with anyone. That's why I simply reverted to the original directory structure. However, it feels a bit sad for me to pollute
It already does this, in either of the solutions.
Please do help me. I am not sure if I can convey properly what I would like to have from a library, but this post sort of attempts to clarify this. I am not a native speaker in English. |
|
As far as I can see, the new installation process creates the following files (and nothing else)... And that is all. That might be fine, but that's almost entirely the opposite of the current behaviour where "make install" copies the header files and nothing else. |
|
Now let me compare with a well established C++ library that uses CMake: https://github.com/jarro2783/cxxopts
|
|
I picked another well established C++ library that uses CMake.
|
|
|
Oh. And...
|
|
I am also concerned regarding how the tests run. I can't quite grasp the CMake logic, but if I try and modify test-high/expected/check-pcg128_once_insecure.out, which should generate one error, and only one error, I get the following... Yet all I did is the following... |
|
So I suggest that the tests be checked so that we know for sure that they are at least as reliable as the current tests in the project as it stands now. Ideally it should come with some validation. Alternatively, one could simply preserve the current Makefile for testing purposes and just use CMake for end users. |
Thank you for spotting the bug. That bug got introduced with 49dedbf when switching from
I am not here to throw s**t at other libraries or design choices, but from my perspective, First, they are building examples and tests by default. I know that these options can be turned off through the GUI or the command line of CMake, but as I stated above, if I am a user interested only in the library, I cannot use their project file easily. Please try it for yourself: Then, please have the following cmake_minimum_required(VERSION 3.8)
add_subdirectory(external/cxxopts)
add_executable(main main.cpp)
target_link_libraries(main PRIVATE cxxopts)and #include <iostream>
#include "cxxopts.hpp"
int main(int argc, char *argv[]) {
cxxopts::Options options("MyProgram", "One line description of MyProgram");
return 0;
}When you build your project with you will notice that you need to build the examples and tests of the dependency. This problem gets worse when you have different dependencies. Second, they change the global language options and compiler settings, which is affecting the whole project's compilation behavior. If we do not want to install the binaries, we can remove the related part from the project. The other library,
Uhm, I am afraid you have skipped the you will see that (after applying your patch) there will be only 1 failure. you can also use As for the CMake magic, I am only using a CMake function. Instead of defining many cpp files that connect to two base files and define different compiler definitions, I create different targets using CMake that use those files, and I add
Please see above. |
|
Ok, you have me convinced. |
Please don't get me wrong. I tried motivating the design choice of this folder structure in #43. After following the talks such as Effective CMake and friends, I understand that CMake is to be used majorly for defining the dependency graph among parts of a project. My understanding could be wrong. However, I try to follow their advices not to define any "settings" ( I am not here to judge anybody's design choice --- I am not an expert, and I just try my best to follow best practices. That said, would you like me
|
|
I would recommend merging what you have now without any major concern. It looks like great work, even after I challenged you. But let me challenge you some more...
Finally, let me comment that, speaking for myself, I am quite happy that people like you are pushing for better build setups. |
|
This is all shaping up very well, thank you! There’s still a bit of time for refinement (e.g. only installing the headers) and the testing without building issue. (Before I merge this, I’ll be putting out a final formal release with the old build setup, and then make the new build scheme part of a version bump.) |
|
Also, due to the removal of |
|
CMake is a little like configure/automake. You run CMake to generate Makefiles, then you can I agree installing the demos is unexpected. Maybe ala GNOME's 'installed tests' initiative it would make sense as a configuration option, but in that case I think the binaries should be namespaced if put in a global context, e.g. But I believe installing the While testing the branch I noticed there is no 'uninstall' target. Can it be added trivially? Thanks for your work on this @aytekinar |
Converted `Makefile`s to `CMakeLists.txt` to add cross-platform support. Closes #43.
|
Thank you all for your appreciation and kind words. Now let me address your concerns/comments. Directory structure@lemire says:
I have already stated several times before that I fully understand that people can have different mental models. Moreover, we are on the same page when it comes to having public header files under For me, having At the end of the day, we mention on the landing page ( Later, we continue on the landing page for the patient users as well as the package maintainers. We state that
If a user chooses to use CMake (now, I am ommitting the testing, which I will come to later), depending on which entry point they use, they will install different build artifacts: Later, in their projects, they will only use the following cmake_minimum_required(VERSION 3.8.0)
project(example)
find_package(pcg) # if installed properly
# add_subdirectory(external/pcg-cpp/lib) # if building a super-project
# add_subdirectory(external/pcg-cpp/src) # ditto
add_executable(example1 example1.cpp)
target_link_libraries(example1 PRIVATE pcg::pcg)They don't care about any usage requirements of Building and Testing@lemire says:
@imneme says:
With CMake, the whole process could be defined in 5 steps:
There is an open issue for over 10 years that is related to your remarks on "testing without building." I do not know if CMake developers would be investing time on this, but I do not feel the need for it. Provided that I know that the 5 steps listed above are needed to properly build, test, install and package my (or any) software, I am fine. We are talking about writing (or forgetting to write) Coming to what is happening behind the scenes in Another thing worth mentioning is that To sum up, if you are not following the procedure (i.e., build before test), then the result you get should be undefined. This makes sense if we make the analogy to not issuing Installing and PackagingNow let's investigate what happens when installing the artifacts and/or packaging Unfortunately, this is neither safe (most Linux distributions come with package managers for a reason) nor desired. For instance, I am an Arch Linux user, and whenever I need a library, I first search the official Arch repositories, then Arch User Repository (AUR), and finally (if I am hopeless) I create my own When I push this With the above in mind, let me address your final comments: @lemire says:
@imneme says:
@talisein says:
Well, hopefully, I have convinced you that it is not our responsibility, as library writers, to think about all the different variants of installing and packaging our code. However, it is our responsibility to (not) make package managers cry. We provide all the things we can, and package maintainers choose to install what parts of the library to install, and where to install them. With 60a2598, I believe that we address @talisein's concerns regarding namespacing. If GNOME-minded people choose to install the sample files, then let them install. If they do not want, there is an easy solution (see two Another thing to note about the above packaging process is that there are three steps: which installs the (build) dependencies, cleans the build process, removes the (build) dependencies after successful packaging, and finally installs the packaged library. If a user chooses to append Other Issues@talisein says:
I have quadruple-checked my repo, but I could not reproduce the issue. Could you please let me know where you had the problem of having an empty directory? @talisein says:
Yes, it can be added trivially. Nevertheless, I am reluctant to do so. CMake provides out-of-source builds, anyways. The simplest way to @imneme says:
It does. As @talisein has already pointed out, if your system uses should just work fine. |
|
I'm not likely to approve this line: The whole practice of piping the dynamic output of a website directly into a shell is not okay with me. I know lots of folks do it, but lots of folks text and drive. |
|
Now, it is on a different branch. I am simply following the documentation, and it shows the coverage. |
|
Curious as to the status of this pull request. I use PCG, but currently have to import it into my project as a submodule. The discussion was interesting. I use the Pitchfork Layout in my project, and PCG goes into |
| cmake_minimum_required(VERSION 3.8.0) | ||
|
|
||
| project(pcg VERSION 0.98.1 LANGUAGES CXX) |
There was a problem hiding this comment.
Only the root CMakeLists.txt needs this. I also think having a separate CMakeLists.txt for the include directory for a header only library to be a bit overkill.
There was a problem hiding this comment.
Basically, include is the "root" directory for the project. If you read the previous comments in the thread, my suggestion was to have include under yet another directory such as src. We can discuss the name, perhaps have include under pcg, etc., but the most important thing is to have a separate project directory.
Having more than 1 CMakeLists.txt files does not hurt anyone. README.md can easily document what you need to do. If you want to install the headers, run
mkdir build
cd build
cmake ../include # or, cmake ../pcg, cmake ../src, whatever that folder would be
cmake --build . && cmake --build . --target installIf you want to also install sample files, do the super build:
mkdir build
cd build
cmake ../
cmake --build && cmake --build . --target installAt the end of the day, you simplify the logic.
There was a problem hiding this comment.
I guess I just find it weird since I've not seen any header only library separate them like that.
We can discuss the name, perhaps have include under pcg, etc.
This should probably be raised as an issue and resolved before this PR is merged.
| if (NOT TARGET pcg::pcg) | ||
| find_package(pcg CONFIG REQUIRED) | ||
| endif() |
There was a problem hiding this comment.
This is needed because all the tests are linking against pcg::pcg. pcg::pcg is a target in CMake.
There are two possibilities:
- You have done a super-build;
pcg::pcgexists as a target in CMake's cache. This is because theCMakeLists.txtfile in the root of the source tree parses the directories accordingly, - You have not done a super-build; in this case, you should have installed
pcgunder one of the default directories CMake searches the libraries for. That's why Line 8 is explicitly asking for the CMake-installedpcgpackage (i.e.,find_package(pcg CONFIG REQUIRED)).
There was a problem hiding this comment.
Yeah I sort of realised shortly after that you may have been going for being able to build the tests standalone as well as from the root. I've not seen any library do that before and unsure of the practicallity of that 🤷♂ .
There was a problem hiding this comment.
Well... As we discussed back then, it seemed to be a nice approach (credits go to Daniel Pfeifer, by the way). Using CMake as a dependency manager (at least, in the context of the library itself) eases a lot of pain in advance. As a consumer of pcg (or, any other library) I should not be forced to remember any "options" to turn on/off testing and other features.
There was a problem hiding this comment.
Now if only we could get everyone to do that... 😅
There was a problem hiding this comment.
So I would imagine most people would use the root CMakeLists for all use cases and then wonder how to not build tests by opening it up and seeing no option for it and then finally go look at the docs.
Fine. But what is your proposal? Under this PR, we have discussed these (or similar) issues. Who is our target audience?
- The impatient will just go find the
includedirectory, use the 3 files. Done. - The developers (hopefully) check the "Installation" section under the landing page (
README.md) and see that the root has the super-build functionality. - Users relying on their package managers will simply issue their package manager's installation subcommand.
First category above is not the target audience, I believe. Regardless of what we do here, they will go fetch the files. Third group will not have any problems at all because all they need to write in their CMake projects is find_package(pcg CONFIG REQUIRED). The second group of people are (mostly, I believe) are the most "nerd" of them all. If they know how to track their dependencies in their source trees using git submodule, for instance, they should be reading the Installation section in the README, which should take 1 minute.
That said, what do we want? If you say "keep the root of the source tree as the CMake project for pcg and make samples and tests separate CMake projects that writes find_package(pcg CONFIG REQUIRED)," I am happy to assist. Then, we will have 3 projects and no options, and I am happy.
If you would like to go with the "options" route, I am sorry. Then, please do however you like. I do not like that route, and I will not work on something I do not like. Similar to the examples you have provided, I can provide you counter-examples that prefer this route (or, talks that suggest against the use of options in CMake projects (i.e., directories under which CMakeLists.txt exists). The problem with options, which I also agree, is that the names are global. If a user has PCG_TESTS variable/option defined in their CMake project, what happens? You are relying on people being rational (or, behaving properly) when you make a decision based on variable names. If we assume they are rational enough, they can as well skim the first couple of lines of the README.
If you definitely need your options for some building/testing/packaging purposes, CMake projects are not the place for it. We have cpack where we can define configuration options and variables for CMake projects. This solution will let you have the best of both worlds, in my opinion.
Please feel free to have the 43-cmake branch of aytekinar/pcg-cpp as a base and introduce your options.
There was a problem hiding this comment.
As a side note, this line and the following two lines (and similar commands in CMake projects) are exactly the ones I am against (or, people giving talks in CppCon or similar venues are against).
CMake projects should define dependencies between files, modules and among different CMake projects. Options and logic around compilers, building, documentation, testing, etc. are not relevant. CMake is not a make tool. It is not a place where we script our logic as in "if it's clang, do this," or "if you want to build docs, do that."
If you follow the talks mentioned in the CMake best practices website you have provided, you will see that people have a cmake folder to support this type of logic. All the three libraries you show as examples define some sort of compiler options. I do not want them. As a library writer, you should not decide which compiler options the consumer has to use. Even worse, you should not use global/project-level variables to set those options. On the other hand, if you would like to have your favorite -Wall -Wpedantic options for the gcc-family, do so in a cmake/gcc.cmake file.
You can check my library (again, I wanted to follow the best practices in these conferences) to have that structure: polo. Is it the best library of them all? Definitely not. But my point is that all the build/test logic goes under cmake/ctest-gcc.cmake, which handles the warning options, coverage, etc. The same file tells ctest to use gcov for test coverage and valgrind to check against leaks. This is my concern. But as a consumer of polo, if you look at the src CMake project, you will only see the dependencies and CXX features required for this library. You can happily use it within your CMake project --- I do not make any intrusive definitions that would change the way you build your project depending on polo.
There was a problem hiding this comment.
Fine. But what is your proposal?
Sorry again for my lack of clarity. I was just explaining my rationale for my original comments and not advocating one way or the other.
keep the root of the source tree as the CMake project for pcg and ...
That's personally how I would have it just on the mere reasoning being that I find include having a CMakeLists a bit weird and unusual.
If one were to move it out of there, would that not make the samples and tests either forced on you (or only when it's the master project) or forced to do them separately? What would you recommend the approach be in that case without using options?
As a side note...
That is interesting and didn't notice that option. I don't know why that's an option that's provided to be honest. It's also interesting because CLIUtils is the owner of both that library and the git book I linked previously 🤔 .
There was a problem hiding this comment.
Sorry again for my lack of clarity. I was just explaining my rationale for my original comments and not advocating one way or the other.
I am not trying to argue with you. I want to know what you or other developers here would like to have. This is the first attempt to provide cmake support to pcg, which also gets rid of the Makefile-based approach. The change is breaking. We do not need to think much about breaking changes, because we are already breaking the current flow of the build/test/package process.
My first proposal was to have
.
├── samples
│ └── CMakeLists.txt
├── src
│ ├── CMakeLists.txt
│ └── include
└── tests
└── CMakeLists.txt
as src is an acronym for "source." Then, this proposal got opposed because it was breaking the mental model of having src and include under some directory to have implementation and header files, respectively. Moreover, because src is used mostly for implementation files and because pcg has only header files, some thought src could give the wrong impression. Even though I do not agree with those opposing ideas, I can understand the mental model they have, and that's fine.
Then, we can discuss and choose to have something like:
.
├── pcg
│ ├── CMakeLists.txt
│ └── include
├── samples
│ └── CMakeLists.txt
└── tests
└── CMakeLists.txt
At least, we have 3 separate CMake projects for 3 different purposes. You can argue that having pcg/include and pcg/CMakeLists.txt directly under the root (i.e., the source tree) is better, and that's also fine. However, in other communities, there seems to be a shift towards such an indirection of locating "source" files of your package under src or similar directories (see, e.g., this for Python projects). The argument around whether such an approach is an overkill for small libraries and such is also going on there. However, it provides flexibility and, more importantly, isolation. Depending on my purpose, I can isolate myself in one of the 3 CMake projects.
That is interesting and didn't notice that option. I don't know why that's an option that's provided to be honest. It's also interesting because
CLIUtilsis the owner of both that library and the git book I linked previously 🤔 .
With all due respect for that author, all I can say is that his approach in CLIUtils is not good. As I mentioned before, I would not want any library writer to dictate compiler flags/options for my CMake project. If I am going to include CLIUtils, for instance, under my CMake project, I would be forced to use his favourite warning flags! The problem is with trying to use CMake in an imperative way. It should be used in a more declarative way, where you simply build a dependency graph between different projects.
As a side note, it's also interesting when Linus Torvalds states
Quite frankly, even if the choice of C were to do nothing but keep the C++ programmers out, that in itself would be a huge reason to use C.
He is a great person, in my opinion, who made Linux and git a reality of our lives. Yet, this does not mean that his opinion about C++ is correct and valid (perhaps, it is, in a general context).
There was a problem hiding this comment.
some thought src could give the wrong impression.
I would argue that as well.
Then, we can discuss and choose to have something like:
I misread and thought you were suggesting the other way around earlier i.e. include/pcg which is a popular convention. Though that would be a bit silly since all headers are prefixed with pcg so that would be superfluous 🤷♂️.
Are there any well known/popular (header-only) libraries that use that structure so I can take a look?
My main line of thinking is that if it's not a big deal (which IMO this is - root being for pcg or include being for pcg) do what people expect.
| if (NOT TARGET pcg::pcg) | ||
| find_package(pcg CONFIG REQUIRED) | ||
| endif() |
| add_subdirectory(sample) | ||
|
|
||
| enable_testing() | ||
| add_subdirectory(test-high) |
There was a problem hiding this comment.
These should be guarded like so
https://cliutils.gitlab.io/modern-cmake/chapters/testing.html
And building the samples should be an option, perhaps an option for installing them too.
There was a problem hiding this comment.
I am not sure if you had time to read through the comments in the pull request thread. It's been almost a year since I made this pull request, and hence, I have forgot most of the code base. Yet, these concerns were discussed back then, and the summary could be found at least under this comment.
Have you tried, for instance
~> git clone https://github.com/aytekinar/pcg-cpp /tmp/pcg-cpp
~> cd /tmp/pcg-cpp
/tmp/pcg-cpp> git checkout 43-cmake
/tmp/pcg-cpp> mkdir build install
/tmp/pcg-cpp> cd build
# only install the header and cmake files
/tmp/pcg-cpp/build> cmake -D CMAKE_INSTALL_PREFIX=../install ../include
/tmp/pcg-cpp/build> cmake --build . && cmake --build . --target install
# super-build, i.e., build sample files as well as tests, and install both header, cmake and sample files
/tmp/pcg-cpp/build> rm -rf *
/tmp/pcg-cpp/build> cmake -D CMAKE_INSTALL_PREFIX=../install ../
/tmp/pcg-cpp/build> cmake --build . && cmake --build . --target install
# if you want to test
/tmp/pcg-cpp/build> ctestBasically, I had followed Daniel Pfeifer's talk, which is also listed in "Effective Modern CMake" link under https://cliutils.gitlab.io/modern-cmake/.
IMHO, the simplest solution with CMake is not to use options at all. This is also proposed/suggested by many different people. If you do not want to install/test anything, simply make an out-of-source build of include. No need to remember any options, etc. If you want to include this project as a submodule, do so and point in your CMake to this include folder (e.g., add_subdirectory(external/pcg-cpp/include)).
|
Sorry to have been slow on this. I haven't forgotten and hope to get to it soon. |
|
Also why is a |
test-high/CMakeLists.txt
Outdated
|
|
||
| # pcg32_unique and pcg64_unique are failing in the old run-tests.sh (/dev/null'ed) | ||
| set_property( | ||
| TEST check-pcg32_unique check-pcg64_unique |
There was a problem hiding this comment.
I have marked pcg32_unique and pcg64_unique as WILL_FAIL. In the original Makefile, the outputs were getting piped to /dev/null anyways.
| add_subdirectory(sample) | ||
|
|
||
| enable_testing() | ||
| add_subdirectory(test-high) |
There was a problem hiding this comment.
I am not sure if you had time to read through the comments in the pull request thread. It's been almost a year since I made this pull request, and hence, I have forgot most of the code base. Yet, these concerns were discussed back then, and the summary could be found at least under this comment.
Have you tried, for instance
~> git clone https://github.com/aytekinar/pcg-cpp /tmp/pcg-cpp
~> cd /tmp/pcg-cpp
/tmp/pcg-cpp> git checkout 43-cmake
/tmp/pcg-cpp> mkdir build install
/tmp/pcg-cpp> cd build
# only install the header and cmake files
/tmp/pcg-cpp/build> cmake -D CMAKE_INSTALL_PREFIX=../install ../include
/tmp/pcg-cpp/build> cmake --build . && cmake --build . --target install
# super-build, i.e., build sample files as well as tests, and install both header, cmake and sample files
/tmp/pcg-cpp/build> rm -rf *
/tmp/pcg-cpp/build> cmake -D CMAKE_INSTALL_PREFIX=../install ../
/tmp/pcg-cpp/build> cmake --build . && cmake --build . --target install
# if you want to test
/tmp/pcg-cpp/build> ctestBasically, I had followed Daniel Pfeifer's talk, which is also listed in "Effective Modern CMake" link under https://cliutils.gitlab.io/modern-cmake/.
IMHO, the simplest solution with CMake is not to use options at all. This is also proposed/suggested by many different people. If you do not want to install/test anything, simply make an out-of-source build of include. No need to remember any options, etc. If you want to include this project as a submodule, do so and point in your CMake to this include folder (e.g., add_subdirectory(external/pcg-cpp/include)).
| cmake_minimum_required(VERSION 3.8.0) | ||
|
|
||
| project(pcg VERSION 0.98.1 LANGUAGES CXX) |
There was a problem hiding this comment.
Basically, include is the "root" directory for the project. If you read the previous comments in the thread, my suggestion was to have include under yet another directory such as src. We can discuss the name, perhaps have include under pcg, etc., but the most important thing is to have a separate project directory.
Having more than 1 CMakeLists.txt files does not hurt anyone. README.md can easily document what you need to do. If you want to install the headers, run
mkdir build
cd build
cmake ../include # or, cmake ../pcg, cmake ../src, whatever that folder would be
cmake --build . && cmake --build . --target installIf you want to also install sample files, do the super build:
mkdir build
cd build
cmake ../
cmake --build && cmake --build . --target installAt the end of the day, you simplify the logic.
| if (NOT TARGET pcg::pcg) | ||
| find_package(pcg CONFIG REQUIRED) | ||
| endif() |
There was a problem hiding this comment.
This is needed because all the tests are linking against pcg::pcg. pcg::pcg is a target in CMake.
There are two possibilities:
- You have done a super-build;
pcg::pcgexists as a target in CMake's cache. This is because theCMakeLists.txtfile in the root of the source tree parses the directories accordingly, - You have not done a super-build; in this case, you should have installed
pcgunder one of the default directories CMake searches the libraries for. That's why Line 8 is explicitly asking for the CMake-installedpcgpackage (i.e.,find_package(pcg CONFIG REQUIRED)).
| if (NOT TARGET pcg::pcg) | ||
| find_package(pcg CONFIG REQUIRED) | ||
| endif() |
I cannot understand this question. Could you please elaborate on this? Have you checked what's inside that
If you are talking about the |
I guess "doesn't have any imported targets" would be more correct. This should achieve the same thing shouldn't it? install(TARGETS pcg EXPORT pcg-config)
install(EXPORT pcg-config
NAMESPACE pcg::
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcg
)
write_basic_package_version_file(pcg-config-version.cmake
COMPATIBILITY SameMajorVersion
)
install(
FILES
${CMAKE_CURRENT_BINARY_DIR}/pcg-config-version.cmake
DESTINATION
${CMAKE_INSTALL_LIBDIR}/cmake/pcg
) |
|
Yes. That solution gets rid of I do not understand, this time, what you mean by '... "doesn't have any imported targets" ...', but yes, the end result creates (as before) the imported target |
By that I mean https://github.com/daniele77/cli/blob/master/cliConfig.cmake.in |
I still don't agree with your argument regarding the use of the In the end, the output of that config file you have proposed will be exactly the same as the one generated by According to the documentation
It is the |
|
I'm saying there's no need for the install(TARGETS pcg EXPORT pcg-config)
install(EXPORT pcg-config
NAMESPACE pcg::
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcg
)
That was an example of where the |
|
Are there any outstanding issues blocking this from being merged? |
|
Would love to see this merged. Is this repository just inactive, or are there any obstacles in this particular PR that one could help with? |
|
I will ping Melissa by email. |
|
I don't think this pull request is ready to merge as is. It installs the PCG sample programs in a To get merged, that stuff needs to go. But even there, in its present form, I'd put it on a |
|
After many fruitful (albeit sporadic) discussions over the course of (roughly) four years, I have failed to communicate the benefit (e.g., modularity, toolchain agnosticism, integrability (as a dependency in super projects), flexibility and package maintainer friendliness) of using CMake even for header-only C++ libraries. Since the last (roughly) four years, I have learnt the following lessons:
As for the two (personal) learnings above, I suggest the community open a fresh issue (should they feel a need for this feature) and discuss/set the definition of "ready/done" and the acceptance criteria before moving forward with the implementation/PR. As for the last learning above, I feel/believe that whenever some feature is left as "an option," we risk the feature getting stale and/or not updated with the same pace as that of the rest in the project, and hence, we risk having (sporadic) bugs in the functionality. That said, I would like to thank everybody under this thread for all the fruitful discussions we have had in the last four years. I would also like to apologize to everybody for any confusion (and time waste, for that matter) I might have caused. I am dropping my PR and closing the issue. |
TBH: Personally, I don't need anything more than a minimal CML like this: https://github.com/MBalszun/pcg-cpp/blob/mba_cmake/CMakeLists.txt without any install support (Running the tests should of course be supported). I don't reallyunderstand why people are so afraid to add CMake to header only libraries. The point for me as a user is that I don't have to worry what kind of library my dependency is. Especially during initial testing I just want ot do a @aytekinar : One thing that has worked for me in the past is to first start with a minimal useful CML (usually without install support, and sometimes not even full test coverage) that even non-cmake users can easily understand. If that gets accepted, one can grow from there and if not, not a lot of effort has been "wasted". |
Agreed, and hence, my two (personal) learnings I have mentioned above. That was a mistake on my end. Please also note that the scope of "minimal (useful)" CML (or any other feature, for that matter) does depend on the "definition of ready/done." Later, we will/might also need the acceptance criteria from the project manager/maintainer to know exactly what it takes to get the PR/feature through. With these learnings in place (as well as ideas on what would be needed at the bare minimum after all the discussions in the thread), one can start from scratch and follow the agile methodology you have proposed, @MBalszun. Unfortunately, I don't have any bandwidth anymore --- I have noticed that my topic branch was lagging some 20 commits behind from the |
|
@imneme : Btw.: Are you still monitoring this repository? |
9 participants
Converted
Makefiles toCMakeLists.txtto add cross-platform support.Closes #43.