-
Notifications
You must be signed in to change notification settings - Fork 118
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
Prefer system-provided robin-map #248
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit more modern CMake style
CMakeLists.txt
Outdated
"fetch the submodule's contents from" | ||
"https://github.com/Tessil/robin-map/") | ||
endif () | ||
find_path(ROBIN_MAP_HEADER NAMES "tsl/robin_map.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_path(ROBIN_MAP_HEADER NAMES "tsl/robin_map.h") | |
find_package(tsl-robin-map) |
It should come with a CMake package. You'll also need to do something with the found file, right? I think ROBIN_MAP_HEADER is unused right now except for checking if it's there.
target_link_libraries(objc PRIVATE tsl::robin_map)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, that works because the headers are installed into /include/
. I've added:
include_directories("${tsl-robin-map_INCLUDE_DIRS}")
but it's redundant in most cases.
The package is a header-only package; there is no library to link against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake models header-only libraries just like other libraries. If you import the target then it adds the relevant flags to find the headers. It also adds any compile flags that the ‘library’ needs.
CMakeLists.txt
Outdated
|
||
include_directories("${CMAKE_SOURCE_DIR}/third_party/robin-map/include/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include_directories("${CMAKE_SOURCE_DIR}/third_party/robin-map/include/") | |
add_library(tsl::robin_map INTERFACE IMPORTED) | |
set_target_properties(tsl::robin_map PROPERTIES | |
INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_SOURCE_DIR}/third_party/robin-map/include/") |
and then it can be linked as above
It would be good to have this in the next release. Do either of you want to incorporate the changes and update the PR? |
I've addressed the feedback (with some comments) and rebased on master. Let me know if there's anything else you need. Thanks! |
selector_table.cc
Outdated
@@ -11,7 +11,7 @@ | |||
#include <vector> | |||
#include <mutex> | |||
#include <forward_list> | |||
#include "third_party/robin-map/include/tsl/robin_set.h" | |||
#include "tsl/robin_set.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "tsl/robin_set.h" | |
#include <tsl/robin_set.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be changed, the header 8s now found via -I. If you turn up warnings a bit, clang will tell you that this is wrong.
CMakeLists.txt
Outdated
"fetch the submodule's contents from" | ||
"https://github.com/Tessil/robin-map/") | ||
endif () | ||
find_path(ROBIN_MAP_HEADER NAMES "tsl/robin_map.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks as it the package provides the correct CMake bits to allow find_package to work, so we should probably look for the package and add it as a dependency (which will also pick up any necessary compiler flags and so on).
CMakeLists.txt
Outdated
find_path(ROBIN_MAP_HEADER NAMES "tsl/robin_map.h") | ||
|
||
if (NOT ROBIN_MAP_HEADER) | ||
if (NOT EXISTS "${CMAKE_SOURCE_DIR}/third_party/robin-map/include/tsl/robin_map.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is now just a fallback, it would be nice to use FetchContent
and grab it if necessary, and remove the submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn’t found, we should use FetchContent to get it and then add it explicitly, then fall through to add_library below.
CMakeLists.txt
Outdated
"fetch the submodule's contents from" | ||
"https://github.com/Tessil/robin-map/") | ||
endif () | ||
find_path(ROBIN_MAP_HEADER NAMES "tsl/robin_map.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake models header-only libraries just like other libraries. If you import the target then it adds the relevant flags to find the headers. It also adds any compile flags that the ‘library’ needs.
@davidchisnall Sorry for the noise. This should be good now. Can you kick off the GitHub CI for this PR, so we get the build signal on all systems? |
Thanks, looks great. |
This reverts commit 32c09c0.
This reverts commit 32c09c0.
This reverts commit 32c09c0.
This reverts commit 32c09c0.
A lot of distributions ship with robin-map in their package management system, so it may not be a hard requirement to have robin-map as a submodule.
This PR updates the build system to search for
tsl/robin_map.h
and use that one if available; if not, fall back to the submodule.