Skip to content

Only add the uninstall target if the C driver is the top level project #1352

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 24, 2023
Merged

Only add the uninstall target if the C driver is the top level project #1352

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
73e9f24
made the uninstall target configurable
kkloberdanz Jul 20, 2023
de2558e
comment explaining naming conflict
kkloberdanz Jul 20, 2023
976ab56
use Ezra's suggestion to only add the uninstall target if the C drive…
kkloberdanz Jul 21, 2023
f06de4f
Update generate_uninstall/CMakeLists.txt
kkloberdanz Jul 21, 2023
c42d9be
Update generate_uninstall/CMakeLists.txt
kkloberdanz Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions generate_uninstall/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@ if (WIN32)
")
install (FILES "${CMAKE_CURRENT_BINARY_DIR}/${UNINSTALL_PROG}" DESTINATION "${UNINSTALL_PROG_DIR}" PERMISSIONS ${UNINSTALL_PERMISSIONS})

add_custom_target (uninstall
COMMAND call "${CMAKE_CURRENT_BINARY_DIR}/${UNINSTALL_PROG}"
)
if ("${CMAKE_SOURCE_DIR}" STREQUAL "${PROJECT_SOURCE_DIR}")
# Define the `uninstall` target if the C driver is the top-level project.
add_custom_target (uninstall
COMMAND call "${CMAKE_CURRENT_BINARY_DIR}/${UNINSTALL_PROG}"
)
endif ()
else ()
install (CODE "
string(REPLACE \";\" \"\\n\" MONGOC_INSTALL_MANIFEST_CONTENT
Expand All @@ -50,7 +53,10 @@ else ()
")
install (FILES "${CMAKE_CURRENT_BINARY_DIR}/${UNINSTALL_PROG}" DESTINATION "${UNINSTALL_PROG_DIR}" PERMISSIONS ${UNINSTALL_PERMISSIONS})

add_custom_target (uninstall
COMMAND sh "${CMAKE_CURRENT_BINARY_DIR}/${UNINSTALL_PROG}"
)
Comment on lines -53 to -55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest a different design:

if(NOT TARGET uninstall)
  add_custom_target(uninstall)
endif()
add_custom_target(mongo-c-driver-uninstall COMMAND [do the uninstall])
add_dependencies(uninstall mongo-c-driver-uninstall)

This way, we "add" to an existing uninstall target if it already defined (e.g. by a parent project).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be preferable to defer the responsibility of including C Driver files in the uninstall command to the CXX Driver (top-level project) rather than unconditionally hooking into it from the C Driver side? The CXX Driver may want to uninstall the C Driver as part of its uninstall command, but another top-level project (i.e. 3rd party that add_subdirectory()'s the C Driver) might not. As I understand, this would either unconditionally attach the C Driver to any existing uninstall command regardless if the parent project actually wants to include it or not, or force the parent project to hook into the C Driver's uninstall target instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's tricky, since the C driver unconditionally(?) adds its install rules, which the parent may not want, which would imply that it should also unconditionally(?) add its uninstall rules as well. It is possible to EXCLUDE_FROM_ALL on the add_subdirectory(), but I'm not sure if that affects the install() commands (the documentation doesn't mention it, but one would intuit that it should).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick local test suggests that EXCLUDE_FROM_ALL does exclude the subproject's install rules when running the install target in the parent project. This is consistent with the GNU Make documentation for the install target:

If possible, write the install target rule so that it does not modify anything in the directory where the program was built, provided ‘make all’ has just been done. This is convenient for building the program under one user name and installing it under another.

combined with CMake documentation for EXCLUDE_FROM_ALL:

If the EXCLUDE_FROM_ALL argument is provided then targets in the subdirectory will not be included in the ALL target of the parent directory by default, and will be excluded from IDE project files. Users must explicitly build targets in the subdirectory.

This is also consistent with this comment by a CMake maintainer, although their remarks are not as confident as I would have hoped:

Regarding the overarching theme of excluding things from the ALL target and then installing them, the thing to remember is that if a target is excluded from ALL, then you the user are responsible for ensuring it is built if you then do something that wants to install that target. The documentation for the install() command calls this undefined behavior, but that’s perhaps a bit strong. In reality, my preceding sentence is the real constraint, as far as I’m aware (but I haven’t checked how export sets affect the picture).

The documentation stating "undefined behavior" seems to have been changed from CMake 3.17 to CMake 3.18 (current wording):

If a target has EXCLUDE_FROM_ALL set to true, it may still be listed in an install(TARGETS) command, but the user is responsible for ensuring that the target’s build artifacts are not missing or outdated when an install is performed.

Summary: there seems to be a lot more "should" qualifiers to this than one would expect or hope, but it should behave as expected (EXCLUDE_FROM_ALL -> exclude from all target -> also exclude from install target).

if ("${CMAKE_SOURCE_DIR}" STREQUAL "${PROJECT_SOURCE_DIR}")
# Define the `uninstall` target if the C driver is the top-level project.
add_custom_target (uninstall
COMMAND sh "${CMAKE_CURRENT_BINARY_DIR}/${UNINSTALL_PROG}"
)
endif ()
endif ()