[kadircanyildirm-crypto]

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

---

Change Description

An overridden configuration destination is written twice per CMake run: once by the module that supplies the original file, once by the module that supplies the override. Each of them writes while the file holds the other one's content, so ONLY_IF_DIFFERENT can never skip either copy. The destination's timestamp moves on every configure, and everything including it is rebuilt.

Both paths now record which source should end up at the destination instead of copying straight away. A single pass, deferred to the end of the top-level directory, writes each destination once, at a point where every override is known. A destination that already holds the winning content is left alone.

Rationale

The write cannot simply be skipped by whoever loses. When the original is processed its overrides have not been registered yet, and they cannot be: an override is rejected outright if the file it overrides has not been seen, so the module supplying the original is always processed first. Any decision made at that moment relies on the previous configure, which cannot tell "the override is still there" apart from "the override was removed". Each variant I worked through along those lines would leave a removed override stale in the build cache, which is the same class of problem as the one being fixed. Deferring the write until all modules are registered is what actually removes the ambiguity.

Worth noting because I got it wrong at first: this is two writers per CMake run, not per build. The info-cache sub-build is a separate CMake process that also materialises configuration into the main build cache, and it runs before the main configure does its own pass. So an overridden destination is written four times per top-level configure, not two. Both processes run this same code, so the single fix covers both.

Testing/Review Recommendations

On devel @ a24a4c8, TestConfigDeployment, which declares four overrides:

	before	after
Configuration destinations whose timestamp moves on a no-op re-configure	4	0
The same, after running the info-cache sub-build by hand	4	0
Objects recompiled by a no-op re-configure	15 of 16	0

TestDeployment, which declares no overrides, moves nothing either way — the four that move map one-to-one onto the four declared overrides.

test_override_survives_reconfigure is the regression test. It fails on devel with only the test applied, which is the run I would check first — #3933 fixed a nearby build-system bug without one, and #3929 had to be opened to add it afterwards.

	result
test_config.py with this change	8 passed (was 7)
test_config.py with the fix reverted, test kept	the new test fails
pytest -s from cmake/test, the command the CI job runs	43 passed (17m04s)

Removing an override is worth reviewing specifically, since that is where the simpler fixes break. Dropping DpCfg.hpp from the project's CONFIGURATION_OVERRIDES and re-configuring restores F Prime's original file byte for byte, because the recorded map is rebuilt from scratch on every configure.

Formatting checked with black==21.6b0, the version python-format.yml pins.

Future Work

cmake_minimum_required in the top-level CMakeLists.txt says 3.18, but config_assembler.cmake already used file(COPY_FILE) (3.21) and cmake_path() (3.20) before this change, so the real floor was already higher than the declared one. cmake_language(DEFER) used here needs 3.19 and is therefore covered. The stale declaration seems worth its own issue rather than a change smuggled in here.

AI Usage (see policy)

I used Claude Code. It helped me trace where the duplicate write comes from, wrote this change and the test, and drafted this description. The work is limited to cmake/config_assembler.cmake and the configuration test module.

The output was heavily modified rather than used as-is. The first designs had the losing writer skip its copy; working through them showed each would leave a removed override stale in the build cache, so they were dropped in favour of deferring the write. Every number above comes from a run I did myself.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

Coverage report — base devel

Overall (line): 83.66% → 83.66% (+0.00)
Regression threshold: 0.50% (line).

Regressions

Module	Line	Δ	Function	Δ	Branch	Δ
Svc/OsTime	70.00	-26.00	83.33	-16.67	60.98	-31.70
Fw/Buffer	80.41	-4.92	89.47	-1.83	58.62	-4.54

Modules changed

Module	Line	Δ	Function	Δ	Branch	Δ
Svc/OsTime	70.00	-26.00	83.33	-16.67	60.98	-31.70
Fw/Buffer	80.41	-4.92	89.47	-1.83	58.62	-4.54
Svc/Ccsds/CfdpManager	88.86	-0.10	96.00	+0.00	73.36	-0.27
Svc/BufferAccumulator	88.16	-0.08	94.12	+0.00	74.75	+0.00
Svc/ComAggregator	95.65	-0.06	93.75	+0.00	96.00	+0.00
Svc/TlmPacketizer	92.26	-0.05	100.00	+0.00	78.00	-0.36
Svc/PrmDb	92.75	-0.03	89.47	-1.44	87.98	+0.42
Svc/ComQueue	97.95	-0.01	100.00	+0.00	88.89	+0.00
Os/Generic	89.15	+0.03	88.16	+0.00	73.49	+0.22
Svc/Ccsds/CfdpManager/Types	79.88	+0.07	96.69	+0.00	60.64	+0.05
Svc/FileDownlink	83.94	+0.09	91.49	+0.00	71.60	+0.00
Drv/Ip	45.56	+0.17	57.38	+0.00	27.17	-0.13
Os	21.08	+0.17	23.26	+0.04	18.81	+0.12
Fw/DataStructures	98.04	+0.26	97.14	+0.00	82.60	+0.37
Os/Generic/Types	92.14	+0.35	92.86	+0.00	74.77	+0.00
Os/Posix	64.56	+0.40	85.26	+0.00	45.71	+0.23
Svc/TlmChan	80.45	+0.78	85.71	+0.00	67.77	-0.23
Svc/FileWorker	90.57	+0.85	100.00	+0.00	86.06	+0.68
Svc/DpCatalog	76.22	+2.05	96.97	+0.00	64.98	+2.03
Svc/EventManager	93.06	+2.65	88.89	+0.00	90.11	+0.86
Svc/Ccsds/SpacePacketFramer	97.37	+11.01	100.00	+0.00	88.00	+18.77
Svc/FprimeFramer	100.00	+13.51	100.00	+0.00	95.45	+23.23

Modules without UTs

CFDP/Checksum/GTest, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports/DataTypes, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Obj, Fw/Port, Fw/Prm, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Ccsds/Types, Svc/FatalHandler, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComCcsdsSdls, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/DpCompression, Svc/Subtopologies/FileHandling, Svc/Subtopologies/FileHandlingCfdp, Svc/Subtopologies/FileHandlingCfdp/FileHandlingCfdpConfig, TestDeploymentsProject/Ref/DpDemo, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestConfigDeployment, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestLinkDepends, cmake/test/data/TestDeployment/TestRelative, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent

[LeStarch]

@kadircanyildirm-crypto would you address the merge conflict?

[lestarch-autobot]

[Test Quality] must fix Override-removal path has no automated regression test.

The PR description identifies removing an override as exactly the case where the simpler fixes break (a removed override left stale in the build cache), yet it was only verified manually. A future regression there would silently ship stale configuration content. Add a test that drops DpCfg.hpp from the project's CONFIGURATION_OVERRIDES, re-configures, and asserts the destination reverts to F Prime's original file byte for byte.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Maintainability] could fix maint-structural-hazard: two parallel global lists whose index alignment is the invariant.

FPRIME_CONFIG_DESTINATIONS/FPRIME_CONFIG_SOURCES only work while every edit touches both at the same index; a future change that appends/removes one without the other silently maps destinations to the wrong sources. A keyed property per destination (e.g. set_property(GLOBAL PROPERTY "FPRIME_CONFIG_SOURCE_${DESTINATION}" "${SOURCE}") plus a single destinations list) removes the FIND/REMOVE_AT/INSERT dance and the alignment invariant entirely.

[lestarch-autobot]

[Maintainability] could fix maint-misleading-comment: multi-paragraph counterfactual rationale in the function header.

This paragraph describes what the code would do under a rejected design, not what it does; it will silently go stale if the flush strategy changes, and the same rationale already lives in the PR/commit message. One line ("copies are deferred so an overridden destination is written once, by the winning source") carries the needed context.

[lestarch-autobot]

(Anchored above the offending lines; the diff does not include lines 180-192.)

[Maintainability] future work maint-misleading-comment: header of fprime_internal_get_configuration_destination documents a CONFIG_NAME argument the function does not take (actual: MODULE_NAME, NEW_CONFIG_NAME) and omits the DESTINATION_MODULE/DESTINATION_OVERRIDE/DESTINATION_DIRECTORY values it sets in the caller. Preexisting, but this PR's new code now depends on those undocumented outputs, so the next editor will read a wrong contract.

[lestarch-autobot]

(Anchored on the new test; the diff does not include line 2.)

[Maintainability] future work maint-misleading-comment: the module header still reads test_basic.py: Basic CMake tests but the file is test_config.py holding configuration tests. Preexisting; a reader grepping for the basic tests lands here.

[lestarch-autobot]

[Design] could fix Deferred flush leaves the destination holding the losing/old content for the rest of the configure pass.

On a fresh configure (or when a source changes), the destination keeps the original/previous content until the end-of-top-level flush. This is safe only while nothing reads configuration content at configure time; I traced the info-cache sub-build (its fpp targets run at build phase, after its own flush) and the include-path check (paths only), and found no configure-time content reader — but I could not verify this exhaustively. Consider stating this invariant in the function comment so a future configure-time reader does not silently see the losing content.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Design] future work Declared CMake minimum (3.18) does not cover this file's real floor.

cmake_language(DEFER) needs 3.19, and this file already used file(COPY_FILE) (3.21) and cmake_path() (3.20) before this PR, so the effective floor was already above the declared cmake_minimum_required(VERSION 3.18). Preexisting, not widened here; the PR description already proposes a separate issue — recording so it is tracked.

[lestarch-autobot]

[Documentation] could fix Build-system docs do not describe the new deferred single-write behavior for overridden configuration files.

docs/user-manual/framework/configuring-fprime.md and docs/user-manual/build-system/ describe CONFIGURATION_OVERRIDES but never say when overridden destinations are materialized; this PR changes that to a single deferred write at the end of the top-level configure, and unchanged destinations are no longer rewritten. A short note in configuring-fprime.md would explain why no-op re-configures no longer rebuild dependents. The generated CMake API reference picks up the new #### doc blocks automatically, so no action is needed there.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Documentation] future work Build-system SDD (cmake/docs/sdd.md) does not cover the configuration-assembly/override mechanism this PR redesigns.

The SDD predates config_assembler.cmake and never described override handling, so the gap is preexisting, not introduced here. The new deferred-flush design (global-property source map plus cmake_language(DEFER) at the top-level directory) is now load-bearing build-system behavior worth capturing when the SDD is next updated.

[lestarch-autobot]

[Correctness] could fix correctness-other: overridden destination holds the original (or previous configure's) content until the deferred flush runs.

Trigger: any configure-time logic that reads an overridden destination's bytes between module registration and the end of top-level directory processing (e.g. a project CMakeLists.txt doing file(READ)/configure_file on a config header). Before this PR the override content landed at registration time; now it lands only at the flush. No in-repo configure-time reader of these files exists today, so the consequence is only reachable from project code outside this repository.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[Correctness] future work correctness-other: cmake_language(DEFER) requires CMake 3.19, but the top-level cmake_minimum_required still declares 3.18.

Trigger: configuring with CMake 3.18 (the declared minimum). Consequence: configure aborts with an unknown-subcommand error instead of a clear version message. The effective floor was already 3.21 before this PR (file(COPY_FILE), cmake_path() in this same file), so this PR does not widen it — flagged as preexisting per the PR's own Future Work note; the declared minimum should be raised in a follow-up.

[lestarch-autobot]

Automated review summary (run 1)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	0	0	0	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	0	0	0	0	0	Go
Documentation Currency	0	0	1	1	2	Go
Design	0	0	1	1	2	Go
Architecture	0	0	0	0	0	Go
Test Quality	1	1	0	0	2	No-Go
Correctness	0	0	1	1	2	Go
Maintainability	0	1	2	2	5	Go
CI safety	—	—	—	—	—	Go
Totals	1	2	5	5	13	No-Go

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Outstanding must-fix items (1)

Test Quality

• Override-removal path has no automated regression test — #5576 (comment)

Merge readiness

Merge readiness: No-Go — Test Quality has 1 outstanding must-fix item (missing override-removal regression test).

Agents that did not run on this PR

• None — all registered reviewers ran.

---

All stations report in; one item on the checklist before this build is go for launch.

[kadircanyildirm-crypto]

Rebased onto devel. The conflict was additive — #5589's new function and mine landed at the same spot; both are kept and theirs is unchanged. cmake/test/src/test_config.py passes 9/9.

[LeStarch]

@kadircanyildirm-crypto would you format python and consider the missing test?

[kadircanyildirm-crypto]

All four red checks are external-repo jobs, and none of them traces back to this change.

• fprime-examples (run / Build, run / Unit Tests) — FlightExamples/CustomFraming/DecafDeframer/DecafDeframer.cpp:101 calls data.advance(..). Fw::Buffer::advance landed on devel in ceee420 on Aug 11; this branch's base is Aug 10, so the method does not exist here yet.
• YAMCS Reference (Test) — test_cdh_core.py:77, assert 81 == (79 + 1). The same assertion fails on devel itself in run 31506686144 (26d4611), which does not carry this change.
• Zephyr — fails inside the get_pr_branch setup action with curl: (60) SSL certificate problem: self-signed certificate, before anything is configured or built.

The branch is 24 commits behind devel — happy to rebase if you would like a fresh run.

AI use per AI_POLICY.md. Tool: Claude Code. Type of assistance: reading the four failing job logs and drafting this comment. Level of modification: the run IDs, file paths and line numbers above are quoted from those logs and checked against the repositories.