Better deps management

[Lord-Kamina]

I was motivated to make this PR by the fact RESTinio, as it is now, is a really good library but very unfriendly to deploy in most environments. Recently I realized my Macports Portfile had several issues and in addressing them, I ended up doing this.

I believe this makes it a lot friendlier and less prone to error; although I think you should really consider addressing some other points such as, 1) Consolidating all third-party or not-restinio related libs under third-party, instead of having some in there and some in /dev. 2) While it's probably ok and understandable for samples or benchmarks to have more requirements, I don't think it's a good idea to make the test suite dependent on relatively large libraries such as SObjectizer; especially considering it's not easily available through most (if any) package managers.

[eao197]

Thanks for your PR!

There are some controversial points we need to discuss inside our team. For example I don't like the idea of embedding any third-party libraries into RESTinio source tree. I also don't like approach when some RESTinio's header files are installed or not installed in dependency of some CMake flags.

[Lord-Kamina]

There are some controversial points we need to discuss inside our team. For example I don't like the idea of embedding any third-party libraries into RESTinio source tree.

I can relate to this, but we're talking specifically about two single-header libraries you depend on for your test-suite. In an ideal world, catch2 and args would be easily obtainable through major package managers and you wouldn't have to worry but that just isn't so. The alternative would be expecting most developers who use the library to completely forgo the test suite.

I also don't like approach when some RESTinio's header files are installed or not installed in dependency of some CMake flags.

In my PR the default behavior is to install everything. The thing is, you cannot install code that relies on missing dependencies, because that means the installation is broken. So, either make them mandatory (which would probably not be a popular choice in the case of three different engines essentially providing the same functionality) or do something similar to what I did.

[eao197]

The alternative would be expecting most developers who use the library to completely forgo the test suite.

I think there are (at least) two point of view on approach to distribute RESTinio. The first is to distribute RESTinio as a whole package with all tests, benchmarks, samples and so on. The second one is to distribute "light" version of RESTinio that contains only main RESTinio header files.

I understand use cases where "light" version of RESTinio will be used by end-users. And for that use cases there is no need to include third-party libraries in RESTinio source tree.

But what use cases for the first approach? Someone wants to get RESTinio and to play with samples or tests? Is system-wide package manager appropriate for that purpose?

The thing is, you cannot install code that relies on missing dependencies, because that means the installation is broken.

I see this problem that way:

An user write #include <restinio/router/pcre2_regex_engine.hpp> in source code and get compiler error that this file is missing. It is not easy to understand why this file is missing: may be RESTinio installation is broken.

But if an user gets linker error or compiler error about missing PCRE2 headers then it identifies the problem more clearly.

[ngrodzitski]

We would definitely make changes based on PR. Actually, some parts of it are already in our private dev repos on bitbucket (move to newer catch). But it would take some time to evaluate the proposed changes.

Regarding MacPorts can you, please, share your scripts, maybe we can do a kind of official port based on it?

[Lord-Kamina]

We would definitely make changes based on PR. Actually, some parts of it are already in our private dev repos on bitbucket (move to newer catch). But it would take some time to evaluate the proposed changes.

For the record; again, I just added the catch include to the source as a fail-safe (which IMO can be easily understood from the fact I wrote it so it's only used if Catch2 can't be located by regular means); in fact, I also wrote a Catch Portfile to submit to macports along with my update if this PR is accepted. That include is meant, for example, for people who want to build RESTinio on Linux, where package managers don't ship out Catch2 at all.

Regarding MacPorts can you, please, share your scripts, maybe we can do a kind of official port based on it?

As I say, I haven't finished it yet but this P.R. would supercede much of what I had and fix most of the errors or gotchas in building for Mac.

In the original version I wrote (which I tested by compiling and testing the basic example in your README) I had to use some very hacky means to produce a custom install because when I wrote it, make install didn't actually work. Only by taking a second-look at it in comparison to the later releases of RESTinio did I realize it's also broken. Nonetheless, you can take a look here:

https://github.com/macports/macports-ports/commit/8c387f2ab2c8ea3a6d039a867eb72e1efc13068a

For it to work I also needed to patch out a very large section of CMakeLists.txt, and I think that is hardly ideal.

I think there are (at least) two point of view on approach to distribute RESTinio. The first is to distribute RESTinio as a whole package with all tests, benchmarks, samples and so on. The second one is to distribute "light" version of RESTinio that contains only main RESTinio header files.

But what use cases for the first approach? Someone wants to get RESTinio and to play with samples or tests? Is system-wide package manager appropriate for that purpose?

I disagree. You're right about samples of course, but not tests. Tests enable you to check that what you got actually works. That's why I didn't even bother with samples and benchmarks on this PR. I think those are truly optional... not so for unit-tests.

I understand use cases where "light" version of RESTinio will be used by end-users. And for that use cases there is no need to include third-party libraries in RESTinio source tree.

Except you already do, you have optional-lite, string-view-lite, variant-lite and even zlib, which is a compiled library.

An user write #include <restinio/router/pcre2_regex_engine.hpp> in source code and get compiler error that this file is missing. It is not easy to understand why this file is missing: may be RESTinio installation is broken.

But if an user gets linker error or compiler error about missing PCRE2 headers then it identifies the problem more clearly.

Perhaps, but for that scenario to play-out this hypothetical user would have to manually disable PCRE2 and then include the header; as I said earlier, my PR does not alter the default behavior in any way; it just makes RESTinio able to build without modification on a larger array of different configurations and environments while also giving individual users the option to leave out parts they're not going to use.

[ahmedyarub]

Any progress here? I can definitely help with deps management for Windows, Linux and Mac

[ArchangeGabriel]

I’m also interested in the patch to find a system http_parser, I had to patch locally in order to allow that and successfully build restinio.

[eao197]

@ArchangeGabriel

I had to patch locally in order to allow that and successfully build restinio.

Can you show your patch?

[ArchangeGabriel]

@eao197 It’s way dirtier than the one proposed here, but works for my case(were I don’t care about fallback, because the library is going to be found) . You can see what I do in the prepare() function here: https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/restinio And the added file is here: https://git.archlinux.org/svntogit/community.git/tree/trunk/FindHTTP_Parser.cmake?h=packages/restinio

[eao197]

@ArchangeGabriel

Thank you. I'll see what can we do to allow users to use RESTinio with already installed http-parser package.

[ArchangeGabriel]

@eao197 Why not integrate the part of this PR that does this? I can cherry-pick that part and rebase it on master if you want (and if @Lord-Kamina does not want to do it himself).

[Lord-Kamina]

I can probably do it come weekend.

[eao197]

Why not integrate the part of this PR that does this?

The main reason: I want to fully understand what is going on in project's CMake files. The only working way to do that for me is to make, check and debug changes by myself. Sorry that this slows down the adoption of RESTinio and disturbs users.

I'm also afraid that the simple changes will break compatibility with vcpkg, where unofficial-http-parser is used.

So I'll take some time to think, try and evaluate proposed variants by myself.

[eao197]

The branch "0.6-dev-0.6.5" now contains changes that allow using an already installed version of http-parse. To activate this feature use `RESTINIO_USE_EXTERNAL_HTTP_PARSER" option during the call to CMake.

For example, to use external http-parser, but bundled catch2 and fmtlib:

cmake -DRESTINIO_USE_EXTERNAL_HTTP_PARSER=ON -DCMAKE_BUILD_TYPE=...

To use external versions of http-parser and other deps:

cmake -DRESTINIO_FIND_DEPS=ON -DRESTINIO_USE_EXTERNAL_HTTP_PARSER=ON -DCMAKE_BUILD_TYPE=...

[ArchangeGabriel]

@eao197 Thanks for those changes, they look good enough to me. I can’t try them right now, but will give them a shot when possible.

[eao197]

@ArchangeGabriel There is no hurry. Those changes are for v.0.6.5 that will be released not early than next week.

[doronbehar]

Hey all, I came about this PR after trying to package restinio to my distro. I noticed 2 issues:

1. There's no way to use SObject sanitizer from the system and the so_5 directory is not included in the git clone / GitHub's archive (I know I can use the release tarball but that's still a sort of inconvenience). This creates confusing cmake errors when using the Git clone:

CMake Error at CMakeLists.txt:165 (add_subdirectory):
  add_subdirectory given source "so_5" which is not an existing directory.

2. clara is unmaintained, see their README. Lyra is a recommended and maintained replacement. It would be nice if it was possible to use Lyra instead of clara2 from the system as well from your release tarball. At the moment, even if lyra was officially supported, it is not possible to use it, as there's no way to tell cmake to find_package(lyra) and not add_directory(clara2). See https://github.com/Stiffstream/restinio/blob/c4ed2d0956948d47a8fd569f3c0f3d2113219421/dev/CMakeLists.txt#L80-L100

I'm using the release tarball now which is not too bad.

Regards.

[doronbehar]

This is roughly what I imagine as an ideall CMakeLists.txt for packagers to interact with:

diff --git i/dev/CMakeLists.txt w/dev/CMakeLists.txt
index 515e8ba..be329fb 100644
--- i/dev/CMakeLists.txt
+++ w/dev/CMakeLists.txt
@@ -16,10 +16,6 @@ option(RESTINIO_SAMPLE "Build samples." ${RESTINIO_MASTER_PROJECT})
 option(RESTINIO_INSTALL_SAMPLES "Build install samples." ${RESTINIO_MASTER_PROJECT})
 option(RESTINIO_BENCH "Build the test target." ${RESTINIO_MASTER_PROJECT})
 option(RESTINIO_INSTALL_BENCHES "Build install samples." ${RESTINIO_MASTER_PROJECT})
-option(RESTINIO_FIND_DEPS "Use `find_package()` for including RESTinio dependencies." OFF)
-option(RESTINIO_FMT_HEADER_ONLY "Include fmt library as header-only." ON)
-
-option(RESTINIO_ALLOW_SOBJECTIZER "Allow add_subdirectory(so_5) in the main RESTinio CMakeLists.txt" ${RESTINIO_MASTER_PROJECT})

 # Since v.0.6.5 a user can force usage of already installed http-parser
 # library.
@@ -36,6 +32,13 @@ option(RESTINIO_USE_EXTERNAL_STRING_VIEW_LITE
 option(RESTINIO_USE_EXTERNAL_VARIANT_LITE
    "Use already installed variant-lite library" OFF)

+# Since v.0.6.8.2 a user can force usage of already installed fmt, sobjectizer
+# & lyra (replacing clara) libraries.
+option(RESTINIO_USE_EXTERNAL_FMT "Use already installed fmtlib" OFF)
+option(RESTINIO_FMT_HEADER_ONLY "If using release tarball fmt, include fmt library as header-only." ON)
+option(RESTINIO_USE_EXTERNAL_LYRA "Use already installed lyra" OFF)
+option(RESTINIO_USE_EXTERNAL_SOBJECTIZER "Use already installed sobjectizer" OFF)
+
 SET(RESTINIO_USE_BOOST_ASIO "none" CACHE STRING "Use boost version of ASIO")
 SET(RESTINIO_USE_BOOST_ASIO_VALUES "none;static;shared")

@@ -77,41 +80,7 @@ IF (RESTINIO_MASTER_PROJECT)
        find_package(Boost 1.66.0 REQUIRED COMPONENTS system regex)
    ENDIF ()

-   IF (RESTINIO_FIND_DEPS)
-       # Require necessary packages.
-
-       # Catch2
-       IF (RESTINIO_TEST)
-           find_package(Catch2 CONFIG REQUIRED)
-       ENDIF ()
-
-       # HTTP parser
-       IF (RESTINIO_USE_EXTERNAL_HTTP_PARSER)
-           find_package(http-parser REQUIRED)
-       ELSE ()
-           find_package(unofficial-http-parser CONFIG REQUIRED)
-       ENDIF ()
-
-       # fmtlib
-       find_package(fmt REQUIRED)
-   ELSE ()
-       # Add necessary dependecies.
-
-       include_directories("${CMAKE_SOURCE_DIR}/clara")
-
-       # HTTP parser
-       IF (RESTINIO_USE_EXTERNAL_HTTP_PARSER)
-           find_package(http-parser REQUIRED)
-       ELSE ()
-           add_subdirectory(nodejs/http_parser)
-       ENDIF ()
-
-       # fmtlib
-       add_subdirectory(fmt)
-   ENDIF ()
-
-   # If RESTINIO_USE_EXTERNAL_*_LITE is defined we should
-   # search that libs regardless of RESTINIO_FIND_DEPS option.
+   # External vs vended libraries
    IF (RESTINIO_USE_EXTERNAL_EXPECTED_LITE)
        find_package(expected-lite REQUIRED)
    ENDIF()
@@ -124,6 +93,23 @@ IF (RESTINIO_MASTER_PROJECT)
    IF (RESTINIO_USE_EXTERNAL_VARIANT_LITE)
        find_package(variant-lite REQUIRED)
    ENDIF()
+   IF (RESTINIO_USE_EXTERNAL_HTTP_PARSER)
+       find_package(http-parser REQUIRED)
+   ELSE ()
+       add_subdirectory(nodejs/http_parser)
+   ENDIF ()
+   IF (RESTINIO_USE_EXTERNAL_FMT)
+       find_package(fmt REQUIRED)
+   ELSE ()
+       # TODO: Include only if existing
+       add_subdirectory(fmt)
+   ENDIF()
+   IF (RESTINIO_USE_EXTERNAL_LYRA)
+       # TODO: lyra doesn't include cmake files?
+       find_package(lyra REQUIRED)
+   ELSE ()
+       include_directories("${CMAKE_SOURCE_DIR}/clara")
+   ENDIF()

    # ------------------------------------------------------------------------------

@@ -157,15 +143,20 @@ IF (RESTINIO_MASTER_PROJECT)
        message( STATUS "PCRE2_INCLUDE_DIRS='" ${PCRE2_INCLUDE_DIRS} "'" )
    ENDIF ()

-   # ------------------------------------------------------------------------------
-   # SObjectizer
-   IF (RESTINIO_ALLOW_SOBJECTIZER)
-       SET(SOBJECTIZER_BUILD_STATIC ON)
-       SET(SOBJECTIZER_LIBS sobjectizer::StaticLib)
-       add_subdirectory(so_5)
+   IF (RESTINIO_TEST)
+       find_package(Catch2 CONFIG REQUIRED)
+       # ------------------------------------------------------------------------------
+       # SObjectizer
+       IF (RESTINIO_USE_EXTERNAL_SOBJECTIZER)
+           find_package(sobjectizer REQUIRED)
+       ELSE ()
+           SET(SOBJECTIZER_BUILD_STATIC ON)
+           SET(SOBJECTIZER_LIBS sobjectizer::StaticLib)
+           add_subdirectory(so_5)
+       ENDIF ()
+       # ------------------------------------------------------------------------------
    ENDIF ()

-   # ------------------------------------------------------------------------------
    # Zlib
    find_package(ZLIB)

But this is not functional yet as e.g installing lyra on a system doesn't install cmake files with which one can find_package, for AFAIK (at least on my distro and as it seems from their git repo). Besides that, I don't know if sobjectizer is discoverable with find_package.

[eao197]

Hi @doronbehar !

We are planning to switch from Clara to Lyra in RESTinio-0.7. Until that Clara will be used in RESTinio because it works and it isn't a major part of RESTinio (it's used only for benchmarks and examples).

SObjectizer is also not a mandatory dependency for RESTinio. There will be relaxed rules for SObjectizer in RESTinio-0.6.9: settings of RESTINIO_ALLOW_SOBJECTIZER to OFF will turn off just a couple of tests/examples that require SObjectizer (those changes are part of an experimental branch 0.6-fix-libatomic-issue-98: https://github.com/Stiffstream/restinio/commit/a0d393a2672ccc1fa5c9a752965059b8c2fff4fb).

It's hard to predict when RESTinio-0.6.9 will be released but I hope it will be at the middle of Aug.

[doronbehar]

Thanks for the quick reply @eao197 :) I trust you to provide the next release(s) with the best experience for us downstream packagers. As for SObjectizer, I wanted my distro to run the tests, that's why I had to enable it, per:

https://github.com/Stiffstream/restinio/blob/c4ed2d0956948d47a8fd569f3c0f3d2113219421/dev/CMakeLists.txt#L44-L47

It's ok to require this library to run tests, it's just a bit inconvenient to have to use your vendored version and not the system's. Ideally, I'd like to see in the next release that all dependencies could be used from the system. Also, note that in my CMakeLists.txt diff, I "squashed" some of the logic and removed the option RESTINIO_FIND_DEPS as it's too general. IMHO, It should be possible to control whether the build will use the vendored vs the system's dependency, per dep individually.