Skip to content

Modernize cmake - #249

Merged
cameron314 merged 1 commit into
cameron314:masterfrom
miketsukerman:export-cmake-targets
Jun 6, 2021
Merged

cameron314 merged 1 commit into
cameron314:masterfrom
miketsukerman:export-cmake-targets

Conversation

@miketsukerman

@miketsukerman miketsukerman commented May 31, 2021 •

Copy link
Copy Markdown
Contributor

Properly export cmake targets in order to consume library in other cmake projects

find_package(concurrentqueue REQUIRED)

add_library(test test.cpp)
target_link_libraries(test  PRIVATE  concurrentqueue::concurrentqueue)

properly export targets
@cameron314

Copy link
Copy Markdown
Owner

I will merge this if it's backwards compatible with how the previous CMake file was being used (would prefer not to break anyone's existing workflow).

Not to disparage your PR, but this is a perfect example of why I dislike CMake (and C++ packagers in general) :-) 74 lines of configuration + auto-generated inputs, all just to copy four files into a folder.

@miketsukerman

Copy link
Copy Markdown
Contributor Author

I think at the moment your cmake script does export any targets. So, I assume, no one consumes your library using cmake at the moment except maybe ones who use conan package + cmake_find_package generator, but it would not collide imho.

Not to disparage your PR, but this is a perfect example of why I dislike CMake (and C++ packagers in general) :-) 74 lines of configuration + auto-generated inputs, all just to copy four files into a folder.

I can reduce the number of lines, if you like :) On the other hand, it all depends on the size of the project and number of dependencies you have. If you have a lot of libraries to use or support different versions, you might start to love packaging ;)

Up to you, anyways, I made this for myself. Just wanted to share if it might help someone

@cameron314

Copy link
Copy Markdown
Owner

Fair enough. I'll merge it as-is. Thanks for the contribution!

@cameron314
cameron314 merged commit b86db7c into cameron314:master Jun 6, 2021
Comment thread CMakeLists.txt
lightweightsemaphore.h
LICENSE.md
DESTINATION
${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}/moodycamel

@dg0yt dg0yt Aug 2, 2025 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently reviewing the vcpkg port.

I will merge this if it's backwards compatible with how the previous CMake file was being used (would prefer not to break anyone's existing workflow).

This PR uses $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}/> and changes the header install location
from ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}
to ${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}/moodycamel.

This is/was a breaking change: The documented

#include "concurrentqueue.h"

doesn't work because the header now lives in moodycamel/concurrentqueue.h.

What is the desired fix:
a) Revert the change to the destination directory.
b) Change the exported interface include directory.
c) Change the documentation.

I prefer a).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's been 4 years, I'm going to go with (c) for this one

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With moodycamel/concurrentqueue.h, it is now difficult to establish convergence between installed config and sub-project usage. Mitigation would require moving the header in this repo to moodycamel subdir.

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.

3 participants