find_package(ament_cmake_core) call defines uninstall target in non-ament CMake packages

[traversaro]

It seems that any call to find_package(ament_cmake_core), even if a non-ament project, defines a uninstall target, that may interfere with existing targets. In general, it is probably not expected by users that just a find_package call can define targets. This is even more confusing if ament_cmake_core is not included directly, but as a dependency when a find_package for a different project is called, for example find_package(rclcpp).

Minimal Reproducible Example

I prepared a MRE in https://github.com/traversaro/mre-rclcpp-uninstall .

In it, there is a minimal project that only installs an empty file, and defines the uninstall target according to CMake guidelines (https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#can-i-do-make-uninstall-with-cmake). In a subdirectory then, find_package(ament_cmake_core) is called. Due to the call to find_package(ament_cmake_core), make uninstall fails:

(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ git clone https://github.com/traversaro/mre-rclcpp-uninstall
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ cd mre-rclcpp-uninstall
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ mkdir build_with_rclcpp
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ cd build_with_rclcpp/
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall/build_with_rclcpp$ cmake -DCMAKE_INSTALL_PREFIX=./install -DUSES_RCLCPP:BOOL=ON ..
-- The C compiler identification is GNU 11.3.0
-- The CXX compiler identification is GNU 11.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ament_cmake_core: 1.3.3 (/home/traversaro/mambaforge/envs/roshumble/share/ament_cmake_core/cmake)
-- Found Python3: /home/traversaro/mambaforge/envs/roshumble/bin/python3.10 (found version "3.10.8") found components: Interpreter
-- Not defining top-level uninstall target as it is already defined
-- Configuring done
-- Generating done
-- Build files have been written to: /home/traversaro/mre-rclcpp-uninstall/build_no_rclcpp
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall/build_with_rclcpp$ make install
Install the project...
-- Install configuration: ""
-- Installing: /home/traversaro/mre-rclcpp-uninstall/build_with_rclcpp/install/share/MREUninstallTarget/empty.file
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall/build_with_rclcpp$ make uninstall
CMake Error at ament_cmake_uninstall_target/ament_cmake_uninstall_target.cmake:34 (message):
  Cannot find install manifest:
  /home/traversaro/mre-rclcpp-uninstall/build_with_rclcpp/subdir/install_manifest.txt


make[3]: *** [subdir/CMakeFiles/MREUninstallTarget_uninstall.dir/build.make:70: subdir/CMakeFiles/MREUninstallTarget_uninstall] Error 1
make[2]: *** [CMakeFiles/Makefile2:125: subdir/CMakeFiles/MREUninstallTarget_uninstall.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:106: subdir/CMakeFiles/uninstall.dir/rule] Error 2
make: *** [Makefile:169: uninstall] Error 2

We can see that if find_package(ament_cmake_core) is not called (via -DUSES_RCLCPP:BOOL=OFF), everything works as expected:

(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ git clone https://github.com/traversaro/mre-rclcpp-uninstall
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ cd mre-rclcpp-uninstall
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ mkdir build_no_rclcpp
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall$ cd build_no_rclcpp/
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall/build_no_rclcpp$ cmake -DCMAKE_INSTALL_PREFIX=./install -DUSES_RCLCPP:BOOL=OFF ..
-- The C compiler identification is GNU 11.3.0
-- The CXX compiler identification is GNU 11.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/traversaro/mambaforge/envs/roshumble/bin/x86_64-conda-linux-gnu-cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/traversaro/mambaforge/envs/roshumble/bin/x86_64-conda-linux-gnu-c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Defining top-level uninstall target
-- Configuring done
-- Generating done
-- Build files have been written to: /home/traversaro/mre-rclcpp-uninstall/build_no_rclcpp
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall/build_no_rclcpp$ make install
Install the project...
-- Install configuration: ""
-- Installing: /home/traversaro/mre-rclcpp-uninstall/build_no_rclcpp/install/share/MREUninstallTarget/empty.file
(roshumble) traversaro@IITICUBLAP257:~/mre-rclcpp-uninstall/build_no_rclcpp$ make uninstall
-- Uninstalling /home/traversaro/mre-rclcpp-uninstall/build_no_rclcpp/install/share/MREUninstallTarget/empty.file
Built target uninstall

[traversaro]

Possible Fix

A possible fix is to move the code in https://github.com/ament/ament_cmake/blob/1.5.3/ament_cmake_core/ament_cmake_uninstall_target-extras.cmake#L17 to a function, and then call that function in ament_package . In this way, the behaviour for ament packages would be exactly the same, without unexpected side-effects for non-ament packages. However strictly speaking this is a breaking change, so it would need to be processed approprately. Probably it would be helpful to do the same change (move code to functions called by ament_package) for other scripts that creates side-effects such as defining CMake options, at a first glance these 5 variables are defined by find_package(ament_cmake_core) :

AMENT_CMAKE_ENVIRONMENT_GENERATION:BOOL=OFF
AMENT_CMAKE_ENVIRONMENT_PACKAGE_GENERATION:BOOL=ON
AMENT_CMAKE_ENVIRONMENT_PARENT_PREFIX_PATH_GENERATION:BOOL=ON
AMENT_CMAKE_SYMLINK_INSTALL:BOOL=OFF
AMENT_CMAKE_UNINSTALL_TARGET:BOOL=ON

Possible Mitigation

A possible mitigation that instead is fully back-compatible is to make sure that the uninstall defined by find_package(ament_cmake_core) works fine even if find_package(ament_cmake_core) is called in subdirectory and not in the root source directory. For doing this, it should be sufficient to change CMAKE_CURRENT_BINARY_DIR in CMAKE_BINARY_DIR

ament_cmake/ament_cmake_core/cmake/uninstall_target/ament_cmake_uninstall_target.cmake.in

Line 32 in 3f305a3

set(install_manifest "@CMAKE_CURRENT_BINARY_DIR@/install_manifest.txt")

. If the code is called in the root source dir, CMAKE_BINARY_DIR is exactly the same of CMAKE_CURRENT_BINARY_DIR, while if the code is called not in the root source dir, CMAKE_BINARY_DIR is the correct one to use in this context, as the install_manifest.txt is always only created in the root binary dir.

[cottsay]

I believe we can move the function call to a new ament extension, which will have the effect of calling it in ament_package as you suggest.

IMO, we don't need to tick/tock this change or undergo a deprecation cycle - it seems like an oversight to me.