Use Catch2 as normal dependency (not as submodule) #527

tdegeus · 2022-03-28T12:54:40Z

tdegeus · 2022-03-28T12:59:15Z

~~Actually I don't think this works yet~~

Should be good to go!

matz-e · 2022-03-28T13:40:35Z

CMakeLists.txt

+ find_package(Catch2)
+ if(EXISTS ${CMAKE_CURRENT_LIST_DIR}/deps/catch2/CMakeLists.txt)
 add_subdirectory(deps/catch2 EXCLUDE_FROM_ALL)
 list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/deps/catch2/contrib)


One issue I have here is that I'm concerned about CMake mixing things up.

For example, we find Catch2, setting a bunch of stuff. Then, if the submodule is present, we include it and append to the search path. Which means that include(Catch) would probably first look at the system Catch2, no?

It would seem cleaner to me to look for Catch2 only if the subdirectory is empty, printing a status message about the missing submodule and then using REQUIRE, maybe?

FWIW, find_library seems to be in the CMake 3.1 we require, what was wrong with it?

I find add_subdirectory(deps/catch2 EXCLUDE_FROM_ALL) very un-Cmake, and that is probably where the issues start. Personally I would find it much simpler (and much more in the spirit of Cmake ;)) to remove the submodule all-together. We did not submodule Boost either and that did not pose any difficulties. Nowadays I do not consider Catch2 much less standard than Boost.

catch2 is packaged under debian / ubuntu / archlinux / fedora at least

and on conda-forge, and I imagine other package managers

I don't have strong opinions but to mention few aspects:

We did not submodule Boost either and that did not pose any difficulties.

I would say Boost is different beast altogether. As we depend on library components, (as we all know) one can't just submodule boost...

catch2 is packaged under debian / ubuntu / archlinux / fedora at least
and on conda-forge, and I imagine other package managers

Using system package managers is perfectly fine when we are doing development on our own system / desktop environments. When we move to clusters (where many of us work), the same doesn't apply. Boost availability via module has been quite universal for long time. I wouldn't say the same thing for Catch2.

Personally I would find it much simpler (and much more in the spirit of Cmake ;)) to remove the submodule all-together.

People involved in this discussion knows how to make catch2 available if needed. But IMO, we don't loose much by adding submodule for lightweight package is like Catch2. As @matz-e mentioned, it's just convenient, commonly used approach and one doesn't need to bother about the details.

So I think it's ok to use external packages but shouldn't get rid of submodule altogether.

There is not something profoundly bad about carrying a submodule. However, my considerations are:

Have a submodule makes as responsible for keeping it up-to-date. Not carrying the submodule reduces work-load in that sense.

I wonder how many people will actually run the unit-tests for highfive. We are responsible for that, and indeed we run quite a bit of CI. So I wonder how many people benefit.

My proposition is on of :

Having the submodule, but not require its clone by default (so having a fallback that takes the system's)

Having an option to specify the path to the user's download of Catch2 (instead of using the system's).

There is already an official way to tell where is Catch2 with cmake, don't add such option

Having the submodule, but not require its clone by default (so having a fallback that takes the system's)

This can be done.

Having an option to specify the path to the user's download of Catch2 (instead of using the system's).

Don't understand this. You can always install Catch2 independently and use -DCMAKE_PREFIX_PATH=…

catch2 is packaged under debian / ubuntu / archlinux / fedora at least

Looking into it a bit more. This is not the case for Ubuntu LTS 20.04. Catch2 occurs first in 21.04. Not to speak of RedHat…

.gitmodules

matz-e · 2022-03-28T15:14:18Z

CMakeLists.txt

@@ -24,7 +24,6 @@ set(USE_OPENCV OFF CACHE BOOL "Enable OpenCV testing")
 mark_as_advanced(USE_BOOST USE_EIGEN USE_XTENSOR)

 option(HIGHFIVE_USE_BOOST "Enable Boost Support" ${USE_BOOST})
-option(HIGHFIVE_USE_EXTERNAL_CATCH2 "Enable external Catch2 for unit tests" OFF)


I think with the submodule gone, and a (compared to Boost and HDF5) less common dependency present, unit tests should be in a tristate:

AUTO (default) to enable unit tests if Catch2 is found

ON to require Catch2 and enable unit tests

OFF to disable unit tests and the Catch2 dependency

(this would be a good way to get switch to disable the Boost dependency, too, IMO)

(see also https://cmake.org/pipermail/cmake/2016-October/064342.html)

I'm fine with this.

Just of clarify:

AUTO: enable tests only of Catch2 is found

ON: force run tests -> fail if Catch2 is not found

OFF: do not run tests
this seems fine to me. If the user choses to install HighFive this is of limited added value though, but certainly not bad.

It would be indeed ok to make this the default of the target too. Still it feels like dragging it along. I would still argue to issue a warning that it won't still the default for a while, and then remove Boost from the target, making it strictly optional, or much better fully modular.

@pramodk and @ohm314 are against deleting the submodule. We should keep the previous system...

.github/workflows/gh-pages.yml

codecov-commenter · 2022-03-28T15:22:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f5706b2). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head b45896b differs from pull request most recent head 7780565. Consider uploading reports for the commit 7780565 to get more accurate results

@@            Coverage Diff            @@
##             master     #527   +/-   ##
=========================================
  Coverage          ?   78.72%           
=========================================
  Files             ?       64           
  Lines             ?     3361           
  Branches          ?        0           
=========================================
  Hits              ?     2646           
  Misses            ?      715           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5706b2...7780565. Read the comment docs.

tdegeus · 2022-03-28T16:30:21Z

Do you know what is going on @matz-e @alkino ?

matz-e · 2022-03-29T14:15:54Z

Do you know what is going on @matz-e @alkino ?

Mamba does not install the right HDF5. Pretty much don't like this - from the looks of the CI file, we rely on Ubuntu system packages, why do we mix and match package managements?

-- Found HDF5: /home/runner/micromamba/envs/myenv/lib/libhdf5.so (found version "1.12.1")  
CMake Warning at CMake/HighFiveTargetDeps.cmake:17 (message):
  Parallel HDF5 requested but libhdf5 doesnt support it
Call Stack (most recent call first):
  CMakeLists.txt:60 (include)

@tdegeus

@tdegeus, I think this should fix #527 sufficiently. Any CI setup therein should maybe be followed up separately. A bit of a butchered CMake in my opinion, but given the "language" constraints, it seems to work. Will set unit tests to AUTO, disabling them if neither the submodule nor a system package for Catch2 is found. The submodule will be preferred, otherwise. Errors out if unit tests are set to ON and no Catch2 is found.

@tdegeus

@tdegeus, I think this should fix #527 sufficiently. Any CI setup therein should maybe be followed up separately. A bit of a butchered CMake in my opinion, but given the "language" constraints, it seems to work. Will set unit tests to AUTO, disabling them if neither the submodule nor a system package for Catch2 is found. The submodule will be preferred, otherwise. Errors out if unit tests are set to ON and no Catch2 is found.

alkino · 2022-03-31T16:23:23Z

This PR is subsided by #535

@tdegeus

Lifts the mamba part of #527 from @tdegeus, fixes a bit of CMake and covers more combinations of dependencies when testing.

matz-e reviewed Mar 28, 2022

View reviewed changes

tdegeus mentioned this pull request Mar 28, 2022

v2.4.0 #528

Merged

tdegeus changed the title ~~Auto-load Catch2~~ Use Catch2 as normal dependency (not as submodule) Mar 28, 2022

tdegeus requested review from alkino and matz-e March 28, 2022 14:47

matz-e reviewed Mar 28, 2022

View reviewed changes

tdegeus added 3 commits March 28, 2022 17:17

Removing catch2 as submodule

a8c3af9

Using provision-with-micromamba

e034db2

Adding deps to CI

7780565

Using provision-with-micromamba in CI

36017ec

matz-e mentioned this pull request Mar 31, 2022

Fall back to a Catch2 in the CMake paths #535

Merged

alkino closed this in #535 Mar 31, 2022

matz-e mentioned this pull request Apr 1, 2022

Try to test more combinatorics in the CI #536

Merged

matz-e added a commit that referenced this pull request Apr 1, 2022

Try to test more combinatorics in the CI (#536)

746791d

Lifts the mamba part of #527 from @tdegeus, fixes a bit of CMake and covers more combinations of dependencies when testing.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Catch2 as normal dependency (not as submodule) #527

Use Catch2 as normal dependency (not as submodule) #527

tdegeus commented Mar 28, 2022

tdegeus commented Mar 28, 2022 •

edited

Loading

matz-e Mar 28, 2022

matz-e Mar 28, 2022

tdegeus Mar 28, 2022

alkino Mar 28, 2022

tdegeus Mar 28, 2022

pramodk Mar 28, 2022 •

edited

Loading

tdegeus Mar 29, 2022

alkino Mar 29, 2022

matz-e Mar 31, 2022

matz-e Mar 31, 2022

matz-e Mar 28, 2022

tdegeus Mar 28, 2022 •

edited

Loading

alkino Mar 28, 2022

codecov-commenter commented Mar 28, 2022

tdegeus commented Mar 28, 2022

matz-e commented Mar 29, 2022

alkino commented Mar 31, 2022

Use Catch2 as normal dependency (not as submodule) #527

Use Catch2 as normal dependency (not as submodule) #527

Conversation

tdegeus commented Mar 28, 2022

tdegeus commented Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pramodk Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdegeus Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 28, 2022

Codecov Report

tdegeus commented Mar 28, 2022

matz-e commented Mar 29, 2022

alkino commented Mar 31, 2022

tdegeus commented Mar 28, 2022 •

edited

Loading

pramodk Mar 28, 2022 •

edited

Loading

tdegeus Mar 28, 2022 •

edited

Loading