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:
[email protected]
