Do not remove catkin file, breaks robostack environment

[cmastalli]

This PR suggests removing the procedure for uninstalling catkin files. This is breaking ROS1 conda environments every time we need to re-install a package.

I don't also understand the reason for having these lines, but it seems we shouldn't do this.

[nim65s]

did you look at #52 ? It seems that there is a test failing without this removal, but I'm not sure which one.
@olivier-stasse : do you remember more about this ?

[gergondet]

I think the "issue" happens when you build a package that uses catkin to find dependencies outside a catkin workspace (or a catkin package outside a catkin workspace)

This will create a .catkin file in the install prefix as well as other files (env.sh and other setup scripts).

I think we can keep the existing behavior if we surround this code with if(NOT DEFINED CATKIN_DEVEL_PREFIX)

For reference, set(CATKIN_INSTALL_INTO_PREFIX_ROOT FALSE) disable the installations of those extra files in this scenario

[cmastalli]

I think the "issue" happens when you build a package that uses catkin to find dependencies outside a catkin workspace (or a catkin package outside a catkin workspace)

Not exactly my case. The code is built with cmake and without dependencies from Catkin packages.
Moreover, I define the installation paths as the Conda prefix, where robotstack installs ROS.

---

This will create a .catkin file in the install prefix as well as other files (env.sh and other setup scripts).

Quite the opposite. Robostack already installs the .catkin file inside the Conda prefix path. Then, make install first uninstalls this and other project files but never installs it again. This is the issue, and when this happens, the robostack gets broken.

---

I think we can keep the existing behavior if we surround this code with if(NOT DEFINED CATKIN_DEVEL_PREFIX)

For reference, set(CATKIN_INSTALL_INTO_PREFIX_ROOT FALSE) disable the installations of those extra files in this scenario

I could try this, but I would first encourage discussing if this existing behaviour is good. My current understanding is this is not a good policy, but probably I am ignoring something.

[olivier-stasse]

Dear @cmastalli I would not recommend installing from source a package in a conda prefix.

Why not using a local workspace after sourcing your robostack ROS environment ?

[gergondet]

Not exactly my case. The code is built with cmake and without dependencies from Catkin packages.
Moreover, I define the installation paths as the Conda prefix, where robotstack installs ROS.

Sorry if this was unclear. I was referring to the original issue that brought the code here but you are right, your scenario is basically the reverse of that (installing into a catkin prefix out of a catkin workspace) so my proposed approach would still break your scenario maybe the alternative is for jrl-cmakemodules to check whether the file already exists when the configuration happens and never touch the file in that case since it's probably not installed by the package we are currently building.

[cmastalli]

Dear @cmastalli I would not recommend installing from source a package in a conda prefix.

I install them inside the conda prefix so I can have multiple installations of packages such as Pinocchio and Crocoddyl. I know I could create different installation folders and create a bash script, but I am lazy :)

Do you have other suggestions?

[cmastalli]

Not exactly my case. The code is built with cmake and without dependencies from Catkin packages.
Moreover, I define the installation paths as the Conda prefix, where robotstack installs ROS.

Sorry if this was unclear. I was referring to the original issue that brought the code here but you are right, your scenario is basically the reverse of that (installing into a catkin prefix out of a catkin workspace) so my proposed approach would still break your scenario maybe the alternative is for jrl-cmakemodules to check whether the file already exists when the configuration happens and never touch the file in that case since it's probably not installed by the package we are currently building.

Yes, this suggestion looks better to me. Although, I am unsure how to do it. Do you have some clues?

[olivier-stasse]

Dear @cmastalli I would not recommend installing from source a package in a conda prefix.

I install them inside the conda prefix so I can have multiple installations of packages such as Pinocchio and Crocoddyl. I know I could create different installation folders and create a bash script, but I am lazy :)

Do you have other suggestions?

Push the binary packages in robostack.
You can do it by building a devel environment where you use the ROS recipes to build pinocchio and crocoddyl.

[gergondet]

Yes, this suggestion looks better to me. Although, I am unsure how to do it. Do you have some clues?

Using the cache to check on the first run (or when CMAKE_INSTALL_PREFIX changes) if the .catkin file already exists there. So, something like (to be tested):

if(NOT DEFINED PACKAGE_CREATES_DOT_CATKIN OR NOT "${PACKAGE_PREVIOUS_INSTALL_PREFIX}" EQUAL "${CMAKE_INSTALL_PREFIX}")
  set(PACKAGE_PREVIOUS_INSTALL_PREFIX "${CMAKE_INSTALL_PREFIX}" CACHE INTERNAL "Cache install prefix given to the package")
  if(EXISTS "${CMAKE_INSTALL_PREFIX}/.catkin")
    set(PACKAGE_CREATES_DOT_CATKIN FALSE CACHE INTERNAL "")
  else()
    set(PACKAGE_CREATES_DOT_CATKIN TRUE CACHE INTERNAL "")
  endif()
endif()

# Then pass PACKAGE_CREATES_DOT_CATKIN to the cmake_uninstall script and act accordingly

[cmastalli]

Yes, this suggestion looks better to me. Although, I am unsure how to do it. Do you have some clues?

Using the cache to check on the first run (or when CMAKE_INSTALL_PREFIX changes) if the .catkin file already exists there. So, something like (to be tested):

if(NOT DEFINED PACKAGE_CREATES_DOT_CATKIN OR NOT "${PACKAGE_PREVIOUS_INSTALL_PREFIX}" EQUAL "${CMAKE_INSTALL_PREFIX}")
  set(PACKAGE_PREVIOUS_INSTALL_PREFIX "${CMAKE_INSTALL_PREFIX}" CACHE INTERNAL "Cache install prefix given to the package")
  if(EXISTS "${CMAKE_INSTALL_PREFIX}/.catkin")
    set(PACKAGE_CREATES_DOT_CATKIN FALSE CACHE INTERNAL "")
  else()
    set(PACKAGE_CREATES_DOT_CATKIN TRUE CACHE INTERNAL "")
  endif()
endif()

# Then pass PACKAGE_CREATES_DOT_CATKIN to the cmake_uninstall script and act accordingly

@gergondet -- I pushed these changes and tested them on my PC, and of course, they work!
Apologise for a late action.
Please let me know if there is something else left in this PR.

[gergondet]

I think you also need to pass -DPACKAGE_CREATES_DOT_CAKTIN=${PACKAGE_CREATES_DOT_CATKIN} to the script (before -P here https://github.com/cmastalli/jrl-cmakemodules/blob/topic/catkin-uninstall/uninstall.cmake#L64) unless it somehow reads it from the cache?

[gergondet]

Hi @cmastalli

I have added passing PACKAGE_CREATES_DOT_CATKIN to the uninstall script (it doesn't use the cache at all) and I have added a unit test around this functionality.

I have also made sure the tests actually fail when there is a failure. Since we introduce the strict requirement on CMake >= 3.10 none the tests have not passed but those failures were not showing up as such.

[cmastalli]

Hi @cmastalli

I have added passing PACKAGE_CREATES_DOT_CATKIN to the uninstall script (it doesn't use the cache at all) and I have added a unit test around this functionality.

I have also made sure the tests actually fail when there is a failure. Since we introduce the strict requirement on CMake >= 3.10 none the tests have not passed but those failures were not showing up as such.

Thanks for including the unit tests and your feedback, @gergondet !

This feature fixes annoying issues encountered during the installation of many of my packages :)