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
828 stars 229 forks source link

Build examples only if master project by default #321

Closed bugdea1er closed 6 months ago

bugdea1er commented 6 months ago

Hi there!

I changed the default value of JWT_BUILD_EXAMPLES to true iff jwt is the main project.

We are using jwt-cpp as a submodule, and although we have manually disabled this option, it is strange to build examples in a case with a submodule where this is not explicitly set.

What do you think?

prince-chrismc commented 6 months ago

Personally not inclined to this, as you mentioned it was easy for you to set this on your end. My reasoning is users like #319 which might not know so it best to have examples compiled by default to help find problems.

Maybe if it was a single line or was more consistent in the ecosystem I would be swayed. This doesn't help for package managers either so it's a really slim user.

Open to my mind being changed!

bugdea1er commented 6 months ago

Honestly, I don't think a new user will need to compile the examples to understand how to use this library. Source codes usually help with this.

Maybe at least add a warning about this in the README? I mean instructions on how to disable example compilation?

This won't help people who know about it (like our team), it can only help new users.

prince-chrismc commented 6 months ago

Honestly, I don't think a new user will need to compile the examples to understand how to use this library.

Ahh, I meant specific that it can build, because without anything to compile (being a header only project) its hard to know if the setup was correct. By having examples enabled by default, comments like this..

I've tried including the jwt-cpp repo under my project directory and tried building it using cmake (the build seemed to work

..have a good amount, of information. Now that's not to say a CMake novice might be using the wrong words. My hope is that by making new users build them we know the system in properly configured (which is very often a pain point).

Hopefully that clarifies my last remarks 😄

prince-chrismc commented 6 months ago

Sure, but using add_subdirectory and fetchcontent are not "officially" supported and there are no tests for them. My hesitation is looking at supporting these the CMake CI will just grow unreasonably from the POV of the maintainer (aka me). Now https://github.com/Thalhammer/jwt-cpp/commit/ca110ade5bf70920175bd6679c39e2c0f79239d7#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a I did change tests so I should be more open to examples. There are very few drawbacks to this suggestion. GTest also recommends override settings https://github.com/Thalhammer/jwt-cpp/blob/953aab6fe47be9e92fcbf5fb8418dee0d682e7ca/tests/CMakeLists.txt#L21-L22 so this is an option.

I do think improving the documentation in this area is super important and I've included your suggestion as I've been working on this.

image

However looking at my copy of Professional CMake 16th edition, I think I've talked myself in this is a good change, I am going to poke some friends and colleagues to get more opinions.