-
Notifications
You must be signed in to change notification settings - Fork 179
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
Improve FindMagic_EP to support system installs #3270
Improve FindMagic_EP to support system installs #3270
Conversation
This pull request has been linked to Shortcut Story #17869: Generalise cmake setup for libmagic to allow existing libraries and binary files to be used. |
782f922
to
3a34665
Compare
Builds cleanly (and more quickly, no
and trying to use the library (from R) then fails in
|
cmake/Modules/FindMagic_EP.cmake
Outdated
REQUIRED_VARS libmagic_LIBRARIES libmagic_INCLUDE_DIR | ||
) | ||
NAMES | ||
bz2 libbz2 |
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.
Should this be magic libmagic
?
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.
Apparently. If I make the change the core library still builds, emits the correct message, and is loadable from R. I'll make the change to the branch and we'll see what CI does.
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.
Console log from the build:
-- Found libmagic: /usr/lib/x86_64-linux-gnu/libmagic.so
-- Found Magic, adding imported target: /usr/lib/x86_64-linux-gnu/libmagic.so
09c0112
to
aa9afd3
Compare
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.
The suggested change in a commit I made earlier (and which was now force-pushed over) is needed to actually pick the identified system library up:
modified cmake/Modules/FindMagic_EP.cmake
@@ -57,7 +57,7 @@ elseif(NOT TILEDB_FORCE_ALL_DEPS)
# Static EP not found, search in system paths.
find_library(libmagic_LIBRARIES
NAMES
- bz2 libbz2
+ magic libmagic
PATH_SUFFIXES lib bin
${TILEDB_DEPS_NO_DEFAULT_PATH}
)
86cd63c
to
877bbda
Compare
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.
Attempt local windows build yielded error
CMake Error at cmake/Modules/FindMagic_EP.cmake:132 (message):
Unable to find libmagic dictionary
Call Stack (most recent call first):
cmake/TileDB-Superbuild.cmake:89 (include)
CMakeLists.txt:187 (include)
and I was wondering, but can't get far enuf because of above, if the new location of 'bin' has to be provided to the unit_mgc_dict target items as well...
cmake/Modules/FindMagic_EP.cmake
Outdated
libmagic${CMAKE_STATIC_LIBRARY_SUFFIX} | ||
libmagic${CMAKE_STATIC_LIBRARY_SUFFIX} |
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.
Does having this twice do something special?
877bbda
to
cd771c7
Compare
This improves the support for detection of system installations with libmagic. We remove checking for the find_package and default to always looking for the library and header files for simplicity.
cd771c7
to
bc235a6
Compare
failed again after pull of recent changes,
|
This improves the support for detection of system installations with libmagic. We remove checking for the find_package and default to always looking for the library and header files for simplicity.
TYPE: IMPROVEMENT
DESC: Improve FindMagic_EP to support system installs of libmagic