search

Thalhammer / jwt-cpp

A header only library for creating and validating json web tokens in c++
https://thalhammer.github.io/jwt-cpp/
MIT License
864 stars 235 forks source link

Unable to use picojson installed to `/usr/include/picojson.h` #213

Open olifre opened 2 years ago

olifre commented 2 years ago

Describe the bug picojson defaults to installation into <prefix>/include/picojson.h. However, jwt-cpp tries to include from <includepath>/picojson/picojson.h which will not be used by a standard picojson installation.

Desktop (please complete the following information):

prince-chrismc commented 2 years ago

You are absolutely correct, https://github.com/kazuho/picojson/blob/111c9be5188f7350c2eac9ddaedd8cca3d7bf394/Makefile#L21

Do you have any suggestions on how we can support both?

Thalhammer commented 2 years ago

C++17 introduced __has_include to check if an include path is valid. For everything before that the best option is probably a preprocessor macro to switch between both, which could be autodetected if cmake is used or user defined if not.

Looking at the git blame, it seems like it originally included picojson without any prefix, but that got changed when we added vcpkg support, so we probably have to make sure we handle that case correctly.

olifre commented 2 years ago

I believe the most flexible approach would be (but that needs some more changes):

That also fixes a second issue I have: as-is, I can't specify JWT_EXTERNAL_PICOJSON even though I have an external picojson, since picojson itself does not ship any CMake files which find_package would look for.

prince-chrismc commented 2 years ago

I think the idea was that the consumer would provide their own find_package(picojson) module, which could look up the the header for their system.

Perhaps that's a solution 🤔 jwt-cpp could provide a custom CMake module that searches both locations and setups the target correctly? You can write something similar to https://github.com/Thalhammer/jwt-cpp/blob/master/cmake/private-find-boost-json.cmake. I'd be glad to help 😉

I am not sure we can do that just yet... Would probably break Conan though 🤢 No clue about hunter or vcpkg

There's a handful of action items to fix this (sadly it's not trivial).

  1. Test build with the three package managers
  2. interim solution might be the PICOJSON_INCLUDE_PATH and some CI tests for that workflow
  3. Docs explaining how to set it up 😆
olifre commented 2 years ago

jwt-cpp could provide a custom CMake module that searches both locations and setups the target correctly?

That sounds like a good solution :-).

My experience with hunter, conan or vcpkg is sadly zero, I have first heard of these in this project. As a quick hack, I have for now used the following approach in Gentoo packaging: https://github.com/gentoo/guru/blob/47471944d857275e82983460b6cd720257510df7/dev-cpp/jwt-cpp/jwt-cpp-0.6.0.ebuild#L31-L38

I'm not sure I'll have sufficient time for a PR in the near future, but I believe the approach is a good one :+1: .

prince-chrismc commented 2 years ago

I have first heard of [hunter, conan or vcpkg is] in this project

The lack of a C++ ecosystem is to blame for that 😭 hopefully you next project can try out the package manager landscape

I think the patch is good for packing, I did the same for Conan https://github.com/conan-io/conan-center-index/tree/master/recipes/jwt-cpp/all/patches 👍

Hopefully in the near future we can make a point release to improve that 🤞

Thalhammer commented 2 years ago

The lack of a C++ ecosystem is to blame for that

More like the abundance of the same. Unlike most "cool new" programming languages, which have a single established build system and package manager/repository, c++ has tons of projects all trying to solve more or less the same problem (and all of them failing in different ways). I can come up with with 7 different build systems (cmake, scons, msvc, ninja, make, premake), 4 different package managers (hunter, conan, vcpkg, deb/rpm/pkg/whatevermicrosoftisdoing) and 3 major compilers in 9 variations (clang with libstdc++, libc++, musl, gcc with libstdc++, libc++, musl, msvc, intel c++, arm cc). And those are just the ones I can remember of the top of my head, I guarantee there are tons more. Some of that is useful (different compilers catch diffferent bugs, though one could argue its not a bug if the compilers all agree on the ?wrong? result), while others (like the abundance of package type) is just plain annoying. Not to mention that soon:tm: we will have to deal with the difference of c++ modules in addiftion to the current state, making it even more fragmented. I personally really like hunter since it is (or at least claims to be) crossplatform and solves many issues I had with others by building all the packages from scratch on your machine with the current compiler and build flags (including debug symbols) but thats just my personal take.

prince-chrismc commented 2 years ago

Note for whoever gets to this first, #195 is closely related and we should write some docs to help the community

csegarragonz commented 2 years ago

Hi,

I am installing the library through Conan with version and revision (both latest):

jwt-cpp/0.6.0@#cd6b5c1318b29f4becaf807b23f7bb44

After installing, I ran into a very similar error where picojson.h is not copied in, and thus no where to be found.

Am I missing something in the Conan installation? I.e. is there any CMake variable I have to set to explicitely install the default picojson.h header?