mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [mesos] greggomann commented on a change in pull request #363: CMake enable install module dependencies.
Date Thu, 14 May 2020 01:30:28 GMT

greggomann commented on a change in pull request #363:
URL: https://github.com/apache/mesos/pull/363#discussion_r424821122



##########
File path: 3rdparty/CMakeLists.txt
##########
@@ -512,6 +523,11 @@ ExternalProject_Add(
   URL               ${RAPIDJSON_URL}
   URL_HASH          ${RAPIDJSON_HASH})
 
+if (ENABLE_INSTALL_MODULE_DEPENDENCIES)
+  install(
+    DIRECTORY ${RAPIDJSON_CMAKE_ROOT}/src/rapidjson-${RAPIDJSON_VERSION}/include
+    DESTINATION .)

Review comment:
       Why are there several different install directories for headers: the root installation
directory is used here, others use `MESOS_INSTALL_HEADERS`, while glog uses `${MESOS_INSTALL_LIBRARIES}/mesos/3rdparty`.
If this inconsistency is really necessary, it would be nice to include some comments explaining
why.

##########
File path: 3rdparty/CMakeLists.txt
##########
@@ -459,6 +459,11 @@ install(
   DIRECTORY ${GLOG_INSTALL_DIR}/lib/
   DESTINATION ${MESOS_INSTALL_LIBRARIES})
 
+if (ENABLE_INSTALL_MODULE_DEPENDENCIES)
+  install(
+    DIRECTORY ${GLOG_INSTALL_DIR}/include
+    DESTINATION ${MESOS_INSTALL_LIBRARIES}/mesos/3rdparty)
+endif ()

Review comment:
       The autotools build system installs the 3rdparty dependencies by either running `make
install` within the dependency's build directory, or by manually copying headers into the
installation directory.
   
   In the autotools build system, glog is installed with `make install`, as are picojson,
protobuf, and zookeeper. Could you explain how you chose the install methods you're using
below? I'm not familiar with the motivations for the precise methods used in our autotools
`INSTALL_MODULE_DEPENDENCIES` implementation, so I'm just curious to figure out the best install
method here. IIUC, the equivalent of the `make install` method here would be using `install(TARGETS
...)` to specify install directories for the 3rdparty builds?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message