Skip to content

Fix CMake unit test race condition#5093

Merged
LeStarch merged 9 commits intonasa:develfrom
JPL-Devin:devin/1777933774-fix-cmake-chained-autocoder-race
May 5, 2026
Merged

Fix CMake unit test race condition#5093
LeStarch merged 9 commits intonasa:develfrom
JPL-Devin:devin/1777933774-fix-cmake-chained-autocoder-race

Conversation

@thomas-bc
Copy link
Copy Markdown
Collaborator

Related Issue(s)
Has Unit Tests (y/n)
Documentation Included (y/n)
Generative AI was used in this contribution (y/n)

Change Description

We've seen intermittent failures in the CMake UTs. Devin says this should fix it.

AI Usage (see policy)

Devin. See JPL-Devin#40

devin-ai-integration Bot and others added 4 commits May 4, 2026 22:29
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>
Co-Authored-By: thomas.boyer.chammard <thomas.boyer.chammard@jpl.nasa.gov>
Co-Authored-By: thomas.boyer.chammard <thomas.boyer.chammard@jpl.nasa.gov>
Added manual dependency to prevent race condition between test targets.
@thomas-bc thomas-bc requested a review from LeStarch May 4, 2026 23:01
thomas-bc and others added 2 commits May 4, 2026 16:03
… 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.
Copy link
Copy Markdown
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't right either. A chained AC should use input from the first autocoder, so creating input files defeats the purpose of the test.

devin-ai-integration Bot and others added 2 commits May 5, 2026 17:44
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>
LeStarch
LeStarch previously approved these changes May 5, 2026
Copy link
Copy Markdown
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this is the appropriate fix in this scenario.

@LeStarch LeStarch merged commit ce501e0 into nasa:devel May 5, 2026
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants