colcon / colcon-cmake

Extension for colcon to support CMake packages
http://colcon.readthedocs.io
Apache License 2.0
16 stars 25 forks source link

Trying to use ament_uninstall uninstalls in wrong order #58

Open rotu opened 4 years ago

rotu commented 4 years ago

While ament_uninstall is supposedly provided for uninstalling packages, often doing so results in a crash as colcon processes the packages in topological dependency order, removing build-time dependencies that later packages need:

➜ colcon build --packages-select-regex ".*yaml.*" --cmake-target uninstall
Starting >>> libyaml_vendor
Starting >>> launch_yaml                      
Starting >>> yaml_cpp_vendor                                                   
Finished <<< yaml_cpp_vendor [7.19s]                                                                                                                      
Finished <<< libyaml_vendor [7.91s]                                                                     
Starting >>> rcl_yaml_param_parser
Finished <<< launch_yaml [8.78s]                                                      
--- stderr: rcl_yaml_param_parser                                             
CMake Error at CMakeLists.txt:7 (find_package):
  By not providing "Findlibyaml_vendor.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "libyaml_vendor", but CMake did not find one.

  Could not find a package configuration file provided by "libyaml_vendor"
  with any of the following names:

    libyaml_vendorConfig.cmake
    libyaml_vendor-config.cmake

  Add the installation prefix of "libyaml_vendor" to CMAKE_PREFIX_PATH or set
  "libyaml_vendor_DIR" to a directory containing one of the above files.  If
  "libyaml_vendor" provides a separate development package or SDK, be sure it
  has been installed.

make: *** [cmake_check_build_system] Error 1
---
Failed   <<< rcl_yaml_param_parser  [ Exited with code 2 ]

Summary: 3 packages finished [22.6s]
  1 package failed: rcl_yaml_param_parser
  1 package had stderr output: rcl_yaml_param_parser
dirk-thomas commented 4 years ago

I am not sure this kind of "catch all" approach to uninstall multiple packages is going to work.

First, the --cmake-target uninstall option will only uninstall CMake packages. Any other package type would still be build normally.

Second, as you discovered the packages would need to be processed in reverse topological order. A new option to reverse the order could probably be added - I am just not sure that would be worth the effort / complexity.

rotu commented 4 years ago

If it's worth having an uninstall feature, it's worth having one that actually works. This is an excellent illustration of why it doesn't fit into the concept of a build target. Maybe uninstall should instead be exposed as part of the colcon build interface?

dirk-thomas commented 4 years ago

Please feel free to propose alternatives. I don't think I will have time to work on this functionality since I don't have a use case for it.

rotu commented 4 years ago

I don’t have a use case for it either. I’m in favor of quietly deprecating this footgun by disabling it in ament_cmake until it can be properly attended to.

dirk-thomas commented 4 years ago

I’m in favor of quietly deprecating this footgun by disabling it in ament_cmake

The functionality provided by ament_cmake is useful on its own when e.g. used without colcon (calling cmake / make install / make uninstall directly). Just because colcon doesn't have a process for uninstall multiple packages is not an argument for removing the functionality from ament_cmake.

rotu commented 4 years ago

That makes sense. I was under the assumption that the ament build tools were not really used outside of catkin.