-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Fix cmake installation failed (Fix #13578) #14692
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.
LGTM, tested locally and works.
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 change looks good, but we are installing some garbage that I don't think should go on the install directory:
-- Installing: /usr/local/lib/cmake/dmlc/DMLCTargets.cmake
-- Installing: /usr/local/lib/cmake/dmlc/DMLCTargets-debug.cmake
...
-- Installing: /usr/local/./doc
-- Installing: /usr/local/./doc/index.md
-- Installing: /usr/local/./doc/conf.py
-- Installing: /usr/local/./doc/parameter.md
-- Installing: /usr/local/./doc/sphinx_util.py
-- Installing: /usr/local/./doc/Doxyfile
-- Installing: /usr/local/./doc/README
-- Installing: /usr/local/./doc/Makefile
-- Installing: /usr/local/./doc/.gitignore
-- Installing: /usr/local/./doc/build.md
-- Installing: /usr/local/lib/cmake/dmlc/dmlc-config.cmake
-- Installing: /usr/local/lib/cmake/dmlc/dmlc-config-version.cmake
Can we get rid of that?
super ugly quick workground
but this sould be fixed in upstream see /~https://github.com/dmlc/dmlc-core/blob/master/CMakeLists.txt#L216 the /~https://github.com/dmlc/dmlc-core/blob/master/CMakeLists.txt#L215 also sould be fixed, but is less problematic because is installed in the "correct" path ( the latest one seems redundant because /~https://github.com/dmlc/dmlc-core/blob/master/CMakeLists.txt#L195-L201, but the option in resume: /~https://github.com/dmlc/dmlc-core/blob/master/CMakeLists.txt#L216 i think, should be configurable by option or use ive try to make a PR with this changes in the dmlc-core, but idk when is merged/reviewed greetings something like: diff --git a/CMakeLists.txt b/CMakeLists.txt
index 91f42fc..84fc0a3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -19,6 +19,7 @@ dmlccore_option(USE_S3 "Build with S3 support" OFF)
dmlccore_option(USE_OPENMP "Build with OpenMP" ON)
dmlccore_option(USE_CXX14_IF_AVAILABLE "Build with C++14 if the compiler supports it" OFF)
dmlccore_option(GOOGLE_TEST "Build google tests" OFF)
+dmlccore_option(INSTALL_DOCUMENTATION "Install documentation" OFF)
# include path
set(INCLUDE_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/include")
@@ -192,16 +193,12 @@ target_include_directories(dmlc PUBLIC
target_compile_definitions(dmlc PRIVATE -D_XOPEN_SOURCE=700
-D_POSIX_SOURCE -D_POSIX_C_SOURCE=200809L -D_DARWIN_C_SOURCE)
+include(GNUInstallDirs)
# ---[ Install Includes
-if(INSTALL_INCLUDE_DIR)
- add_custom_command(TARGET dmlc POST_BUILD
- COMMAND ${CMAKE_COMMAND} -E copy_directory
- ${PROJECT_SOURCE_DIR}/include ${INSTALL_INCLUDE_DIR}/
- )
-endif()
+install(DIRECTORY ${PROJECT_SOURCE_DIR}/include/dmlc
+ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
# ---[ Install the archive static lib and header files
-include(GNUInstallDirs)
install(TARGETS dmlc
EXPORT DMLCTargets
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
@@ -212,8 +209,10 @@ install(EXPORT DMLCTargets
EXPORT_LINK_INTERFACE_LIBRARIES
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/dmlc)
-install(DIRECTORY include DESTINATION .)
-install(DIRECTORY doc DESTINATION .)
+# ---[ Install documentation
+if(INSTALL_DOCUMENTATION)
+ install(DIRECTORY doc DESTINATION ${CMAKE_INSTALL_DATADIR})
+endif()
# ---[ Package configurations
include(CMakePackageConfigHelpers) |
pushed PR to upstream (dmlc/dmlc-core#528) |
I tried both PRs together and I have errors:
|
LGTM, the issues listed are related to dmlc-core not to mxnet. I suggest we merge this and follow up on posterior PRs as it improves the current state. |
@mxnet-label-bot update [build,cmake,pr-awaiting-merge] |
Merged. Thanks for your contribution! |
Description
fix installation when use cmake
#13578
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.