Convert `Makefile`s to `cmake`

aytekinar commented 6 years ago

Converted Makefiles to CMakeLists.txt to add cross-platform support.

Closes #43.

aytekinar commented 6 years ago

If you are interested in making the change, please feel free to merge.

Passes all the tests on Arch Linux 64-bit.

lemire commented 6 years ago

This is actually quite nice.

aytekinar commented 6 years ago

Glad that you liked it :) I needed to make pcg-cpp a relocatable library for my project. Then, I wanted to share it with you. Let me know if you need some more change.

vastsoun commented 6 years ago

So are there plans for this to be merged?

aytekinar commented 6 years ago

@imneme, do you think we can merge this PR anytime soon?

aytekinar commented 5 years ago

@imneme and @lemire, I have spent some time today, and I have done the following:

[x] restructured the source directory to decouple src/include from test-high (tests) and sample (samples). This way, users can simply add src as a subdirectory, and use the pcg::pcg CMake target in their projects directly to benefit from the generators. Separating the test and sample directories from the source directory helps build dependent projects much more easily and faster (previously, all the dependent packages needed to build the tests when building super projects),
[x] added Travis CI support for Linux, OSX and Windows. Now, the tests are failing in Windows. Moreover, we can observe the warnings mentioned in #38,
[x] added badges for the build status and the dual license.

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):

talisein commented 5 years ago

Why move include to src/include? It doesn't match common practice, and anyone that has a currently working build will get broken if they update.

imneme commented 5 years ago

I think I'd echo @talisein here; happy to hear others chime in though.

lemire commented 5 years ago

Given that these are public headers, I would leave them in include.

aytekinar commented 5 years ago

TL;DR. I have moved back to include (no nesting under src), but here is why I had chosen to go for a nested structure (well, src could have been lib or something else):

Why move include to src/include? It doesn't match common practice, ...

@talisein I wanted to separate the tests and sample programs from the actual library. Imagine you have a super-project directory structure as follows:

.
├── CMakeLists.txt
├── external
│   └── pcg
│       ├── CMakeLists.txt
│       ├── CONTRIBUTING.md
│       ├── LICENSE-APACHE.txt
│       ├── LICENSE-MIT.txt
│       ├── LICENSE.spdx
│       ├── Makefile
│       ├── README.md
│       ├── sample
│       ├── src
│       └── test-high
└── main.cpp

Then, with the following CMakeLists.txt file

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 main.cpp:

#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 main executable (without needing to build the tests, nor the samples). Of course, I could (and I have already done it, anyways) keep include as is and put CMake-related files inside include, but then the lazy users need to copy the directory and remove the two unnecessary files from their system path.

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., src/api (and, inside src, the folders will only have C++-related files, but not any CMake-related files). You can observe this type of structuring in some other libraries, too.

As for

and anyone that has a currently working build will get broken if they update.

I could not understand the problem. If we are switching from Makefile-based build and installation system to a CMake-based system (and, I do not know if this will at all happen), then currently working builds will anyways get broken.

Given that these are public headers, I would leave them in include.

@lemire, they are already in include, but just one level nested under src. Generally, modern IDEs anyways search recursively for include inside the project (for auto completion). As for CMake, since the project is (as it stands out now) relocatable, this shouldn't have been a problem, either.

lemire commented 5 years ago

@aytekinar

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.)

I could not understand the problem. If we are switching from Makefile-based build and installation system to a CMake-based system (and, I do not know if this will at all happen), then currently working builds will anyways get broken.

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.

aytekinar commented 5 years ago

@lemire

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?

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., CMakeLists.txt files) should be defined accordingly.

As it is now, there are 4 CMake projects in this repo: pcg for the library (under include), pcg-tests for the tests (under test-high), pcg-samples for building the samples (under sample), and finally, pcg-super (under ., i.e., the root, of the repo) for building the samples, running the unit tests and installing the headers as well as the samples.

If the user is willing to run the tests and do everything the previous make all did (as well as installing the sample files), they should run:

mkdir build
cd build
cmake ..
cmake --build .
cmake --build . --target test
cmake --build . --target install

This is what the CI is doing right now on Travis.

If the user would like to skip testing and the binaries, then

cmake ../include # instead of `cmake ..` above

would be enough. However, now I have polluted the include directory with two CMake-related files (i.e., the project file and the configuration file). If you are fine with putting include under some directory (say, e.g., src or lib), then the CMake project will go there, and we will not have a polluted include 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.)

It is supporting it, and my solution works just as well (be it include or src/include, with the former having the problem of having two CMake-related files inside, and the latter being the "problematic" solution).

This is a header-only library. I would expect users to just pull in the files and then add "include" to their "include path".

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 pcg as follows

git clone https://github.com/imneme/pcg-cpp /tmp/pcg-cpp
sudo cp --verbose --interactive /tmp/pcg-cpp/include/*.hpp /usr/include/
# (or, something similar with `install` with the correct file permissions).

However, in this setup, I always need to remember that pcg requires at least C++11. To solve this problem, I use CMake, and I do

git clone https://github.com/imneme/pcg-cpp /tmp/pcg-cpp
mkdir /tmp/pcg-cpp/build
cd /tmp/pcg-cpp/build
cmake ../include
sudo cmake --build . --target install

This time, CMake installs the necessary configuration files, which contain information about not only the include path but also the C++11 requirement of pcg.

In the second use case, I am a developer who is working on a super-project with a lot of dependencies. pcg is one of them, and I want to simply use it (without needing to install the library). First, I create my super-project directory by cloning my dependencies under, say, external (see my previous post for an MWE). I use some editor that natively supports CMake projects. Then, I simply recurse into my dependencies' library directories (e.g., src) to read the corresponding CMake projects and create my targets (in this case pcg::pcg). I write my code, and in the end, I just use target_link_libraries to link against my dependencies (in pcg, this command will only point to the correct include directory and tell CMake that pcg requires at least C++11).

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 pcg is Makefile. Users can use CMake to generate any build system of their choice (while also being able to use their favorite CMake-ready IDE with CMake-friendly projects to automagically handle all the dependencies and requirements).

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.

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 include with some CMake-related files. I can understand that some projects have (maybe unwritten) rules regarding directory structures, but with CMake, this becomes less of an issue. As for modern IDEs, they do support either way. As I already stated in my previous post, modern IDEs such as CLion and VSCode do recurse into all the subdirectories and add includes to their search paths for autocompletion purposes. As such, be it include or xxx/yyy/include, they will find and add all the include subdirectories within the super-project.

All CMake needs to do is adjust the include path.

It already does this, in either of the solutions.

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.

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.

lemire commented 5 years ago

As far as I can see, the new installation process creates the following files (and nothing else)...

/usr/local//lib/cmake/pcg/pcg-config-version.cmake
/usr/local//lib/cmake/pcg/pcg-targets.cmake
/usr/local//lib/cmake/pcg/pcg-config.cmake
/usr/local/bin/cppref-sample
/usr/local/bin/make-partytrick
/usr/local/bin/pcg-demo
/usr/local/bin/spew
/usr/local/bin/use-partytrick

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.

lemire commented 5 years ago

Now let me compare with a well established C++ library that uses CMake:

https://github.com/jarro2783/cxxopts

The header file is in the include directory.
There is nothing else in the include directory.
Installation does not copy tools and benchmarks to /usr/local/bin.
Installation does copy the header file to /usr/local/include/cxxopts.hpp

lemire commented 5 years ago

I picked another well established C++ library that uses CMake. https://github.com/foonathan/type_safe

Again, header files and nothing else are in the include directory.
Installation copies the header files and nothing else...

-- Installing: /usr/local/include/type_safe/visitor.hpp
-- Installing: /usr/local/include/type_safe/integer.hpp
-- Installing: /usr/local/include/type_safe/flag.hpp
....
-- Installing: /usr/local/include/type_safe/types.hpp

lemire commented 5 years ago

Personally, I do not care if there is more fluff in the include directory, but these examples show that it is not necessary.
I think that the current "installation" process should be preserved. That is, installation is merely a matter of copying the header files.

lemire commented 5 years ago

Oh. And...

It shows that there is no need to put the include in the a src subdirectory. The include directory is the common default.

lemire commented 5 years ago

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...

The following tests FAILED:
      1 - check-pcg8_once_insecure (Failed)
      2 - check-pcg8_oneseq_once_insecure (Failed)
      3 - check-pcg16_once_insecure (Failed)
      4 - check-pcg16_oneseq_once_insecure (Failed)
      5 - check-pcg32_c64_fast (Failed)
      6 - check-pcg32_c64_oneseq (Failed)
      7 - check-pcg32_c64 (Failed)
      8 - check-pcg32_c1024_fast (Failed)
      9 - check-pcg32_c1024 (Failed)
     10 - check-pcg32_fast (Failed)
     11 - check-pcg32_k2_fast (Failed)
     12 - check-pcg32_k2 (Failed)
     13 - check-pcg32_k64_fast (Failed)
     14 - check-pcg32_k64_oneseq (Failed)
     15 - check-pcg32_k64 (Failed)
     16 - check-pcg32_k1024_fast (Failed)
     17 - check-pcg32_k1024 (Failed)
     18 - check-pcg32_k16384_fast (Failed)
     19 - check-pcg32_k16384 (Failed)
     20 - check-pcg32_once_insecure (Failed)
     21 - check-pcg32_oneseq_once_insecure (Failed)
     22 - check-pcg32_oneseq (Failed)
     24 - check-pcg32 (Failed)
     25 - check-pcg64_c32_fast (Failed)
     26 - check-pcg64_c32_oneseq (Failed)
     27 - check-pcg64_c32 (Failed)
     28 - check-pcg64_c1024_fast (Failed)
     29 - check-pcg64_c1024 (Failed)
     30 - check-pcg64_fast (Failed)
     31 - check-pcg64_k32_fast (Failed)
     32 - check-pcg64_k32_oneseq (Failed)
     33 - check-pcg64_k32 (Failed)
     34 - check-pcg64_k1024_fast (Failed)
     35 - check-pcg64_k1024 (Failed)
     36 - check-pcg64_once_insecure (Failed)
     37 - check-pcg64_oneseq_once_insecure (Failed)
     38 - check-pcg64_oneseq (Failed)
     40 - check-pcg64 (Failed)
     41 - check-pcg128_once_insecure (Failed)
     42 - check-pcg128_oneseq_once_insecure (Failed)
Errors while running CTest

Yet all I did is the following...

$ git diff
diff --git a/test-high/expected/check-pcg128_once_insecure.out b/test-high/expected/check-pcg128_once_insecure.out
index 0bbf8cd..420dc50 100644
--- a/test-high/expected/check-pcg128_once_insecure.out
+++ b/test-high/expected/check-pcg128_once_insecure.out
@@ -1,4 +1,5 @@
-pcg128_once_insecure:
+
+ pcg128_once_insecure:
       -  result:      128-bit unsigned int
       -  period:      2^128   (* 2^127 streams)
       -  size:        32 bytes

lemire commented 5 years ago

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.

aytekinar commented 5 years ago

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.

Thank you for spotting the bug. That bug got introduced with 49dedbf when switching from src/include back to include. The offending line is here, which has been fixed in f8197c4.

Now let me compared with a well established C++ library that uses CMake:

https://github.com/jarro2783/cxxopts

The header file is in the include directory.

There is nothing else in the include directory.

Installation does not copy tools and benchmarks to /usr/local/bin.

Installation does copy the header file to /usr/local/include/cxxopts.hpp

I am not here to throw s**t at other libraries or design choices, but from my perspective, cxxotps's design choice on CMakeLists.txt is not the best.

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:

mkdir super
cd super
git clone https://github.com/jarro2783/cxxopts external/cxxopts

Then, please have the following super/CMakeLists.txt file:

cmake_minimum_required(VERSION 3.8)

add_subdirectory(external/cxxopts)

add_executable(main main.cpp)
target_link_libraries(main PRIVATE cxxopts)

and super/main.cpp file:

#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

mkdir build
cd build
cmake ..
cmake --build .

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, type_safe, has turned off testing and documentation generation by default, and they do not mess with any global language options or compiler settings, either. Hence, they do not have the previous problems of cxxopts.

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...

Uhm, I am afraid you have skipped the cmake --build . part. If you do

git clone https://github.com/aytekinar/pcg-cpp /tmp/pcg-cpp
cd /tmp/pcg-cpp
mkdir build
cd build
git checkout 43-cmake
cmake ..
cmake --build .
cmake --build . --target test

you will see that (after applying your patch) there will be only 1 failure. you can also use ctest --output-on-failure instead of cmake --build . --target test, and you can see detailed information as to why your test(s) have failed.

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 target_compile_definitions for each of those targets.

So I suggest that the tests be checked so that we now 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.

Please see above.

lemire commented 5 years ago

Ok, you have me convinced.

aytekinar commented 5 years ago

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" (option, set, etc.) or global language options and compiler settings (you can see these advices on their talks, and I admit that they can be wrong, too). These settings should go to different files (either for cpack when packaging the library or in toolchain files when cross compiling for different targets). As a result, we end up having clean CMake projects which only define dependencies (add_{executable,library} and target_xxx_yyy friends for the targets).

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

[ ] to undo the reverting and have src/include as before, and/or,
[ ] remove the installation of binary files (samples).

lemire commented 5 years ago

@aytekinar

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...

I am not sure I understand why running the tests without first build results in false positives. I verify your statement, if you build and test, as you recommend, it works... but I don't think it is a desirable feature that if a user runs the tests without first build, it apparently builds an incorrect version of the setup. Is there some way to at least issue a warning? The current Makefile build is safe in this respect: doing "make test" will build the code if needed. There is no meed to type "make" first. So the CMake build is more fragile, evidently.
I don't think @imneme meant these binary tools to be installed in /usr/local/bin. I think she meant to produce a header-only library where "installing" does not involve building anything. Not that it matters very much, but I think that by switching to CMake, you should preserve the current behaviour as much as possible.
Regarding directories, the same concept should apply: shy away from moving things if you can avoid it. I am not sure what the motivation is for moving include to src. If the "cost" is that you have to create a new file, what is the big deal? Surely, you are not concerned with having one extra file? The convention that I follow is that "include" is the public stuff whereas "src" is the private stuff. So it breaks my mental model for a header-only library to have its headers in src. To be clear, there could be two types of headers... there could be private headers and there could be public ones... I'd put the private headers in src and the public ones in include. The include directory is meant to be used by the users of the library. The src is off-limit. (That's my mental model.)

Finally, let me comment that, speaking for myself, I am quite happy that people like you are pushing for better build setups.

imneme commented 5 years ago

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.)

imneme commented 5 years ago

Also, due to the removal of Makefiles in subdirectories, it no longer builds with make.

talisein commented 5 years ago

CMake is a little like configure/automake. You run CMake to generate Makefiles, then you can make. This assumes Unix Makefiles is the default cmake generator on your system, which it probably is on a Linux box. @aytekinar 's approach of using cmake --build is generator agnostic, but make will still work once the Makefiles have been generated.

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. pcg-codebook. But easiest to just remove them as installation targets.

But I believe installing the .cmake files in addition to the headers is correct behavior, that makes the installed library discoverable to other CMake projects. I'm less certain about /usr/local/include/CMakeFiles/Export/lib/cmake/pcg.. it seems this is just an empty directory. If we are being critical then it would be nice not to create this if its trivial to prevent somehow.

While testing the branch I noticed there is no 'uninstall' target. Can it be added trivially?

Thanks for your work on this @aytekinar

aytekinar commented 5 years ago

Thank you all for your appreciation and kind words. Now let me address your concerns/comments.

Directory structure

@lemire says:

Regarding directories, the same concept should apply: shy away from moving things if you can avoid it. I am not sure what the motivation is for moving include to src. If the "cost" is that you have to create a new file, what is the big deal? Surely, you are not concerned with having one extra file? The convention that I follow is that "include" is the public stuff whereas "src" is the private stuff. So it breaks my mental model for a header-only library to have its headers in src. To be clear, there could be two types of headers... there could be private headers and there could be public ones... I'd put the private headers in src and the public ones in include. The include directory is meant to be used by the users of the library. The src is off-limit. (That's my mental model.)

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 include. Please also note that, above, I have also stated that the name src can be anything. To be precise, we can have the following directory structure, as well, if this better suits your (or other contributors') mental model:

pcg-cpp/
├── CMakeLists.txt
├── lib
│   ├── CMakeLists.txt
│   ├── include
│   └── src
├── LICENSE
├── LICENSE-Apache2
├── README.md
├── samples
│   └── CMakeLists.txt
└── tests
    └── CMakeLists.txt

For me, having lib/{include,src} is no different from src/{include,impl}. However, what is more important is to have more than one CMake project (i.e., CMakeLists.txt file). While being simple (and) stupid, this serves the important need: separate the library (lib) from testing (tests) and sample programs (samples).

At the end of the day, we mention on the landing page (README.md), for the impatient, that all they need is to copy over the contents of lib/include (or, src/include) to their favorite PATH location, and use a proper switch to tell their favorite compiler to use at least C++11 standard. This statement is a single sentence regardless of where include resides w.r.t the root of the repository.

Later, we continue on the landing page for the patient users as well as the package maintainers. We state that pcg uses CMake (>=3.8.0) to manage the whole building, testing and installation steps. We point out to the two different projects:

./CMakeLists.txt: entry point to the super-build of pcg (i.e., building the samples, running the tests, and installing the headers as well as the samples), and,
lib/CMakeLists.txt (or, src/CMakeLists.txt): entry point to the library itself (i.e., where the CMake target pcg is defined, what its usage requirements (cxx_std_11) and/or configurations are). This project only installs the headers (for other projects, this is where we tell our users what our public and private usage requirements (i.e., public and private include directories, compiler definitions, etc.) are).

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:

mkdir build
cd build
cmake .. # generates a build system to install headers, configuration files, and samples
# cmake ../lib # generates a build system to install headers and configuration files
# cmake ../src # ditto
cmake --build . --target install

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 pcg, nor do they care about where the include folder lies in the repo. To be honest, they most often do not know what public and private headers are for. And they should not know it, either.

Building and Testing

@lemire says:

I am not sure I understand why running the tests without first build results in false positives. I verify your statement, if you build and test, as you recommend, it works... but I don't think it is a desirable feature that if a user runs the tests without first build, it apparently builds an incorrect version of the setup. Is there some way to at least issue a warning? The current Makefile build is safe in this respect: doing "make test" will build the code if needed. There is no meed to type "make" first. So the CMake build is more fragile, evidently.

@imneme says:

... the testing without building issue.

With CMake, the whole process could be defined in 5 steps:

Generating a favorite build system, i.e., cmake [options] source-tree,
Building the software, i.e., cmake --build build-tree,
Testing the built software, i.e., ctest [options] (optional),
Installing the artifacts, i.e., cmake --build build-tree --target install (optional), and,
Packaging the installed artifacts properly, i.e., cpack (optional).

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) cmake --build build-tree (Step 2) --- that's literally a one-liner.

Coming to what is happening behind the scenes in pcg... First, because the original tests do not use tools such as gtest, and instead, relies on file comparisons, I needed to rely on compare_files command of CMake to make it cross-platform compatible. However, this comes at a price: I have to use cmake as the test command as opposed to using the actual target name (i.e., ${rng}). This practically means that even if CMake decides to fix the 10-year-standing issue, CMake will have no means to check which targets are required for testing so that it can build them beforehand. There exists a workaround, but I would be hesitant to implement it. There are two reasons for this: 1) usual software packaging pipelines consist of the above 5 steps (again, I will come to that later), and 2) I do not want to outsmart CMake (CMake is best at what it does, and the only thing I need for it to do its job is to keep things simple and not to get on CMake's way).

Another thing worth mentioning is that ctest is much more powerful than make test. For instance, I might want to check if my library builds on my system, and whether a bunch of (but not all) tests run properly. To this end, I can choose to run ctest --tests-regex "check-pcg32*", for instance in pcg, to run only the tests whose name starts accordingly. Basically, forcing some "sane" defaults between make all and make test is not reasonable, in my opinion, and thus, I would not want CMake people to enable this by default.

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 configure before make, for instance. ctest fails all tests, because it cannot find any of the executables it needs in the first place, and this is not a false positive, in my opinion.

Installing and Packaging

Now let's investigate what happens when installing the artifacts and/or packaging pcg (or, any other CMake-friendly library). So far, we have mostly assumed that the user is priveleged (meaning that they have root access), and they are bold enough to follow the README and run cmake --build . --target install with root access to install the library files into the system paths.

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 PKGBUILD files to package the library and install it using the tools recommended by my distribution. A typical PKGBUILD for pcg would hopefully look like this:

pkgname=pcg-cpp
pkgver=0.98.1
pkgrel=1
pkgdesc='PCG Random Number Generation, C++ Edition.'
arch=('any')
url='http://www.pcg-random.org/'
license=('MIT')

# change aytekinar to imneme
source=("${pkgname}-${pkgver}::git+https://github.com/aytekinar/pcg-cpp")
sha256sums=('SKIP')

# use ninja for parallel builds by default
makedepends=('git' 'cmake>=3.8.0' 'ninja')

provides=('pcg-cpp')
conflicts=('pcg-cpp')

build() {
  cd "${srcdir}/${pkgname}-${pkgver}"
  # git checkout -b install v${pkgver}
  git checkout 43-cmake # replace this with the above
  mkdir build
  cd build
  # build optimized binaries, install to a staging area first, i.e., ${pkgdir}/usr
  cmake -D CMAKE_BUILD_TYPE=Release           \
        -D CMAKE_INSTALL_PREFIX=${pkgdir}/usr \
        -G Ninja ..
  cmake --build .
}

check() {
  cd "${srcdir}/${pkgname}-${pkgver}/build"
  ctest --output-on-failure
}

package() {
  cd "${srcdir}/${pkgname}-${pkgver}/build"
  cmake --build . --target install
  rm -rf ${pkgdir}/usr/bin # I dont want to have sample binaries, for instance
}

# # or
# package() {
#   cd "${srcdir}/${pkgname}-${pkgver}/build"
#   rm -rf *
#   cmake -D CMAKE_INSTALL_PREFIX=${pkgdir}/usr ../include
#   cmake --build . --target install
# }

When I push this PKGBUILD file to AUR, for instance, all Arch Linux users would be able to install pcg properly, without even knowing what's going behind the scenes. Moreover, pacman will report errors if pcg tries to install build artifacts that collide with some other library's artifacts.

With the above in mind, let me address your final comments:

@lemire says:

I don't think @imneme meant these binary tools to be installed in /usr/local/bin. I think she meant to produce a header-only library where "installing" does not involve building anything. Not that it matters very much, but I think that by switching to CMake, you should preserve the current behaviour as much as possible.

@imneme says:

There’s still a bit of time for refinement (e.g. only installing the headers) ...

@talisein says:

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. pcg-codebook. But easiest to just remove them as installation targets.

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 60a2598cd, 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 package() variants in my PKGBUILD above). package() for Arch PKGBUILDs is also the place where you change directory names (e.g., use lib for both lib32 and lib64) and do other stuff such as adding license files or defining symbolic links, etc.

Another thing to note about the above packaging process is that there are three steps: build(), check(), and package(), which are very well suited for cmake --build build-dir, ctest, and cmake --build build-dir --target install, respectively. Normally, Arch Linux users issue the follownig (if they are not using another tool):

makepkg --syncdeps --clean --cleanbuild --rmdeps --install

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 --nocheck to the above command, then check() section will be dropped. Had I relied on CMake building the artifacts that the tests need (rather than explicitly typing cmake --build . under build()), I could have had problems in package().

Other Issues

@talisein says:

I'm less certain about /usr/local/include/CMakeFiles/Export/lib/cmake/pcg.. it seems this is just an empty directory. If we are being critical then it would be nice not to create this if its trivial to prevent somehow.

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:

While testing the branch I noticed there is no 'uninstall' target. Can it be added trivially?

Yes, it can be added trivially. Nevertheless, I am reluctant to do so. CMake provides out-of-source builds, anyways. The simplest way to make uninstall is to do rm -rf * inside, say, build and start from scratch.

@imneme says:

Also, due to the removal of Makefiles in subdirectories, it no longer builds with make.

It does. As @talisein has already pointed out, if your system uses make by default,

mkdir build
cd build
cmake ../
cd test-high
make # instead, I use cmake --build .
make test # instead, I use ctest

should just work fine.

imneme commented 5 years ago

I'm not likely to approve this line:

        bash <(curl -s https://codecov.io/bash) -G build-test-gcc/test-high/CMakeFiles

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.

aytekinar commented 5 years ago

Now, it is on a different branch.

I am simply following the documentation, and it shows the coverage.

acgetchell commented 4 years ago

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 external.

imneme commented 4 years ago

Sorry to have been slow on this. I haven't forgotten and hope to get to it soon.

SuperWig commented 4 years ago

Also why is a pcg-targets needed if there's no dependencies?

aytekinar commented 4 years ago

Also why is a pcg-targets needed if there's no dependencies?

I cannot understand this question. Could you please elaborate on this? Have you checked what's inside that pcg-targets.cmake file? I could not find any information in the documentation regarding any connection between the "exports"-file and "dependencies."

pcg-targets.cmake is included in pcg-config.cmake file (which is also generated), and it tells CMake what configuration options are needed to link against the so-called pcg::pcg target.

If you are talking about the pcg-config.cmake.in file, the dependencies are already commented out. If you do not like the commented out lines, we can definitely remove them. It's the place where you tell CMake what pcg depends on, if at all. That config file, together with pcg-config-version.cmake file tells CMake what pcg version is installed and what dependencies it has. Later, the config file loads the configuration options of the pcg::pcg target (i.e., pcg-targets.cmake).

SuperWig commented 4 years ago

I cannot understand this question. Could you please elaborate on this?

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
)

aytekinar commented 4 years ago

Yes. That solution gets rid of pcg-config.cmake.in file and removes a bunch of lines.

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 pcg::pcg and propagates its usage requirements (i.e., cxx_std_11 and the include directories) to the caller (in a simpler way).

SuperWig commented 4 years ago

I do not understand, this time, what you mean by

By that I mean pcg doesn't import any targets so there's no need for a -targets.cmake, unlike if for example pcg required Boost. Example usage by cli

https://github.com/daniele77/cli/blob/master/cliConfig.cmake.in

aytekinar commented 4 years ago

By that I mean pcg doesn't import any targets so there's no need for a -targets.cmake, unlike if for example pcg required Boost. Example usage by cli

https://github.com/daniele77/cli/blob/master/cliConfig.cmake.in

I still don't agree with your argument regarding the use of the -targets.cmake file. I think what you are trying to say is that we do not need to have a -config.cmake.in file that has the commented-out dependency lines as well as the @PACKAGE_INIT@ placeholder (and I agree with that).

In the end, the output of that config file you have proposed will be exactly the same as the one generated by @PACKAGE_INIT@. If you peek into either of the files, you will see that it is including all the -targets-*.cmake files in the directory automatically.

According to the documentation

The EXPORT form generates and installs a CMake file containing code to import targets from the installation tree into another project.

It is the CMakeFindDependencyMacro module that provides find_dependency macro, which wraps find_package, that finds and imports the dependencies' targets.

SuperWig commented 4 years ago

I'm saying there's no need for the -targets.cmake and the config.in because they aren't doing anything that isn't being done by

install(TARGETS pcg EXPORT pcg-config)

install(EXPORT pcg-config
  NAMESPACE pcg::
  DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/pcg
)

... find_package, that finds and imports the dependencies' targets. ..

That was an example of where the -config.cmake.in and -targets.cmake are actually being used and needed, sorry if that wasn't clear.

chiumichael commented 4 years ago

Are there any outstanding issues blocking this from being merged?

MBalszun commented 2 years ago

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?

lemire commented 2 years ago

I will ping Melissa by email.

imneme commented 2 years ago

I don't think this pull request is ready to merge as is. It installs the PCG sample programs in a INSTALL_DIR/bin which doesn't make any sense. They're just some casual demos of functionality, not something anyone would want to use on a regular basis and desire to have installed.

To get merged, that stuff needs to go.

But even there, in its present form, I'd put it on a cmake branch for folks who need to integrate PCG into CMake-based projects. In general, PCG is a C++ header-only library, and checking this list, although some header-only libraries provide CMake support, plenty don't. Poking around to look at ones that do, this one looks more like what I'd hope for. It has some sample programs, but doesn't go installing them always for everyone, and stresses that the whole CMake machinery is entirely optional, only for those that want/need it. Something more like that could be on the main branch.

aytekinar commented 2 years ago

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:

[x] (personal) do not start a PR unless there is an actual need for a feature (i.e., there is an issue with strong interest/support from not only the community but also the maintainer of the project),
[x] (personal) do not start a PR unless the definition of "ready/done" and acceptance criteria are in place, and,
[x] (pcg-cpp) the project maintainer(s) are willing/OK to move forward with the make-based approach, while leaving CMake as "an option."

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.

MBalszun commented 2 years ago

But even there, in its present form, I'd put it on a cmake branch for folks who need to integrate PCG into CMake-based projects. In general, PCG is a C++ header-only library, and checking this list, although some header-only libraries provide CMake support, plenty don't. Poking around to look at ones that do, this one looks more like what I'd hope for. It has some sample programs, but doesn't go installing them always for everyone, and stresses that the whole CMake machinery is entirely optional, only for those that want/need it. Something more like that could be on the main branch.

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 target_link_libraries(my_exe PUBLIC dep_library1 dep_library2) and be done with it. I don't want to check for each dependency if it is header only or if there is some special way I have to build/install it. If someone else just wants to copy the headers to a central include directory, a CMakeLists file in the root directory is not going to stop them from doing that.

@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".

aytekinar commented 2 years ago

@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 master branch, and there have been some issues and PRs that are also related to this PR.

MBalszun commented 2 years ago

@imneme : Btw.: Are you still monitoring this repository?

imneme / pcg-cpp