jll63 / yomm2

Fast, orthogonal, open multi-methods. Solve the Expression Problem in C++17.
Boost Software License 1.0
343 stars 18 forks source link

CMake overhaul #12

Closed derpda closed 4 years ago

derpda commented 4 years ago

Sorry for the pull request out of the blue. I am suggesting major changes to the CMake scripts of yomm2. First let me explain why I was motivated to make these changes

Motivation

When using yomm2 as a dependency, we usually employ the same technique as suggested at for googletest: Download, build and locally install yomm2 automatically during CMake configure step using ExternalProject_Add and include the result in our main build. For yomm2, it was not possible to use CMake's find_package command to include yomm2 in our build since yomm2 does not define a proper CMake package upon installation. This is only a minor inconvenience, but was enough to motivate me to work on yomm2's CMake scipts. Additionally, "benchmark" was always downloaded even when YOMM2_ENABLE_BENCHMARKS was not set to ON, which takes unnecessary time.

Content of Pull Request

I have mainly made two changes:

  1. Defined CMake package building instructions (install instructions) for yomm2
  2. Overhaul of how the "benchmark" dependency is pulled and built

As for the first point, the commits are fairly short and are the best explanation of the content of my changes that I can come up with at the moment.

For the second point, there are a few advantages to the new method of automatically downloading and building "benchmark"

Conclusion

I believe the suggested changes make yomm2 a more complete library, as it is now a fully defined CMake package that can easily be used from user side code (like the library we're working on).

Please let me know if you have any questions, and I will try to respond quickly!

derpda commented 4 years ago

I forgot to add something: In the commit "Compilation of examples made optional" I have changed the script so that examples will only be built when YOMM2_BUILD_EXAMPLES=ON is set explicitly, just like for tests and benchmarks. I probably should have considered that you might want those to be built by default. If that is the case, we could set the default of YOMM2_BUILD_EXAMPLES to ON and instead allow their compilation to be turned off by setting YOMM2_BUILD_EXAMPLES=OFF. For user-side code like our library, this would be a nice setting to have since we don't need examples and they take much longer to build than the actual library since yomm2 is mostly headers.

jll63 commented 4 years ago

Thank you so much for working on this! As you can tell, I did not spend a lot of time on the cmake stuff, which is not my field of expertise. Can you look take a look at this error please? https://travis-ci.org/github/jll63/yomm2/jobs/678822450

jll63 commented 4 years ago

Also could you update the README.md? It still mentions the submodule. It would be nice if you could make it current with your changes.

derpda commented 4 years ago

There should be some way to make the CI output show up here for this pull request, that way I could confirm the results myself! I think I know how to fix the issue though.

derpda commented 4 years ago

Nevermind about setting up Travis CI for this project, looks like it's $69/month...

jll63 commented 4 years ago

We're almost there: it passes on Travis but not on Windows. See my review. Thanks for the README :)

derpda commented 4 years ago

While doing some testing, I found an unrelated small issue in blackbox.cpp: Two tests have the same name and boost::tests complains about this, causing blackbox to fail.

derpda commented 4 years ago

This latest commit should fix the issue on Windows, could you confirm that for me?

jll63 commented 4 years ago

This latest commit should fix the issue on Windows, could you confirm that for me?

It all looks good. Merging now. Thanks for this.