LT-22816: Rebuild native objects when an included header changes

[johnml1135]

Start here: the end of Bld/_targ.mak, then Bld/Write-NmakeHeaderDependencies.ps1. Two commits, reviewable in order.

What it does

Editing a header in Views, FwKernel, Generic or DebugProcs now rebuilds every object that includes it. Before this, nmake's inference rules made each .obj depend on its own .cpp only. A header edit rebuilt nothing, the build reported success, and Views.dll stayed stale. A class-layout change recompiled some objects and not others, which corrupts memory (hit on #1166 and LT-22674).

Every compile now passes cl /sourceDependencies. For each product, the helper turns those JSON lists into plain "obj" : "header" lines and _targ.mak includes them, so nmake still makes every up-to-date decision.

The unknown: does this slow every build down?

It doesn't. A no-change native build rebuilds 0 objects and takes 21-45 s. The first commit fixes the one thing that would have made it slow: GenerateCellarConstants declared Outputs through an item evaluated before $(dir-fwoutputCommon) was set. The target ran on every build and rewrote CellarConstants.h with identical bytes, which with header tracking would recompile 43 Views objects every time.

Where to look

• Include placement. nmake runs with no target, so its default goal is the first target it reads. The include must stay after build:, as the comment says.
• Objects built before this change have no sidecar. They depend on an always-out-of-date target and rebuild once.
• Generated headers. The edges are evaluated at make time, so a header regenerated earlier in the same build still rebuilds its objects.
• Failure is loud. A helper error stops nmake with U1050, and a path containing # fails with a message.

Not here

• The same evaluation-order bug in the Fragments item (mkall.targets:39-40); it gets its own ticket.
• Real /Yc//Yu PCH builds. No shipped product compiles a PCH through these rules; the PCH handling is covered by a synthetic test only.

Verification

build.ps1 -CommentHygiene -TokenHygiene is clean, and so is a clean native build. TestViews passes 309/0/0. Header-touch checks are in the accordion. Release wasn't built locally. To check it yourself:

1. .\build.ps1 -Project Build/Src/NativeBuild/NativeBuild.csproj
2. (Get-Item Src/views/lib/LayoutCache.h).LastWriteTime = Get-Date
3. Run step 1 again. UniscribeEngine.obj, UniscribeSegment.obj and VwTextBoxes.obj should be the only fresh objects under Obj/Debug/Views.

Next: approve, or tell me to split the CellarConstants fix into its own PR.

---

Reading this a year from now: start here

The working briefs and reviews for this branch stayed outside the tree. This body is the record of why the design looks the way it does. The Jira is LT-22816; the original report is LT-22674 comment 2.

Decisions, and why

• /sourceDependencies, not /showIncludes. It produces structured JSON per translation unit, with no parsing of localized compiler text. It needs MSVC 16.7+, and VS 2022/v143 is already the minimum.
• Full edges, not "stale objects only". nmake compares timestamps at make time. A helper that judges staleness itself does so before the build has generated its headers.
• Per product, at the end of _targ.mak. All six makefiles that include _rule.mak include _targ.mak exactly once, after defining build:. That gives one helper run per nmake invocation, whatever entry point started it.
• No cache. The helper rereads the sidecars every time (0.4-1.2 s per product) and rewrites the include only if its content changed. Getting cache invalidation right cost more than the time it saved.
• Vendored Include/ (ICU) headers are tracked. Views gets 13,307 edges. Excluding them would leave objects stale after an ICU bump.
• Only repository headers are tracked, and missing ones are dropped, so deleting or renaming a header can't produce "don't know how to make".
• The include is written through a temp file and File.Replace, so nmake never reads a partial file.

Paths not taken

• Every object depends on every header. Correct, but any header edit rebuilds a whole product.
• Moving the native libraries to .vcxproj, which tracks includes itself. The largest change, and it reorders FieldWorks.proj.
• The first implementation emitted edges only for objects the helper judged stale, and cached that judgement. Review showed three problems:
  • It missed headers generated later in the same build.
  • It skipped products built without DebugProcs first, because the gate compared against TestViews while the makefile sets testViews.
  • It had cache-invalidation gaps.
    Its performance motivation (a 122 s no-change build) turned out to be the CellarConstants rewrite recompiling 46 objects, not the size of the include.
• Placing the include at the top of _rule.mak. That made the first object in the include nmake's default goal, so a build compiled that one object and nothing else. The first implementation had this too; its stale-only edges hid it.

Evidence

Debug, build.ps1 -Project Build/Src/NativeBuild/NativeBuild.csproj, comparing object timestamps before and after:

Check	Objects rebuilt
No-change build, twice, and again after the rebase	0
Touch Src/views/lib/LgUnicodeCollateInit.h	LgUnicodeCollater.obj
Touch Src/views/lib/LayoutCache.h	UniscribeEngine, UniscribeSegment, VwTextBoxes, then Views.dll relinks
Touch Src/views/Main.h	43 of 46 Views objects
Delete VwEnv.obj.source-dependencies.json	VwEnv.obj once, then 0
Touch Src/Kernel/CellarConstants.vm.h	header regenerated, 43 objects in the same build, 0 the next

Before the mkall.targets fix, a detailed MSBuild log showed Output file "/CellarConstants.h" does not exist, and three consecutive builds rewrote the file with the same SHA-256.

The helper produces identical output under Windows PowerShell 5.1 and PowerShell 7, and exits 1 on error. A synthetic product covers a missing sidecar, a missing PCH (producer and consumer forced), $ in a path (doubled), and # in a path (rejected).

Two independent read-only reviews were run: one on the first design and one on the final diff. Every valid finding is addressed.

Preflight review details

Code Review Summary

Branch: LT-22816-native-header-deps
Base: main
Date: 2026-09-30
Review model: Claude Opus 5.5 (synthesis); two independent luna-6 (gpt-6-luna, xhigh) read-only reviews
Files changed: 4

Overview

Purpose (author): implement generated header dependencies for the nmake native build
(LT-22816), so a header edit rebuilds every object that includes it, and explain the
rationale.

The nmake inference rules in Bld/_rule.mak made each object depend only on its own
.cpp. Every compile now writes a cl /sourceDependencies sidecar. At the end of
Bld/_targ.mak, Bld/Write-NmakeHeaderDependencies.ps1 turns each product's sidecars
into plain obj : header edges, and nmake makes every up-to-date decision.

Making this work exposed a separate bug that predates the branch: GenerateCellarConstants
declared its Outputs through an item evaluated before $(dir-fwoutputCommon) was set. The
target ran on every build and rewrote CellarConstants.h with identical bytes, which with
header tracking would recompile 43 Views objects on every build. It's fixed in its own
commit.

The first implementation (luna-6) decided which objects were stale in the helper and
cached the answer. The first review showed that design missed headers generated later
in the same build, and missed builds that skip DebugProcs. Its performance motivation was
also confounded by the CellarConstants bug. It was replaced with full edges per product.
That exposed a latent bug in both versions: nmake is invoked with no target, so an
include placed before build: became the default goal. The include now sits at the end
of _targ.mak.

Contract/API Changes

None to product code, COM or exported surfaces. Build-system contract: native makefiles
now require MSVC 16.7+ (/sourceDependencies); VS 2022/v143 is already the minimum.
Each native product writes *.obj.source-dependencies.json and header-dependencies.mak
under Obj/<Config>/<Product>/.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

• Objects built before this change never gain sidecars (fixed during review:
  an object with no sidecar depends on an always-out-of-date target and rebuilds once)
• Staleness judged at DebugProcs parse time misses headers generated later in the
  build (fixed during review: full edges, evaluated by nmake at make time; verified by
  touching CellarConstants.vm.h, which rebuilt 43 objects in the same build)
• Interrupted include write can be accepted by the cache (fixed during review:
  the cache was removed; the include is written through a temp file and File.Replace)
• A missing PCH drops the consumer's edge (fixed during review: producer and
  consumers are forced; covered by a synthetic sidecar test)
• Include before build: became nmake's default goal (found while verifying,
  fixed: include moved to the end of _targ.mak)

Minor - Consider

• Cache ignored object timestamps (fixed: no cache)
• Direct Views.mak or test-makefile runs skipped the refresh; testViews casing
  mismatch (fixed: runs for every product, with no product-name gate)
• ^# escaping is wrong inside quotes (fixed: a path containing # fails the
  build with a clear message; $ is doubled)
• A lost concurrent first-write race throws (fixed: falls back to Replace)
• Help text named _rule.mak (fixed)
• _rule.mak had mixed line endings (fixed: CRLF working copy, LF in index as
  the repo stores it)
• Avalonia detail views lose row filters; LibPalaso rolled back (not in this
  branch: artifacts of a two-dot git diff origin/main..HEAD after main moved; the
  merge-base diff has 4 files)
• Pre-existing: Fragments item in mkall.targets:39-40 uses $(dir-fwdistfiles)
  before Setup sets it (out of scope; author chose to file a separate Jira ticket)

Required Validation / Evidence

• build.ps1 -CommentHygiene -TokenHygiene: clean, 0 errors.
• build.ps1 -Clean of the native project: succeeds from scratch.
• test.ps1 -SkipManaged -TestProject TestViews: 309/0/0.
• No-change native builds: 0 objects rebuilt (21-45 s), including on the rebased branch.
• Touch LgUnicodeCollateInit.h: rebuilds LgUnicodeCollater.obj only.
• Touch LayoutCache.h: rebuilds exactly UniscribeEngine, UniscribeSegment and
  VwTextBoxes, and relinks Views.dll.
• Delete the VwEnv sidecar: VwEnv.obj rebuilds once, then never again.
• Touch CellarConstants.vm.h: the header regenerates and 43 objects rebuild in the
  same build; the next build rebuilds 0.
• Helper under Windows PowerShell 5.1 and PowerShell 7: identical output; exits 1 on
  error, surfaced by nmake as U1050.
• Real /Yc//Yu PCH path: no shipped product compiles a PCH through these rules;
  covered only by a synthetic sidecar test.
• Release configuration not built locally; CI builds clean.

Positive Observations

• nmake keeps every up-to-date decision; the helper only emits edges, with no cache and
  no staleness logic.
• Vendored Include/ (ICU) headers are tracked deliberately: an ICU bump now rebuilds
  its consumers. Views has 13,307 edges; no-change native builds take 21-39 s. That
  wasn't measured against main under identical conditions.
• The CellarConstants fix follows the pattern CopyCellarBaseConstants already uses, and
  the warning comment in SetupInclude.targets:319.

Interview Notes

• Author asked for solution 1 (generated dependencies) over the coarse
  all-headers guard and a .vcxproj move.
• Author chose a luna-6 re-review of the final design before posting (done; its valid
  findings are fixed above).
• Author chose to file the Fragments evaluation-order bug as a separate ticket.
• Jira: LT-22816. Developer-only, so no tester action is needed.

Suggested Review Focus

• Bld/_targ.mak tail: helper call, !INCLUDE, and why it must be last.
• Write-NmakeHeaderDependencies.ps1: the force target for missing sidecars and PCH.
• Build/mkall.targets: GenerateCellarConstants Inputs/Outputs.

🤖 Generated with Claude Code

---

This change is 

[github-actions]

NUnit Tests

    1 files  ±0      1 suites  ±0   13m 3s ⏱️ -16s
6 313 tests ±0  6 228 ✅ ±0  85 💤 ±0  0 ❌ ±0 
6 322 runs  ±0  6 237 ✅ ±0  85 💤 ±0  0 ❌ ±0 

Results for commit 6fad4c1. ± Comparison against base commit 506c2a6.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.02%. Comparing base (506c2a6) to head (6fad4c1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1168      +/-   ##
==========================================
- Coverage   39.03%   39.02%   -0.01%     
==========================================
  Files        1522     1522              
  Lines      353035   353035              
  Branches    40744    40744              
==========================================
- Hits       137792   137770      -22     
- Misses     185926   185947      +21     
- Partials    29317    29318       +1     

see 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.