Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.13k stars 93 forks source link

allow using catch2 from system #208

Closed Apteryks closed 5 months ago

Apteryks commented 6 months ago

Hello!

I'm glad restinio now uses a recent Catch2 (3.5.0) version, but it'd be even nicer if it allowed users to select a system-provided copy instead of relying on its bundled source.

I was able to use the system provided one using the following Guix substitution:

(add-after 'change-directory 'use-system-catch2
            (lambda _
              (substitute* "CMakeLists.txt"
                (("add_subdirectory\\(catch2\\)")
                 "find_package(Catch2 REQUIRED)"))))

So not much is missing. the find_package call is necessary otherwise the include(Catch) directive below fails.

Cheers!

eao197 commented 6 months ago

Hi!

Can you check the latest commit from 0.7-dev-0.7.2-issue-208 branch? It's necessary to specify -DRESTINIO_DEP_CATCH2=find to the CMake configure (like here: https://github.com/Stiffstream/restinio-dockerfiles/blob/7b511213325091ad040b678bd2e0ea4ef18a714f/v0.7/archlinux-gcc-system-catch2.Dockerfile#L43-L44).

If it will work for you then I'll make a similar fix for #207.

Apteryks commented 6 months ago

Hi! It worked with -DRESTINIO_DEP_CATCH2=find, but not with -DRESTINIO_DEP_CATCH2=system. Perhaps it should, for uniformity with the other RESTINIODEP* options?

Thank you!

eao197 commented 6 months ago

I'm afraid that -DRESTINIO_DEP_CATCH2=system won't work because it's necessary to do include(Catch) in the root CMakeLists.txt and it seems that include(Catch) will work only after find_package(Catch2) or add_subdirectory(catch2).

Apteryks commented 6 months ago

Is there a benefit to separating 'find' and 'system'? From my user perspective, I'm not sure what it means; I'm always interested in using system libraries; I guess three uses cases could be:

  1. Enforce system library used (my use case)
  2. Use system, but fallback to bundled library if not found
  3. Use bundled library.

Is 'find' # 2 above?

eao197 commented 6 months ago

The system is for cases when a library is already installed in the system and there is no need to detect include/library paths.

The find is for cases when a non-system package management is used (like vcpkg/conan or something similar).

The local is for cases when something like git submodules, CMake's fetch_content or MxxRu::externals is used for managing dependencies.

Because sometimes CMake's find_package can find a system-wide library, the find option could be used for searching a library installed in the system.

The problem with Catch2 is that we need not only to find Catch2's header files and Catch2's .a/.lib files, but also use CMake's catch_discover_tests command (for that purpose include(Catch) is used in CMakeLists.txt).

Apteryks commented 6 months ago

Hi, and thanks for explaining. That's a very featureful library detection/fallback system! I guess -DRESTINIO_DEP_CATCH2=find can be fine, although I suspect some users may stumble upon trying -DRESTINIO_DEP_CATCH2=system.

Thank you!

eao197 commented 5 months ago

The fix is now a part of v.0.7.2. Fill free to reopen the issue if something goes wrong for you.

Apteryks commented 3 months ago

Tested OK! Thank you.