Skip to content

Conversation

@bugdea1er
Copy link
Contributor

@bugdea1er bugdea1er commented Dec 25, 2023

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

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

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.

@bugdea1er bugdea1er changed the title Build tests only if master project by default Build examples only if master project by default Dec 27, 2023
prince-chrismc added a commit to prince-chrismc/jwt-cpp that referenced this pull request Dec 27, 2023
@prince-chrismc
Copy link
Collaborator

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

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 ca110ad#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://google.github.io/googletest/quickstart-cmake.html
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
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.

@bugdea1er bugdea1er closed this by deleting the head repository Dec 31, 2023
prince-chrismc added a commit that referenced this pull request Jan 10, 2024
* Create getting-started.md

* Update getting-started.md

* Create header-only.yml

* Create package-managers.yml

* Update package-managers.yml

* Update getting-started.md

* Update getting-started.md

* Update header-only.yml

* Update header-only.yml

* Update header-only.yml

* Update header-only.yml

* use `jwt::algorithm::hs256`

 to link against openssl

* add crypto

* switch to ssl

* Update header-only.yml

* tidy

* lets install conan

* tidying

* Adding a CMake target to make the docs

* cleaning up repo root folder

* fix typo

* set docs flag as on

* update awesome doxy theme to latest

* Updated overrides for new version

* output folder and target name are changed

* add a few more docs

* white space

* cmake format

* playing with settings

* move algorithms - magically fixed missing structs

* playing with the idea of more refactoring

* more testing

* moved evp_handle def to special namespace for doxygen

* split evp class by it's self

* fix doc links

* playing with evp class to fix error -- no luck

* revert testing changes

* clang format

* cache should be container specific

* clean up includes

* combined class definition

* split algos but running in ci with older version

* trying slightly newer version of doxygen

* doxygen 196

* doxygen 197

* doxygen 198

* avoid regression in 198

* more docs

* playing with adding examples

* playing around with example support to see if I like

* docs include all the example files to be referenced in the code

* bump version to 0.7.0

* missing header

* devcontainer for testing openssl 3

* add test explorer

* add clang helpers

* there's a lot of package managers in 2023

* fixup colors with doxygen 1.10

* bump doxygen version

since my bug has been fixed

* move everything to one file (too many changes)

* update doxyfile to 1.10

* minimize diff

* fix extra line

* fix whitespace

* clean up dev container

* min changes

* cleanup

* better checking + warnings for doxygen

* whitespace

* adding in more docs

* fix spelling

* make sure traits appear

* trying to add snippets from examples

cant figure it out
doxygen/doxygen#10517

* Update faqs.md

* Delete .github/workflows/header-only.yml

* Delete .github/workflows/package-managers.yml

* Delete example/conan/CMakeLists.txt

* Delete example/conan/conanfile.txt

* Delete example/conan/main.cpp

* Delete example/conan/README.md

* Update nlohmann-json.cpp

* add updated getting started docs

first pass towards #319

* touch ups

* linking deprecated messages

* improve cmake with find_package examples

* fix code link color

* be more clear about turning off examples

good suggestion

#321 (comment)

* clarify openssl install and fixup fetch tag

* apply workaround from

doxygen/doxygen#10517 (comment)

* fix renamed types

* linting

* cross reference new docs

* trying new alert syntax

https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts

* move sections out of readme

trying to make it shorter and more approachable

* improve "getting started" readme section with a more through example

* sort out doubled "getting started"

second one in docs/ is just installation

* linter

* fixup english

* formatting

* whitespace

* tie in more examples with snippets

* minor touch ups

* Update signing.md

* JWT_BUILD_DOCS

* better cache string for new option

* word crafting

* word crafting

* add missing open-source-parsers/jsoncpp to traits list

* better github alert

* fixup new shields io start style

* improve docs cmake target name

* linter

---------

Co-authored-by: Christopher McArthur <christopherm@jfrog.com>
prince-chrismc added a commit to prince-chrismc/jwt-cpp that referenced this pull request Jan 11, 2024
* Create getting-started.md

* Update getting-started.md

* Create header-only.yml

* Create package-managers.yml

* Update package-managers.yml

* Update getting-started.md

* Update getting-started.md

* Update header-only.yml

* Update header-only.yml

* Update header-only.yml

* Update header-only.yml

* use `jwt::algorithm::hs256`

 to link against openssl

* add crypto

* switch to ssl

* Update header-only.yml

* tidy

* lets install conan

* tidying

* Adding a CMake target to make the docs

* cleaning up repo root folder

* fix typo

* set docs flag as on

* update awesome doxy theme to latest

* Updated overrides for new version

* output folder and target name are changed

* add a few more docs

* white space

* cmake format

* playing with settings

* move algorithms - magically fixed missing structs

* playing with the idea of more refactoring

* more testing

* moved evp_handle def to special namespace for doxygen

* split evp class by it's self

* fix doc links

* playing with evp class to fix error -- no luck

* revert testing changes

* clang format

* cache should be container specific

* clean up includes

* combined class definition

* split algos but running in ci with older version

* trying slightly newer version of doxygen

* doxygen 196

* doxygen 197

* doxygen 198

* avoid regression in 198

* more docs

* playing with adding examples

* playing around with example support to see if I like

* docs include all the example files to be referenced in the code

* bump version to 0.7.0

* missing header

* devcontainer for testing openssl 3

* add test explorer

* add clang helpers

* there's a lot of package managers in 2023

* fixup colors with doxygen 1.10

* bump doxygen version

since my bug has been fixed

* move everything to one file (too many changes)

* update doxyfile to 1.10

* minimize diff

* fix extra line

* fix whitespace

* clean up dev container

* min changes

* cleanup

* better checking + warnings for doxygen

* whitespace

* adding in more docs

* fix spelling

* make sure traits appear

* trying to add snippets from examples

cant figure it out
doxygen/doxygen#10517

* Update faqs.md

* Delete .github/workflows/header-only.yml

* Delete .github/workflows/package-managers.yml

* Delete example/conan/CMakeLists.txt

* Delete example/conan/conanfile.txt

* Delete example/conan/main.cpp

* Delete example/conan/README.md

* Update nlohmann-json.cpp

* add updated getting started docs

first pass towards Thalhammer#319

* touch ups

* linking deprecated messages

* improve cmake with find_package examples

* fix code link color

* be more clear about turning off examples

good suggestion

Thalhammer#321 (comment)

* clarify openssl install and fixup fetch tag

* apply workaround from

doxygen/doxygen#10517 (comment)

* fix renamed types

* linting

* cross reference new docs

* trying new alert syntax

https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts

* move sections out of readme

trying to make it shorter and more approachable

* improve "getting started" readme section with a more through example

* sort out doubled "getting started"

second one in docs/ is just installation

* linter

* fixup english

* formatting

* whitespace

* tie in more examples with snippets

* minor touch ups

* Update signing.md

* JWT_BUILD_DOCS

* better cache string for new option

* word crafting

* word crafting

* add missing open-source-parsers/jsoncpp to traits list

* better github alert

* fixup new shields io start style

* improve docs cmake target name

* linter

---------

Co-authored-by: Christopher McArthur <christopherm@jfrog.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants