mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [mesos] asekretenko commented on a change in pull request #363: CMake enable install module dependencies.
Date Tue, 19 May 2020 13:18:00 GMT

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



##########
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:
       Given that the output layout of `enable-install-module-dependencies` is not really
documented, I would suggest that we should fix the inconsistency unless there are issues preventing
this (filename conflicts, etc.).
   
   As for the location of that single directory, we basically have two choices:
    1) the same directory where `mesos/...`  headers are installed (currently `${CMAKE_INSTALL_PREFIX}/include`
)
    2) some other directory (currently `${CMAKE_INSTALL_PREFIX}/lib/mesos/3rdparty/include`)
   
   Option (2) allows for more flexibility. One can install Mesos built with bundled module
dependencies and `enable-install-module-dependencies` into root (without a prefix) alongside,
for example, system boost (ours bundled boost subset is header-only). 
   Note that to avoid conflict between a bundled mesos installed into root and a system glog,
3rdparty libraries will also need to be installed into another directory.
   
   Option (1) effectively disallows installing Mesos with bundled 3rdparty module dependencies
without a prefix. Probably this is not an issue: I don't see why would one want to use bundled
3rdparties, but be unable to install Mesos into prefix.
   
   At this point I don't have a good understanding which of these two makes using cmake to
build modules simpler. This might be a factor in choosing between (1) and (2).

##########
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:
       Do we have a good understanding whether `install(TARGETS ...)` can be configured up
to install headers:
    - of 3rdparties which have `INSTALL_COMMAND  ${CMAKE_NOOP}` in `ExternalProject_Add()`?
    - of 3rdparties that have **their** install step executed during build time?
   
   If the answer is "no" in both cases, then `install(FILES ...`/`install(DIRECTORY ...` seems
 to be the only option for installing headers, unfortunately.

##########
File path: src/CMakeLists.txt
##########
@@ -675,6 +675,13 @@ install(
   RUNTIME DESTINATION ${MESOS_INSTALL_RUNTIME}
   ARCHIVE DESTINATION ${MESOS_INSTALL_LIBRARIES}
   LIBRARY DESTINATION ${MESOS_INSTALL_LIBRARIES})
+install(
+    DIRECTORY
+      ${CMAKE_SOURCE_DIR}/3rdparty/libprocess/include
+      ${CMAKE_SOURCE_DIR}/3rdparty/stout/include

Review comment:
       libprocess/stout headers should be installed by cmake files in libprocess/stout accordingly.

   Note that you will need a separate commit for each of them: there is a convention that
one commit should not span two projects (stout and libprocess are considered separate projects).

##########
File path: src/CMakeLists.txt
##########
@@ -675,6 +675,13 @@ install(
   RUNTIME DESTINATION ${MESOS_INSTALL_RUNTIME}
   ARCHIVE DESTINATION ${MESOS_INSTALL_LIBRARIES}
   LIBRARY DESTINATION ${MESOS_INSTALL_LIBRARIES})
+install(
+    DIRECTORY
+      ${CMAKE_SOURCE_DIR}/3rdparty/libprocess/include
+      ${CMAKE_SOURCE_DIR}/3rdparty/stout/include
+      ${MESOS_PUBLIC_INCLUDE_DIR}
+      ${MESOS_BIN_INCLUDE_DIR}
+    DESTINATION .)

Review comment:
       The fact that both these paths end with '/include' is a mere coincidence (especially
for BIN_INCLUDE_DIR), it would be better not to rely on this.
   I would suggest to install **contents** of these directories (added '/') and specify  ${MESOS_INSTALL_HEADERS}
as destination, like
   ```
   ${MESOS_PUBLIC_INCLUDE_DIR}/
   ${MESOS_BIN_INCLUDE_DIR}/
   DESTINATION ${MESOS_INSTALL_HEADERS})
   ```




----------------------------------------------------------------
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