Allowing CMake projects to include this library as dependency #73
Conversation
|
I have created a test case for this: https://github.com/adi-g15/tmp-pr-fix
Sorry if I have got some error in the CMakeLists |
|
Thanks! I haven't looked at this in a long time and recently got some "more modern" CMake exposure at work, so overall I'd like to start with saying thanks and I know that this stuff needs a lot of work. This PR is a good start, line comments to follow. |
| @@ -15,6 +14,10 @@ endif() | |||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |||
| set(CMAKE_CXX_EXTENSIONS OFF) | |||
|
|
|||
| set(BUILD_TESTS OFF) | |||
| set(BUILD_EXAMPLES OFF) | |||
| set(BUILD_TOOLS OFF) | |||
There was a problem hiding this comment.
These should probably be CACHE variables, right? Then there's a way for the user to set it in the configuration, including via ccmake.
There was a problem hiding this comment.
Yes, that should be a CACHE variable, didn't notice earlier... maybe with option(BUILD_TESTS "Build bundled tests" OFF) (https://stackoverflow.com/q/36358217/12339402)
There was a problem hiding this comment.
And, btw, these changes were made by @quiga, so the credit goes to him. I just found it made it easier to include so created this PR upstream.
There was a problem hiding this comment.
Sorry yeah, I meant option() (for ON/OFF variables).
There was a problem hiding this comment.
Also, just to say, I can't edit the code, I created the PR from quiga's branch (so can't edit that code), maybe you being a maintainer can make changes in the PR's changes :D
|
|
||
| if (BUILD_TESTING) | ||
| target_include_directories(cppcodec PUBLIC |
There was a problem hiding this comment.
Style nitpick: a single target_include_directories() can contain both PUBLIC and PRIVATE sections, so I think it's cleaner to move the PUBLIC into the next line (so a hypothetical PRIVATE wouldn't look out of place, not that it matters much here).
There was a problem hiding this comment.
Yes (I don't know much, I simply separate out each target_include_directories in my projects, I checked the docs to make sure what are the optional args 😅)
| @@ -64,7 +64,7 @@ set(PUBLIC_HEADERS | |||
| cppcodec/detail/hex.hpp | |||
| cppcodec/detail/stream_codec.hpp) | |||
|
|
|||
| add_library(cppcodec OBJECT ${PUBLIC_HEADERS}) # unnecessary for building, but makes headers show up in IDEs | |||
| add_library(cppcodec ${PUBLIC_HEADERS}) # unnecessary for building, but makes headers show up in IDEs | |||
There was a problem hiding this comment.
Would this also work if declared as INTERFACE library? I think that would be ideal given that this is what cppcodec actually is.
There was a problem hiding this comment.
Sorry, I don't know this one too, it didn't work if the OBJECT is just replaced with INTERFACE as it doesn't take source arguments. I use INTERFACE only for header only libs of mine, this one requires .cpp files too, so i don't know how that works for INTERFACE.
There was a problem hiding this comment.
If we can restrict CMake versions >= 3.12, we can leave OBJECT here as-is, because since version 3.12, object libraries can be linked to with target_link_libraries(); and with added target_include_directories(), this library can be consumed simply via FetchContent.
Otherwise, you may need to specify INTERFACE in add_library() and use target_sources to add source files.
and target_sources requires CMake 3.1, you also must use INTERFACE in target_include_directories()
There was a problem hiding this comment.
Fyi, maybe we can use nlohmann/json's CMakeLists.txt as a reference impl
|
Hi everyone, I took everyone's contributions to heart and finally uploaded PR #76, hopefully containing the best version of what was started on this one here. If you have time, please have a look and let me know if that works for you (or any other suggestions). Closing this PR in favor of the other one. |
Currently, if we add it as submodule, or with CPM (which I did), we can't include the
<cppcodec/...>headers.It fails with this error, even after including cppcodec as a library dependency:
fatal error: cppcodec/base64_rfc4648.hpp: No such file or directory
@quiga has already fixed it (https://github.com/quiga/cppcodec).
https://github.com/quiga/cppcodec/blob/849ccb04a6f75aa300f6d6d6c8d163aca5e1e936/CMakeLists.txt#L70-L73
I used his fork as an upstream and it worked, there are changes mainly in CMakeLists you can see the diff. So, I created this PR so that the fix is merged with upstream :D