Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
olifre opened this issue Feb 26, 2022 · 9 comments
Open

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

olifre opened this issue Feb 26, 2022 · 9 comments
Assignees
Labels
Milestone

Comments

@olifre
Copy link

olifre commented Feb 26, 2022

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):

  • OS: Gentoo Linux with picojson installed from upstream via system package dev-cpp/picojson-
  • Compiler: gcc (Gentoo 11.2.0 p1) 11.2.0
@prince-chrismc prince-chrismc self-assigned this Feb 26, 2022
@prince-chrismc
Copy link
Collaborator

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
Copy link
Owner

Thalhammer commented Feb 26, 2022

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
Copy link
Author

olifre commented Feb 26, 2022

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

  • Make JWT_EXTERNAL_PICOJSON a bit more flexible, i.e. in case it is set, allow the user to specify a PICOJSON_INCLUDE_PATH from outside instead of find_package.

  • Embed that path (which could default to picojson/ so it works with the bundled version) into the include statements. That could be done via a preprocessor macro and a header config file created by CMake for example.

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
Copy link
Collaborator

prince-chrismc commented Feb 26, 2022

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 😆

@prince-chrismc prince-chrismc added this to the 0.7.0 milestone Feb 26, 2022
@olifre
Copy link
Author

olifre commented Feb 26, 2022

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 👍 .

@prince-chrismc
Copy link
Collaborator

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
Copy link
Owner

Thalhammer commented Feb 26, 2022

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
Copy link
Collaborator

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

@prince-chrismc prince-chrismc changed the title [bug] Unable to use picojson installed to /usr/include/picojson.h Unable to use picojson installed to /usr/include/picojson.h Mar 14, 2022
@csegarragonz
Copy link
Contributor

csegarragonz commented Apr 4, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants