restinio icon indicating copy to clipboard operation
restinio copied to clipboard

Optional dependencies not actually optional.

Open Lord-Kamina opened this issue 6 years ago • 18 comments

I've been following RESTinio a bit as I plan to use it for a game and also because I've been making a Macports portfile. Initially, the cmake file was so broken I had to implement a custom install routine in the portfile (which worked but is a PITA to maintain). As such, I was looking into the changes introduced in recent versions and I noticed that in fact most of the issues appear to have been fixed.

However, SObjectizer is still always being expected/included by CMake. Shouldn't this be wrapped in a conditional for tests, samples or somesuch?

Lord-Kamina avatar Oct 03 '18 12:10 Lord-Kamina

Shouldn't this be wrapped in a conditional for tests, samples or somesuch?

Yes, I think we can do something like that. But it can take some time because we are a little busy with other projects. I hope we resolve this issue in several days.

Thanks for reporting it.

eao197 avatar Oct 03 '18 13:10 eao197

Shouldn't this be wrapped in a conditional for tests, samples or somesuch?

Yes, I think we can do something like that. But it can take some time because we are a little busy with other projects. I hope we resolve this issue in several days.

Thanks for reporting it.

For now I'll be adding patches to fix issues in my portfile. Similar issues I see are with OpenSSL which is billed as "optional" (I figure you mean, if you don't need TLS don't include it and it should be fine?). There should perhaps also be a CMake option to handle that and a stub tls in case OpenSSL is not available. As far as I can tell, Restinio doesn't use OpenSSL directly anyway bur rather through asio and asio::ssl, no?

And pretty much the same thing for PCRE/Boost regex. They're both looked for always and I don't think the CMake file actually allows to choose between the two (or eventually allow supporting both if they're not mutually exclusive)

Lord-Kamina avatar Oct 03 '18 13:10 Lord-Kamina

And pretty much the same thing for PCRE/Boost regex. They're both looked for always and I don't think the CMake file actually allows to choose between the two (or eventually allow supporting both if they're not mutually exclusive)

Usage of PCRE/PCRE2 or Boost is not controlled by CMake. You select which one will be used by manually including restinio/router/pcre_regex_engine.hpp or restinio/router/boost_regex_engine.hpp in your C++ file (see this section for more details). CMake script just calls find_package to check presence of these tools.

eao197 avatar Oct 03 '18 13:10 eao197

And pretty much the same thing for PCRE/Boost regex. They're both looked for always and I don't think the CMake file actually allows to choose between the two (or eventually allow supporting both if they're not mutually exclusive)

Usage of PCRE/PCRE2 or Boost is not controlled by CMake. You select which one will be used by manually including restinio/router/pcre_regex_engine.hpp or restinio/router/boost_regex_engine.hpp in your C++ file (see this section for more details). CMake script just calls find_package to check presence of these tools.

I figured the logic was that. It's a relatively sensible approach but far some failsafe. I'd recommend having CMake switches to choose between either, both or none and installing files as appropriate.

I personally think restinio is a great library with lots of potential, but have seen most of the criticism it has gotten (on reddit for example) is because of the heavy dependencies.

Now, I realize that most of these are truly optional; but by not making them so in CMakeLists, you're probably giving the impression that the dependencies are heavier than they actually are and alienating people who would otherwise use the RESTinio.

Anyway, cheers.

Lord-Kamina avatar Oct 03 '18 14:10 Lord-Kamina

For now I'll be adding patches to fix issues in my portfile.

Can you, please, share your patch with us?

And by the way, regarding MacPorts (no experience with it), is it possible to pass cmake definition to be applied for a given lib?

ngrodzitski avatar Oct 03 '18 14:10 ngrodzitski

For now I'll be adding patches to fix issues in my portfile.

Can you, please, share your patch with us?

And by the way, regarding MacPorts (no experience with it), is it possible to pass cmake definition to be applied for a given lib?

You can pass arguments into most steps like configure.args-appendor build.post_args-appendand such; passing options to cmake is just appending -DCMAKE_SOME_VARIABLE=VALUE to args during configure step.

Can you, please, share your patch with us?

For now I was doing some very basic things, mostly removing some stuff that would not be needed for a basic install. Will definitely share once I've done something more robust.

Lord-Kamina avatar Oct 03 '18 14:10 Lord-Kamina

So... I've decided to make a few substantial changes to the way RESTinio manages at least some of its dependencies after all. I will submit a PR once I'm done, but for now I've got some questions.

Why are there apparently benchmarks (that have another dependency) in the tests folder?

Lord-Kamina avatar Oct 03 '18 22:10 Lord-Kamina

Why are there apparently benchmarks (that have another dependency) in the tests folder?

I suppose it because we saw these benchmarks as part of test suite.

eao197 avatar Oct 04 '18 07:10 eao197

As a comment on how to use this as a "standalone" 3rd party build in a larger docker build we do the following:

wget -O restinio.tar.gz "https://github.com/Stiffstream/restinio/archive/v.0.5.1.tar.gz" &&
mkdir -p restinio &&
tar
--extract
--file restinio.tar.gz
--directory restinio
--strip-components 1 cd restinio cd dev sed -i.bak '/add_subdirectory(so_5)/d' CMakeLists.txt mkdir build && cd build cmake -DRESTINIO_TEST=OFF -DRESTINIO_SAMPLE=OFF -DRESTINIO_INSTALL_SAMPLES=OFF -DRESTINIO_BENCH=OFF -DRESTINIO_INSTALL_BENCHES=OFF -DRESTINIO_FIND_DEPS=ON .. ./configure make sudo make install cd ..

The only thing that is ugly is the sed to get rid of the add_subdirectory(so_5)

Could add an option to not always add so_5 since it's not actually required, please :-)

Otherwise, thanks for an excellent library!!

skarlsson avatar Jun 29 '19 14:06 skarlsson

I second that the optional dependencies are not optional due to simple fact that the conditional if else with the -RESTINIO_FIND_DEPS at this line does not contain the further find_package(OpenSSL), find_package(PCRE), find_package(PCRE2), find_package(ZLIB).

Moreover, the else condition in case one sets RESTINIO_FIND_DEPS=Off will look for local folders which is still forcing you to use the dependencies that you don't want.

As @skarlsson suggested the only way to truly chose dependencies is to activate their finding to avoid including relative directories and then remove with sed all find_package outside of the scope of the dependencies conditional.

Personally, I think that it is a huge overhead for integrating a headers only library that doesn't even need to generate a shared library to pass simple dependencies checks that should be inside of the same conditional scope to be able to control their checks through cmake variables.

binarytrails avatar Jul 11 '19 14:07 binarytrails

Certainly my PR would need to be updated anf possibly refined but I think it provides a more than acceptable solution to the issue and would greatly improve usability and inclusion in package managers for different platforms.

Lord-Kamina avatar Jul 11 '19 17:07 Lord-Kamina

I've added an option RESTINIO_ALLOW_SOBJECTIZER to the main CMakeLists.txt. If that option is OFF then add_subdirectory(so_5) won't be used. It allows to run cmake that way:

cmake -DRESTINIO_TEST=OFF \
   -DRESTINIO_SAMPLE=OFF \
   -DRESTINIO_BENCH=OFF \
   -DRESTINIO_ALLOW_SOBJECTIZER=OFF \
   -DRESTINIO_FIND_DEPS=ON ..

If RESTINIO_TEST or RESTINIO_SAMPLE or RESTINIO_BENCH is ON then RESTINIO_ALLOW_SOBJECTIZER can't be OFF.

The updated version is in master branch. It would be great if someone tried it and provided some feedback: is that change solve the problem or not?

eao197 avatar Jul 18 '19 06:07 eao197

Works! Thanks!

skarlsson avatar Jul 18 '19 08:07 skarlsson

Hey! I'll try it as soon as I get a chance. I'd still prefer to be able to individually choose the regex implementations but if that's just not an option for you guys I guess I can just turn those into a patch for whichever package manager.

Sent with GitHawk

Lord-Kamina avatar Jul 19 '19 16:07 Lord-Kamina

@Lord-Kamina I would like to join the effort. I do have decent experienc in such tasks. Please take a look at my WIP micro-service based on Restinio: https://github.com/ahmedyarub/micro-service

ahmedyarub avatar Aug 01 '19 09:08 ahmedyarub

@ahmedyarub not sure to what effort you're referring 😅

Sent with GitHawk

Lord-Kamina avatar Aug 01 '19 12:08 Lord-Kamina

Simplifying the process of acquiring and building dependencies by including everything in the CMake script https://stiffstream.com/en/docs/restinio/0.5/obtaining.html#prerequisites

ahmedyarub avatar Aug 02 '19 01:08 ahmedyarub

you should make a clear statement in the README.md possible just point to the prerequisites you point to here

When I read there that RESTinio was a "header-only C++14 library" I was led to think I could just include it, and be good. But that is not the case.

ti8m-hic avatar Oct 16 '19 19:10 ti8m-hic