catkin_generate_environment: don't overwrite existing .catkin file #676
Conversation
|
Can you please update the PR to only write an empty file within the |
Yeah that makes more sense. Done. |
| install(FILES | ||
| ${CMAKE_BINARY_DIR}/catkin_generated/installspace/.catkin | ||
| DESTINATION ${CMAKE_INSTALL_PREFIX}) | ||
| set(_catkin_marker_file_installed "${CMAKE_INSTALL_PREFIX}/.catkin") |
There was a problem hiding this comment.
I am not sure that it works but have you tried to move the variable into the install(CODE ..) block and actually keep CMAKE_INSTALL_PREFIX as a variable (escaping the $) to expand it at install time?
|
Putting the escaped I also modified the develspace marker file generation in the case that an empty |
| install(FILES | ||
| ${CMAKE_BINARY_DIR}/catkin_generated/installspace/.catkin | ||
| DESTINATION ${CMAKE_INSTALL_PREFIX}) | ||
| set(_catkin_marker_file_installed "") |
There was a problem hiding this comment.
This variable seems to be not used anymore?
There was a problem hiding this comment.
Sorry, thought I got that.
- During install step of a catkin package, don't overwrite an existing .catkin file - During a devel build, don't append a semicolon-prefixed source path to the .catkin file if the .catkin file is empty. This is related to the previous race condition fix which prevented overwriting the .catkin file. This should introduce negligible sensitivity to parallel invocations of catkin-based CMake builds only if the .catkin file is empty. For all other cases, it is still an atomic <4k APPEND write.
|
Thanks for keeping up with all my nitpicking. |
…nstall catkin_generate_environment: don't overwrite existing .catkin file
Thank you for responding so quickly to these PRs! |
| # install empty workspace marker if it doesn't already exist | ||
| install(CODE " | ||
| if (NOT EXISTS \"\${CMAKE_INSTALL_PREFIX}/.catkin\") | ||
| file(WRITE \"\${CMAKE_INSTALL_PREFIX}/.catkin\" \"\") |
There was a problem hiding this comment.
This code fails when the folder CMAKE_INSTALL_PREFIX does not exist: http://jenkins.ros.org/job/ros-indigo-catkin_binarydeb_trusty_amd64/11/console
I hope the fix in f2efd9e works...
Actually that won't be sufficient since it also needs to handle DESTDIR correctly: 149a241
The Orocos RTT package, a pure CMake package, currently doesn't use catkin for building, but it does call
find_package(catkin)in order to determine if catkin is installed on the system, and if so, it callscatkin_add_env_hooks()to add an env hook. This works fine if you're installing it to an isolated workspace or somewhere else, but when installing to a develspace (like withcatkin_tools), it causes the.catkinfile to be overwritten and you lose the list of all previously-built packages.This patch replaces the normal CMake
install(FILES ...)command with aninstall(CODE ...)command which just checks if the file already exists or not.