-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CMakeLists INSTALL DESTINATION and INSTALL_INTERFACE are inconsistent #384
Comments
I regret accepting PRs for CMake support, it seems to cause no end of issues for what is a header-only library. It seems like if I change this, it would break code currently including |
I appreciate it being there. It has proven itself useful for me in a couple projects already. Nothing that I probably couldn't do with some custom CMake code but when I'm pulling in 5+ 3rd party dependencies, it's nice when most of them just need a
Are you concerned about overwriting in the system Or are you concerned about shadowing I'm hoping we can achieve syntax parity among the CMake-installed workflow and the clone-and-include workflow. With the proposed changes, the syntax for including concurrentqueue for CMake-installed users is aligned with the non-cmake workflow. I'd also be happy with |
@cameron314 , I'd be happy to put forth a PR, but I'm hoping you can help me decide on the preferred approach, if any. AFAIK, there are 3 main workflows to use concurrentqueue in a project:
Admittedly, the first workflow is far more common than the others, and that is totally appropriate because it's a header-only library (except C-API users). I would like to use the 3rd workflow for a specific project where automated auditing of dependencies is important and is much easier to do by installing it (as a yocto layer, in case you are curious). Unfortunately, the 3rd workflow requires different include statements to the others, which is a bit tricky for documentation and for people like me who want to share any concurrentqueue-using code between build workflows. The different include statements are:
Is it desirable to you to unify the include statements across these different workflows?
My preferred is the last or 2nd-last, but that would require that the header files are placed one folder deeper in the repository (e.g., For now, I can solve my problem by patching concurrentqueue when I pull it, or manually adding the |
I agree that the include paths should be consistent.
Installing into non-namespaced directories can create conflicts. What if someone else writes a Hence, my preferred installation structure would be Thanks for taking the time to look into this, and for bearing with me through my slow response times. |
edit: temporary user side solution: use cmake's cmake file: if(NOT TARGET concurrentqueue::concurrentqueue)
add_library(concurrentqueue::concurrentqueue ALIAS concurrentqueue)
endif()
# after cmake 3.29: Alias targets to imported libraries are also supported in try_compile.
# add these code to compatible with cmake<3.29
get_property(aliased_target TARGET concurrentqueue::concurrentqueue PROPERTY ALIASED_TARGET)
if(aliased_target)
set(concurrentqueue_link_name concurrentqueue)
else()
set(concurrentqueue_link_name concurrentqueue::concurrentqueue)
endif()
try_compile(
include_moodycamel_concurrentqueue
SOURCES ${PROJECT_SOURCE_DIR}/src/try_compile/test_include_moodycamel_concurrentqueue.cc
LINK_LIBRARIES ${concurrentqueue_link_name})
# since `try_compile` only works for imported target, if the target is added by `add_library` or `add_subdirectory`,
# the `try_compile` will always fail. since the `concurrentqueue` target always exists, we just assume the header
# is included as `#include <concurrentqueue.h>`
if(NOT include_moodycamel_concurrentqueue)
set(include_concurrentqueue true)
endif()
configure_file("include_concurrentqueue.h.in" "${CMAKE_BINARY_DIR}/configured_files/include/internal_use_only/include_concurrentqueue.h" ESCAPE_QUOTES) include_concurrentqueue.h.in #pragma once
// clang-format off
#cmakedefine include_concurrentqueue
#cmakedefine include_moodycamel_concurrentqueue
// clang-format on
#ifdef include_concurrentqueue
#include <concurrentqueue.h>
#elifdef include_moodycamel_concurrentqueue
#include <moodycamel/concurrentqueue.h>
#else
#include <concurrentqueue/concurrentqueue.h>
#warning "try to include system concurrentqueue"
#endif test_include_moodycamel_concurrentqueue.cc #include <moodycamel/concurrentqueue.h>
int main() {} then add |
Previously, I had been grabbing concurrentqueue with cmake's
FetchContent
and it worked great. Recently, I switched to using it as a bitbake recipe in an embedded system, where concurrentqueue gets installed as a system-level dependency. I suddenly had to change my includes to#include <moodycamel/concurrentqueue.h>
. And of course this fails when I want to build without the system-level install for local debugging without the embedded sysroot.I think the problem is simple:
concurrentqueue/CMakeLists.txt
Lines 9 to 13 in 189e381
and
concurrentqueue/CMakeLists.txt
Lines 54 to 62 in 189e381
should match.
I propose removing the trailing
moodycamel
from the install destination.That way, all the include statements in the source code will be the same (
#include <concurrentqueue.h>
) whether someone grabs concurrentqueue with FetchContent or they install concurrentqueue to the system and usefind_package(concurrentqueue)
followed bytarget_link_libraries(${PROJECT_NAME} concurrentqueue::concurrentqueue)
.Currently, I'm patching concurrentqueue on fetch with this exact change and it's working well for me.
The text was updated successfully, but these errors were encountered: