Skip to content

Commit ce501e0

Browse files
thomas-bcdevin-ai-integration[bot]cognition-team
authored
Fix CMake unit test race condition (#5093)
* Fix race in CMake chained autocoder test target Two test targets (`_test_autocode` and `_test_chained_autocode`) both invoke the `test_target_autocoder`, which registers an `add_custom_command(OUTPUT ${BASENAME}.test-target.generated.txt COMMAND ${CMAKE_COMMAND} -E copy ...)` for each module. CMake emits the same copy rule into each custom target's submakefile, so under `make -jN` the two rules can fire simultaneously and race on the destination file, intermittently producing: Error copying file "...test1.test-target.txt" to "...test1.test-target.generated.txt". Make the chained-autocode custom target depend on the `_test_autocode` sibling. Make then serializes the two, ensuring the copy fires once and the second build.make sees the destination as up-to-date. Co-Authored-By: thomas.boyer.chammard <thomas.boyer.chammard@jpl.nasa.gov> * Expand comment explaining the parallel-build race fix Co-Authored-By: thomas.boyer.chammard <thomas.boyer.chammard@jpl.nasa.gov> * Tighten comment on test_chained_autocoder serialization Co-Authored-By: thomas.boyer.chammard <thomas.boyer.chammard@jpl.nasa.gov> * Clarify comment Added manual dependency to prevent race condition between test targets. * fix spelling * Differentiate chained-autocoder test inputs to avoid duplicate output filenames Both target/test_autocoder and target/test_chained_autocoder used to invoke autocoder/test_target_autocoder on the same module, so CMake emitted the same `cmake -E copy` rule for *.test-target.generated.txt into both targets' sub-makefiles. Under `make -jN` the two copies of the rule could fire in parallel and race on the destination file, producing intermittent 'Error copying file' failures in CI. Give the chained test its own input suffix (.chained-input.txt) and a dedicated first-stage autocoder (autocoder/test_chained_input_autocoder) that emits .chained-input.generated.txt. The standalone test_target_autocoder now only matches files in TestTargetAutocoder, so no two test targets produce the same output filename and the race is eliminated at the source. * Revert "Differentiate chained-autocoder test inputs to avoid duplicate output filenames" This reverts commit d4df7f9. * Make test_autocoder and test_chained_autocoder targets non-overlapping Both targets are registered globally and applied to every module, and both invoke test_target_autocoder. CMake's Makefile generator then emits the same `cmake -E copy` rule into both submakefiles for any module reached by both targets, causing make -jN to race on the destination file ("Error copying file ..."). Each test fixture only needs one of the two targets, so opt out of the other via a CMake variable read inside the target's add_module_target hook: - TestChainedAutocoderModule sets SKIP_TEST_AUTOCODER_TARGET. The chained target still runs test_target_autocoder as the genuine first stage. - TestTargetAutocoderModule sets SKIP_TEST_CHAINED_AUTOCODER_TARGET. The standalone target keeps testing test_target_autocoder by itself. The flags must be set before register_fprime_module since custom targets are attached inside that call. Each cmake -E copy rule is now emitted into exactly one submakefile, so the race cannot occur. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix spelling --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Devin <devin@cognition.ai>
1 parent e7ac9e5 commit ce501e0

4 files changed

Lines changed: 24 additions & 1 deletion

File tree

cmake/test/data/TestDeployment/TestChainedAutocoder/CMakeLists.txt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
# Opt out of target/test_autocoder. target/test_chained_autocoder runs test_target_autocoder
2+
# itself (as the first stage of the chain), so applying both targets to this module would
3+
# duplicate the same `cmake -E copy` rule into two sub-makefiles and race under `make -jN`.
4+
# Custom targets are attached inside register_fprime_module, so the flag must be set first.
5+
set(SKIP_TEST_AUTOCODER_TARGET TRUE)
16
register_fprime_module(
27
TestChainedAutocoderModule
38
AUTOCODER_INPUTS
@@ -6,3 +11,4 @@ register_fprime_module(
611
EXCLUDE_FROM_ALL
712
INTERFACE
813
)
14+
unset(SKIP_TEST_AUTOCODER_TARGET)

cmake/test/data/TestDeployment/TestTargetAutocoder/CMakeLists.txt

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
# Opt out of target/test_chained_autocoder. This module exercises the standalone
2+
# target/test_autocoder; without this, the chained target would also run test_target_autocoder
3+
# on it and CMake would emit the same `cmake -E copy` rule into two sub-makefiles, racing
4+
# under `make -jN`. Custom targets are attached inside register_fprime_module, so the flag
5+
# must be set first.
6+
set(SKIP_TEST_CHAINED_AUTOCODER_TARGET TRUE)
17
register_fprime_module(
28
TestTargetAutocoderModule
39
AUTOCODER_INPUTS
@@ -6,4 +12,5 @@ register_fprime_module(
612
EXCLUDE_FROM_ALL
713
INTERFACE
814
)
15+
unset(SKIP_TEST_CHAINED_AUTOCODER_TARGET)
916
target_include_directories(TestBuildAutocoderModule PRIVATE "${CMAKE_CURRENT_BINARY_DIR}")

cmake/test/data/cmake/target/test_autocoder.cmake

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ function(test_autocoder_add_deployment_target MODULE TARGET SOURCES DIRECT_DEPEN
1212
endfunction(test_autocoder_add_deployment_target)
1313

1414
function(test_autocoder_add_module_target MODULE TARGET SOURCES DEPENDENCIES)
15+
# Allow modules to opt out (e.g. TestChainedAutocoderModule, which owns its own
16+
# run of test_target_autocoder via target/test_chained_autocoder).
17+
if (SKIP_TEST_AUTOCODER_TARGET)
18+
return()
19+
endif()
1520
run_ac_set("${MODULE}" "autocoder/test_target_autocoder")
1621
# Use the variable from this run as set by the autocoder
1722
add_custom_target("${MODULE}_test_autocode" DEPENDS "${AUTOCODER_GENERATED_OTHER}")

cmake/test/data/cmake/target/test_chained_autocoder.cmake

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,14 @@ function(test_chained_autocoder_add_deployment_target MODULE TARGET SOURCES DIRE
1212
endfunction(test_chained_autocoder_add_deployment_target)
1313

1414
function(test_chained_autocoder_add_module_target MODULE TARGET SOURCES DEPENDENCIES)
15+
# Allow modules to opt out (e.g. TestTargetAutocoderModule, which exercises the standalone
16+
# target/test_autocoder). See note in test_autocoder.cmake.
17+
if (SKIP_TEST_CHAINED_AUTOCODER_TARGET)
18+
return()
19+
endif()
1520
# Run both autocoders in sequence: target autocoder first, then chained autocoder
1621
run_ac_set("${MODULE}" "autocoder/test_target_autocoder" "autocoder/test_chained_autocoder")
17-
22+
1823
# Use the variable from this run as set by the autocoders
1924
add_custom_target("${MODULE}_test_chained_autocode" DEPENDS "${AUTOCODER_GENERATED_OTHER}")
2025
add_dependencies("${MODULE}" "${MODULE}_test_chained_autocode")

0 commit comments

Comments
 (0)