RESTinio CMake issues - Githubissues

Lord-Kamina commented 2 years ago

This is an offshoot from #166. I normally use a modified version of RESTinio to suit my main project. Obviously, for a test case I am trying to set-up vanilla RESTinio to see if the error persists.

However, the CMakeLists contains several errors that make it pretty hard to use. The whole IF (RESTINIO_MASTER_PROJECT) means RESTinio cannot be used properly via add_subfolder. I generally always prefer to use find_package, and I know that RESTinio provides CMake config files, but given that it's not a library commonly available in many package managers, add_subdirectory would be a pretty common way to include it in a project.

The issue is, when included like that, RESTINIO_MASTER_PROJECT is not defined. When that happens, RESTinio does not even try to look for FMT, so then restinio/CMakeLists.txt produces an error because it's trying to link against a non-existent target, and similar errors will likely occur for most other dependencies.

If one just removes that check, http_parser is still a problem. If RESTINIO_FIND_DEPS is on but USE_EXTERNAL_HTTP_PARSER isn't, RESTinio tries to use find_package to find a library that hasn't been installed anywhere because it's included in RESTinio. The correct behavior would be to use add_subdirectory(nodejs/http_parser) as is being done when RESTINIO_FIND_DEPS is off. It might also be a good idea to have USE_EXTERNAL_HTTP_PARSER be the default.

Lord-Kamina commented 2 years ago

Incidentally, there's also a typo in the USE_EXTERNAL_HTTP_PARSER of dev/restinio/CMakeLists.txt, because it tries to link against http_parser, but the library's name is http-parser

Edit: And nevermind that, because Findhttp-parser.cmake is using the deprecated CMake patterns using variables, so there is not even an http-parser target either.

eao197 commented 2 years ago

Hi!

CMake's related topic is hard part to me because I really hate CMake and almost not understand how it works. We have to support it because it's a de-facto standard at the time and we're trying to do it as best as we can, but shit can happens, sorry.

As far as I can understand our CMake files supports the following case:

you get RESTinio somehow (cloning the repo, for example);
you go to restinio/dev subfolder;
you run cmake for configuring and installing it somewhere;
you go to your project folder and specify a path to installed RESTinio via CMAKE_INSTALL_PREFIX (or something like that).

I suppose that the use of restinio via add_subfolder is also possible but in that case you have to copy restinio/dev/restino subfolder to your project (so you load restinio/dev/restinio/CMakeLists.txt instead of restinio/dev/CMakeLists.txt) and you have to do find_project before you do add_subfolder(restinio).

If those scenarios aren't appicable to you then could you please tell us what do you want to have? Maybe you can point to some good written example of similar CMake file(s) in other OpenSource projects?

Lord-Kamina commented 2 years ago

CMake is a bit like PHP, in that it's not necessarily a bad language and has a million ways to do anything, but it's full of developers who misuse it horribly and stick to ancient patterns. It's very likely that this has contributed to your hatred of it. I don't have a project in mind with particularly good usage of it but I'll ask around and see if I can find something.

I'm willing to eventually making PRs to try and fix at least some of the issues, of course.

Lord-Kamina commented 1 year ago

@eao197 I'm taking a crack at fixing some of the issues and several aspects of RESTinio's CMake scripts. I need you to clarify some stuff for me, though.

How/where exactly are either Catch or Clara included? In the CMake script, I see that using FIND_DEPS, RESTinio is looking for Catch2 and apparently not Clara (although I think Clara is a submodule in Catch2?). When not using FIND_DEPS, You're including Clara, from where MxxRu should have put it, if I understand correctly. But, Catch2 is not being added in the same way?

Also, the externals.rb file lists both fmt @ v9. and fmtlib @5.0. Is that intentional or an error?

Would you mind explaining when other libraries are used and how are they obtained in the various scenarios (some of them are pretty obvious. Others not so much)

eao197 commented 1 year ago

Hi!

How/where exactly are either Catch or Clara included?

It's a hard question. When FIND_DEPS isn't used that means that sources of dependencies are downloaded and placed at the right places by MxxRu::externals. In that case Catch2 and Clara are get as two independent tarballs.

But if FIND_DEPS is used... I don't remember exactly what it going on here. There could be two possibilities:

Clara is a part of Catch2 and there is no need to find Clara;
there is an error in our CMakeLists.txt and we just forgot to find Clara package.

Also, the externals.rb file lists both fmt @ v9. and fmtlib @5.0. Is that intentional or an error?

There are two different projects. The first is fmt (fmtlib):

MxxRu::arch_externals :fmt do |e|
  e.url 'https://github.com/fmtlib/fmt/archive/9.1.0.zip'

and the second is MxxRu wrapper for fmtlib named fmtlib_mxxru:

MxxRu::arch_externals :fmtlib_mxxru do |e|
  e.url 'https://github.com/Stiffstream/fmtlib_mxxru/archive/fmt-5.0.0-1.tar.gz'

RESTinio uses fmtlib-9.1.0 and fmtlib_mxxru version fmt-5.0.0-1. The name of fmtlib_mxxru version means that this version of fmtlib_mxxru can be used with fmtlib v5.0.0 and newer.

Would you mind explaining when other libraries are used and how are they obtained in the various scenarios (some of them are pretty obvious. Others not so much)

You have to understand that the content of root CMakeLists.txt is a result of long evolution process. Initially our idea (and intent) was to get all dependencies for the development of RESTinio via MxxRu::externals. When we fixed a new version of RESTinio we made a tarball with all dependencies. We thought that a user get this tarball, unpack it and add just one directory to INCLUDE path. So the first versions of CMakeLists.txt were very simple because they were created for such simple scenario.

But then we started to get questions like "I'm already have library X in my project how can I tell RESTinio to use it?" So we started to add new conditions to CMakeLists.txt. Moreover, after some time we added a support for Conan and vcpkg. And now our CMakeLists.txt is a mess of workarounds for various cases. It should be refactored and a new strategy for dependency management has to be implemented (there is already a related issue). But because this project is on hold now there is no progress in that area.

Lord-Kamina commented 1 year ago

What I'm currently going for is to do something akin to what is proposed in #160, but I thought perhaps I could also leave FIND_DEPS but repurpose it to just be an initial value for all the USE_EXTERNAL_*

eao197 commented 10 months ago

It seems that this issue is no more actual after release of v.0.7.0 where CMake scripts were refactored. Maybe v.0.7.0 also have some drawbacks, but they have to be discussed in a separate issue.

Stiffstream / restinio

RESTinio CMake issues #167