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

[kkloberdanz]

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

The uninstall target was conflicting with the C++ driver's uninstall target. This fixes the conflict.

[eramongodb]

Given this workaround is specific to the uninstall CMake custom target, is the conditional use of the CMake global ALLOW_DUPLICATE_CUSTOM_TARGETS property an option? Note, ALLOW_DUPLICATE_CUSTOM_TARGETS only applies to Makefile generators.

[kevinAlbs]

If the C++ driver renames the target, will running the uninstall target in the C++ driver only uninstall the C++ driver (and leave the C driver installed files)?

[kkloberdanz]

With this PR, make uninstall will uninstall both the C and C++ drivers:

$ make install
$ tree -L 2 /tmp/cxxdriver/
/tmp/cxxdriver/
├── bin
│   └── mongoc-stat
├── include
│   ├── bsoncxx
│   ├── libbson-1.0
│   ├── libmongoc-1.0
│   └── mongocxx
├── lib
│   ├── cmake
│   ├── libbson-1.0.so -> libbson-1.0.so.0
│   ├── libbson-1.0.so.0 -> libbson-1.0.so.0.0.0
│   ├── libbson-1.0.so.0.0.0
│   ├── libbson-static-1.0.a
│   ├── libbsoncxx.so -> libbsoncxx.so._noabi
│   ├── libbsoncxx.so.0.0.0
│   ├── libbsoncxx.so._noabi -> libbsoncxx.so.0.0.0
│   ├── libmongoc-1.0.so -> libmongoc-1.0.so.0
│   ├── libmongoc-1.0.so.0 -> libmongoc-1.0.so.0.0.0
│   ├── libmongoc-1.0.so.0.0.0
│   ├── libmongoc-static-1.0.a
│   ├── libmongocxx.so -> libmongocxx.so._noabi
│   ├── libmongocxx.so.0.0.0
│   ├── libmongocxx.so._noabi -> libmongocxx.so.0.0.0
│   └── pkgconfig
└── share
    ├── mongo-c-driver
    └── mongo-cxx-driver

12 directories, 15 files
$ make uninstall
$ tree -L 2 /tmp/cxxdriver/
/tmp/cxxdriver/ [error opening dir]

0 directories, 0 files

[kkloberdanz]

Given this workaround is specific to the uninstall CMake custom target, is the conditional use of the CMake global ALLOW_DUPLICATE_CUSTOM_TARGETS property an option? Note, ALLOW_DUPLICATE_CUSTOM_TARGETS only applies to Makefile generators.

Setting this global variable instead of the current workaround results in this error:

CMake Error at generate_uninstall/CMakeLists.txt:90 (add_custom_target):
  add_custom_target cannot create target "uninstall" because another target
  with the same name already exists.  The existing target is a custom target
  created in source directory
  "/home/kyle-mongo/src/mongo-cxx-driver/cmake-build/_deps/mongo-c-driver-src/generate_uninstall".
  See documentation for policy CMP0002 for more details.

[eramongodb]

Setting this global variable ...

@kkloberdanz Just confirming, this is a global property, not a variable. You need to use set_property() or set_directory_properties() rather than set(). Does this error still manifest even after using set_property() or set_directory_properties()?

Edit: global properties != directory properties.

[eramongodb]

Per https://gitlab.kitware.com/cmake/cmake/-/issues/23024#note_1098749, it may actually be more reasonable to simply not define the uninstall target at all if this project is not the top-level project (note: PROJECT_IS_TOP_LEVEL is a CMake 3.21 feature). The top-level project (i.e. the CXX Driver) would be responsible for defining the uninstall target behavior to include (or ignore) this project's files.

if ("${CMAKE_SOURCE_DIR}" STREQUALS "${PROJECT_SOURCE_DIR}")
  add_custom_target(uninstall ...)
endif ()

[kkloberdanz]

This seems to work too. I don't have an opinion on which one we go with.

[kkloberdanz]

Setting this global variable ...

@kkloberdanz Just confirming, this is a global property, not a variable. You need to use set_property() or set_directory_properties() rather than set(). Does this error still manifest even after using set_property() or set_directory_properties()?

Edit: global properties != directory properties.

Setting this as a property results in failures on Windows:

CMake Error: This project has enabled the ALLOW_DUPLICATE_CUSTOM_TARGETS global property.  The "Visual Studio 15 2017 Win64" generator does not support duplicate custom targets.  Consider using a Makefiles generator or fix the project to not use duplicate target names.

I set it like so in the top level CMakeLists.txt for mongo-cxx-driver:

project(MONGO_CXX_DRIVER LANGUAGES CXX)

+set_property(GLOBAL PROPERTY ALLOW_DUPLICATE_CUSTOM_TARGETS TRUE)

if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")

[vector-of-bool]

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

[eramongodb]

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.

[vector-of-bool]

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

[eramongodb]

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