search

catkin / catkin_tools

Command line tools for working with catkin
Apache License 2.0
163 stars 146 forks source link

Fix catkin clean for single packages #710

Closed MichaelGrupp closed 2 years ago

MichaelGrupp commented 2 years ago

The clean command looks up the installed files from the install_manifest.txt of the package. But this file was not copied to the install space.

This patch adds that copy in the same way it is already done in the plain CMake build job (see jobs/cmake.py).

In addition to that, installed Python modules are also removed if their package name is the same as the catkin package.

timonegk commented 2 years ago

I agree that it would be good to remove the installed files with catkin clean and that the current stage cleaninstall in create_catkin_clean_job does not really make sense as there are never any packages to clean for catkin packages.

Currently, there are two possible setups for the install space: the merged layout and the isolated layout. For the isolated layout, your approach works fine, but for the merged layout, it must be adapted. The install_manifest.txt also contains common files (e.g. the setup.bash file). Consequently, they would also be removed by catkin clean, rendering the install space unusable.

Therefore it would be necessary to add a way of distinguishing the package specific files from the common files added by catkin to avoid removing too many files.

MichaelGrupp commented 2 years ago

@timonegk I added a patch that filters the files in the shared install base directory when deleting: https://github.com/catkin/catkin_tools/commit/d5d0b0bbb22d241ef594487007a5974888ddc5fc Seems to work, but I don't know if there are some corner case where this rather cheap trick could be broken?

timonegk commented 2 years ago

This looks fine to me. The solution might not be perfect because in theory, any package could install to the install directory root. But that would only lead to these files not being removed in this particular case, which is still better than not removing any files.

MichaelGrupp commented 2 years ago

One more thing that should be resolved: when cleaning a single package with Python code, the modules in install/lib/python3/dist-packages/ are not removed. This would lead to unexpected behavior because module imports would still use this path after cleaning.

MichaelGrupp commented 2 years ago

Added another commit to handle Python packages that follow the standard ROS practice for package naming. I also changed the name of the PR since the scope is now a bit more than just the install_manifest.

Now everything works in my local test environment with mixed C++/Python Catkin packages.

timonegk commented 2 years ago

Yes, that is probably the best way to do it currently. In theory, we should be able to get the exact list of files from setup.py via the --record parameter. But this parameter would have to be passed through the catkin / cmake pipeline, and there is currently no way to do that. (If you wanted to look into that, the call to setup.py is done in python_distutils_install.sh.in. But it is probably not worth it.) I think your solution looks fine, I will test it a bit more to make sure it doesn't break anything. If you have the resources, do you mind adding a test?

timonegk commented 2 years ago

I added a test and everything works nicely, so I will merge it now. Thanks again for the patch!