Fix parallel builds #955
Fix parallel builds #955
Conversation
.. to allow operation across file systems
.. to sync multiple cmake processes writing to that location
rhaschke
force-pushed
the
fix-parallel-builds
branch
from
6787656 to
bbfe1a9
Compare
cmake/atomic_configure_file.cmake
Outdated
| endif() | ||
| file(RENAME "${atomic_file}" "${output}") | ||
| # sync multiple catkin cmake processes writing to that location | ||
| file(LOCK "${output_path}" DIRECTORY GUARD FUNCTION) |
There was a problem hiding this comment.
This signature is only available as of CMake 3.2. Please use it only conditionally.
While this ensures mutually exclusive access in CMake are any of the files generated by this function accessed in parallel from non-CMake processes?
|
@dirk-thomas Valid point. Fixed. |
cmake/atomic_configure_file.cmake
Outdated
| if(NOT EXISTS ${output_path}) | ||
| file(MAKE_DIRECTORY ${output_path}) | ||
| # sync multiple catkin cmake processes writing to that location | ||
| if (CMAKE_VERSION VERSION_GREATER 3.2) |
There was a problem hiding this comment.
The condition evaluates to FALSE for CMake 3.2 for which it should be TRUE.
cmake/atomic_configure_file.cmake
Outdated
| @@ -1,9 +1,11 @@ | |||
| function(atomic_configure_file input output) | |||
| set(atomic_file "${CMAKE_BINARY_DIR}/atomic_configure_file") | |||
| get_filename_component(atomic_file ${output} NAME) | |||
| set(atomic_file "${CMAKE_BINARY_DIR}/${atomic_file}") | |||
There was a problem hiding this comment.
To reduce the chance of collisions if the basename of output is the same as an existing file in the binary dir please keep atomic_configure_file as a prefix to the name: set(atomic_file "${CMAKE_BINARY_DIR}/atomic_configure_file_${atomic_file}")
There was a problem hiding this comment.
This was changed intentionally. Using a different name than the final one, the copying needs two steps: first copying and then renaming the file at the destination location. (Remember that COPY doesn't allow for a simultaneous rename).
For me, this contradicts the atomic idea.
There was a problem hiding this comment.
Good point - I didn't consider that. Can you place the file in a subdirectory atomic_configure_file then to minimize the chance of collisions.
rhaschke
force-pushed
the
fix-parallel-builds
branch
from
7fabc69 to
52bc686
Compare
|
Thank you for the patch and for iterating on it. |
This PR addresses an issue originally raised in catkin/catkin_tools#422, but which is actually related to the underlying catkin cmake files.
file RENAMEonly working within a filesystem. It fails when trying to move a file across file systems. However, probably the operation within a single filesystem is what makes the operation atomic in cmake.file LOCKon the destination folder as suggested by @guihomework in catkin build fails when build space on a different device due to cross-device link catkin/catkin_tools#422 (comment). This additionally avoids issues of several parallel catkin cmake processes writing the same environment files simultaneously (which is a real issue for our jenkins setup deploying directly into a shared install-space.)