Make library target visible to downstream clients #64
Conversation
This will allow outside packages, e.g. through FetchContent, to consume the library and find the header files using `#include`.
|
Thank you for your contribution. CMake support is a contribution of @RedSkittleFox. I'd like him to to review the change and let us know whether it should be merged as is or if any adjustments are needed. |
|
Hey @yaneury If you do: FetchContent_Declare(
atomic_queue
GIT_REPOSITORY https://github.com/max0x7ba/atomic_queue.git
GIT_TAG 10ae35d40449b07607238c3809d360765a8cff91 # Latest commit tag
)
FetchContent_MakeAvailable(atomic_queue)
target_link_libraries(${PROJECT_NAME} PRIVATE max0x7ba::atomic_queue) # use aliased targetthen it should work just fine. |
| @@ -26,5 +26,3 @@ endif() | |||
| if ( ATOMIC_QUEUE_BUILD_TESTS OR ATOMIC_QUEUE_BUILD_EXAMPLES) | |||
| add_subdirectory( src ) | |||
| endif() | |||
|
|
|||
| add_library(max0x7ba::atomic_queue ALIAS atomic_queue) | |||
There was a problem hiding this comment.
I don't understand this change.
There was a problem hiding this comment.
I thought the name was clobbering with the library target defined inside include/CMakeLists.txt. But I wasn't sure...
| @@ -11,7 +13,8 @@ add_library( | |||
| ) | |||
|
|
|||
| target_include_directories( | |||
| atomic_queue | |||
| ${PROJECT_NAME} | |||
There was a problem hiding this comment.
An explicit target name should be used here (as it was being used before), we should not assume that ${PROJECT_NAME} is the same as the target name passed to add_library.
| INTERFACE | ||
| ${CMAKE_CURRENT_SOURCE_DIR} | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> | ||
| $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> |
There was a problem hiding this comment.
Okay, but we don't have install(...) set up for this project. If we want to do this then we need to provide the installation config as well.
|
The tag has been created now. Should anything from this change be merged? |
This PR can be closed without merging. |
|
@yaneury Would you like to close this PR if your issue is resolved, please? |
|
Just tested the updated tag and it works now. Thanks for your help! |
Using the library with
FetchContentsuccessfully downloaded the library onto thebuild/_depsdirectory but all the#includestatements yielded a compiler error because CMake wasn't includingatomic_queuedirectory for the target. You can test this on this repo likeso:And link to an executable that includes the library. It should fail to compile.
If you instead link against my fork's version, it will work: