Allowing CMake projects to include this library as dependency #73
No reviewers
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
need more info
need verification for fix
not so hard
question
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-tplgy/cppcodec!73
Loading…
Reference in a new issue
No description provided.
Delete branch "master"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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).
github.com/quiga/cppcodec@849ccb04a6/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
I have created a test case for this: https://github.com/adi-g15/tmp-pr-fix
main: Using current repoquiga-fix: Using the modified oneSorry if I have got some error in the CMakeLists
github.com/adi-g15/tmp-pr-fix@ad7a33e0aa/CMakeLists.txt (L1-L6)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.
@ -36,0 +24,4 @@# fine for anything except ancient MSVC versions.set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")else()set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")These should probably be
CACHEvariables, right? Then there's a way for the user to set it in the configuration, including viaccmake.@ -99,3 +84,4 @@if (BUILD_EXAMPLES)add_subdirectory(example)endif()Would this also work if declared as
INTERFACElibrary? I think that would be ideal given that this is what cppcodec actually is.@ -118,3 +95,1 @@FILES ${CMAKE_CURRENT_BINARY_DIR}/cppcodec-1.pcDESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/pkgconfig)if (NOT WIN32)Style nitpick: a single
target_include_directories()can contain bothPUBLICandPRIVATEsections, so I think it's cleaner to move thePUBLICinto the next line (so a hypotheticalPRIVATEwouldn't look out of place, not that it matters much here).@ -36,0 +24,4 @@# fine for anything except ancient MSVC versions.set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")else()set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")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)@ -118,3 +95,1 @@FILES ${CMAKE_CURRENT_BINARY_DIR}/cppcodec-1.pcDESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/pkgconfig)if (NOT WIN32)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 😅)
@ -99,3 +84,4 @@if (BUILD_EXAMPLES)add_subdirectory(example)endif()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.
@ -36,0 +24,4 @@# fine for anything except ancient MSVC versions.set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")else()set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")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.
@ -36,0 +24,4 @@# fine for anything except ancient MSVC versions.set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")else()set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")Sorry yeah, I meant
option()(for ON/OFF variables).@ -36,0 +24,4 @@# fine for anything except ancient MSVC versions.set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")else()set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")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
@ -99,3 +84,4 @@if (BUILD_EXAMPLES)add_subdirectory(example)endif()If we can restrict CMake versions >= 3.12, we can leave
OBJECThere as-is, because since version 3.12, object libraries can be linked to withtarget_link_libraries(); and with addedtarget_include_directories(), this library can be consumed simply via FetchContent.Otherwise, you may need to specify
INTERFACEinadd_library()and use target_sources to add source files.and
target_sourcesrequires CMake 3.1, you also must useINTERFACEintarget_include_directories()@ -99,3 +84,4 @@if (BUILD_EXAMPLES)add_subdirectory(example)endif()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.
Pull request closed