Anchor dev code and data directories on the running source tree - #1093
Anchor dev code and data directories on the running source tree#1093johnml1135 wants to merge 2 commits into
Conversation
FwDirectoryFinder found the dev DistFiles by assuming the running assembly sat exactly two levels below the tree root, and then let HKCU RootCodeDir/RootDataDir override whatever it found. So a build run from any other output folder missed DistFiles entirely, and every worktree read the DistFiles named by the shared registry value, which belongs to whichever tree last ran the build or the launch script. FindDevDistFiles now walks up from the running assembly to the directory holding both DistFiles and FieldWorks.sln, and that tree wins over the registry. An installed FieldWorks has no solution file beside it, so it keeps reading the registry as before. Set FW_USE_REGISTRY_DIRS to opt a dev build back into the registry. Tests cover the walk from Output/Debug, from an architecture subfolder, and from a project bin folder; the installed case; and the precedence of the source tree over a registry value naming another worktree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
+ Coverage 38.31% 38.35% +0.04%
==========================================
Files 1507 1507
Lines 350524 350651 +127
Branches 40288 40307 +19
==========================================
+ Hits 134302 134496 +194
+ Misses 186990 186919 -71
- Partials 29232 29236 +4
🚀 New features to boost your workflow:
|
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
The diagnosis is right and the installed-build safety argument holds up — I checked it
rather than taking it on trust (see item 6). The "Paths not taken" section is the most
useful part of the body: rejecting the Output/<Configuration> idea with actual file
counts, and rejecting the overlay because CodeDirectory returns one string that ~100
sites Path.Combine onto, are both the right calls argued the right way. Worktree
behaviour is correct too — the walk stops at the innermost match, so an assembly under
fw/.claude/worktrees/X/Output/Debug resolves X/DistFiles, which is the point.
The through-line of what follows: this change is correct when it works and invisible when
it does not. Every failure mode below degrades to "silently reads the wrong tree" rather
than to an error, which is the same class of bug the PR was written to eliminate.
1. The walk-up is a no-op on any path containing a space.
FwDirectoryFinder.cs:330 derives the start directory with
FileUtils.StripFilePrefix(Assembly.GetExecutingAssembly().CodeBase). I loaded SIL.Core.dll
and called it:
StripFilePrefix('file:///C:/My%20Repos/fw/Output/Debug/FwUtils.dll')
=> C:/My%20Repos/fw/Output/Debug/FwUtils.dll
It strips the scheme but does not unescape. Every Directory.Exists in the new walk then
misses, FindDevDistFiles returns null, and resolution falls back to the registry — the
exact stale-worktree bug this PR exists to fix, with no diagnostic.
This is the PR's to fix rather than inherited debt, because the same class already gets it
right fifteen lines away: ExeOrDllDirectory (:417-418) uses Uri.UnescapeDataString, as
does InitializeFwRegistryHelperAttribute.cs:43-47. The probe was promoted to authoritative
without being made correct.
Please have GetDevDistFilesPath call ExeOrDllDirectory instead of re-deriving the path a
second way, and add a test case whose fixture path contains a space — nothing currently
exercises that.
(Worth noting for item 3: FwUtils.cs:219 uses Assembly.GetExecutingAssembly().Location,
which is already a plain filesystem path and has no escaping problem. CodeBase is
presumably why this class does what it does — shadow-copying under test runners — so
ExeOrDllDirectory is still the right fix here, but Location is worth knowing about.)
2. Please drop FW_USE_REGISTRY_DIRS.
To be clear about what I am not saying: it is genuinely distinct from
FW_ROOT_CODE_DIR / FW_ROOT_DATA_DIR, and I looked hard at whether it duplicated them. It
does not. Those pin a specific path; this defers to whatever the registry currently says, and
there is no value you can set the pair to that means "the registry". Having managed code
honour that pair would also be a CI change — Build/Agent/Setup-FwBuildEnv.ps1:104-108
already sets both on every agent, and managed FwDirectoryFinder ignores them today — which
does not belong in a dev-ergonomics PR. And honouring them would apply to installed builds
too, where a stale exported variable would silently win. So the mechanism you chose is the
narrower and safer one.
The problem is discoverability. Nobody who needs it will know it exists. It is documented
only in the source, it is not named in the skill docs this PR updates, and a developer
hitting the problem it solves has no path to finding it. Unused configuration still has to be
maintained, tested and reasoned about, and it is one more environment variable in a space
that already has two honoured by a different layer with different semantics. If the need
turns up in practice, add it then, with documentation.
3. Decide, explicitly, whether FindDevDistFiles is the seam.
The PR calls the fixed-depth probe "a second, independent defect". FwUtils contains three
implementations of it and this fixes one:
FwDirectoryFinder.cs:328— fixed here.FwUtils.cs:215-241TryGetDevIcuDataDir()— stillPath.Combine(assemblyDir, "..", "..", "DistFiles").StringTable.cs:71-113— a third idiom, stripping at the lastoutputsegment.
Plus FwDirectoryFinder.SourceDirectory (:429-450), eleven lines below the new method,
still does two hard-coded GetDirectoryName hops and throws ApplicationException when they
miss.
The concrete result: after this PR a build run from Output/Debug/x64 — a layout
FindDevDistFiles_InsideSourceTree_FindsTreeDistFiles explicitly claims support for —
resolves code and data correctly but still fails to find dev ICU data and still throws from
SourceDirectory. One file, three answers.
I am not asking you to fix all of them. I am asking for a decision per site, stated: route it
through FindDevDistFiles, or keep it separate and say why. You made the method public
for reuse, so either it is the seam or it is not.
SourceDirectory is the sharpest case, being in this file and contradicting the new test's
own claim. ICU deserves extra thought rather than a default answer — it starts from
Location rather than CodeBase, it is a best-effort catch-all returning null, and it
runs on the CustomIcu.InitIcuDataDir() bootstrap path, so taking a new dependency on
FwDirectoryFinder there is an initialisation-order question, not just a refactor. It may
well be right to leave it alone; I would rather that be a conclusion than an omission.
4. ProjectsDirectory is not untouched.
The body says it is. The code is; the behaviour is not. FwDirectoryFinder.cs:502 is
GetDirectory(ksProjectsDir, Path.Combine(DataDirectory, ksProjects)) — its default derives
from DataDirectory, which this PR changed. On any machine where the HKCU/HKLM ProjectsDir
value is absent — a fresh dev box, a CI agent, a container — the projects directory moves
from %ProgramData%\SIL\FieldWorks\Projects to <tree>\DistFiles\Projects.
Where the registry value is set nothing changes, which is most established dev machines, so
this is narrow. But ProjectsDir resolution touches installer expectations and test-fixture
scratch directories, and a reviewer reading "untouched" will not check. Please correct the
claim and state plainly what happens where the registry value is absent.
5. Two problems in CodeAndDataDirectory_PreferSourceTreeOverRegistry.
RegistryKey.SetValue(name, null) throws ArgumentNullException. If the fixture setup at
FwDirectoryFinderTests.cs:38-39 ever stops seeding those values, this test fails with an
ArgumentNullException from the finally rather than the assertion that actually broke. Your
own preflight caught this; please guard it.
More importantly, expectedDir is computed as UtilsAssemblyDir/../../DistFiles — re-encoding
the exact fixed-depth assumption this PR removes. It passes today and misleads tomorrow, and
it sits in the file that is meant to be the evidence for the change. Derive it the way the
production code now derives it.
6. Installed-build safety: verified, with one question.
I checked this independently and you are right. Build/Installer.targets:133,149 harvests
$(fwrt)\DistFiles\**\* and copies with %(RecursiveDir), flattening the contents into the
staged app folder, so no DistFiles directory name survives; the only root-level repo file
installed is License.htm (:134), so no FieldWorks.sln either; and
FLExInstaller/Overrides.wxi:11-12 points RootCodeDir at APPFOLDER. The marker conjunction
cannot occur in an install tree.
The question: does anything in install validation or patch staging run the packaged binaries
from a path inside the checkout? If so, the probe would prefer the tree's DistFiles over
the staged payload and could mask a packaging defect — a validator passing because it read the
source tree is a bad failure to have. Related: dropping FW_USE_REGISTRY_DIRS (item 2) removes
the only lever such a flow could have pulled, so it is worth answering this before dropping it.
7. Make the marker's disappearance loud.
ksSolutionFilename = "FieldWorks.sln" (:334) is the only thing distinguishing a source tree
from an install. Rename or remove the solution — plausible in an SDK-style consolidation — and
every dev build silently reverts to the registry with no diagnostic.
Please add a test that fails loudly if the marker stops existing at the repo root. That turns
"someone renamed the solution" from a confusing dev-environment regression into a red build,
which is the cheapest possible guard on the whole mechanism.
8. TidyRootDir's doc comment.
FwDirectoryFinder.cs:320-323 reads "Strips the trailing separator that hundreds of callers
would otherwise pass on to Path.Combine". It names its consumers, who change silently, and it
describes what the method does to them rather than its own contract. The pre-existing // said
the same thing, but this PR promoted it to a doc comment, so it is fair game.
Something closer to: returns the directory without a trailing separator, except at a drive root,
where Path.Combine requires one.
9. Four smaller comment items.
FindDevDistFiles<remarks>(:344-348): "Walking up to the tree root, rather than
assuming a fixed depth" narrates what the code no longer does; the rest enumerates three
folder layouts, restating the[TestCase]list.<param name="startDirectory">(:349) restates the name and type only — omit it.ksUseRegistryDirsVariable(:337): "let the registry name the directories again" implies a
prior state the reader cannot see. Moot if item 2 lands.FindDevDistFiles_InsideSourceTree_FindsTreeDistFiles's doc restates the method name and
narrates removed behaviour.
Credit where it is due: the <summary> null contract ("or null if it lies outside a source
tree") is exactly the kind of tag worth keeping, and CreateFakeSourceTree's doc explains
fixture shape rather than mechanics. The comments in this file are better than most.
10. Two documentation items.
.claude/skills/fieldworks-winapp/SKILL.md:123 — "Current builds anchor on their own source
tree, so this now matters mainly for older builds" — is temporal framing that will read as
wrong within a release or two. Say what is true rather than what recently changed.
And an accuracy note on the body's reasoning, which is otherwise a good record: it attributes
the shared registry slot to Build/mkall.targets target setKeysInHKCU, but mkall.targets:277
carries a REVIEW (Hasso) 2026.03 comment saying those targets "appear unused by the
recently-modernized build process". The likelier live writers are the MSI
(FLExInstaller/Overrides.wxi:11-12) and Resolve-FieldWorksDevRegistry.ps1:54-55. The defect
is real either way — it would just be a shame for the record to name the wrong culprit.
This review was assisted by Claude Fable 5.
This comment has been minimized.
This comment has been minimized.
The walk-up never ran on any repo path containing a space. The start directory came from StripFilePrefix, which removes the file:// scheme without unescaping, so %20 survived, every Directory.Exists missed, and resolution fell back to the machine registry with no diagnostic -- the stale-worktree bug this change exists to remove. Verified by calling it: StripFilePrefix leaves C:/My%20Repos/fw/Output/Debug/FwUtils.dll intact. Derive the directory through AssemblyDirectoryFromCodeBase, which unescapes the URI, and drive the new test through that conversion rather than calling the walk with an already-clean path. Removing the unescaping turns that test red; calling the walk directly would not have. Route SourceDirectory through the same walk. It did two hard-coded parent hops and threw, so a build from Output/Debug/x64 resolved code and data but still failed here -- contradicting the layout the new test claims support for. Leave the ICU and StringTable probes alone, with a reason at each site. ICU runs on the CustomIcu.InitIcuDataDir bootstrap path, where depending on FwDirectoryFinder is an initialisation-order question rather than a refactor (LT-22768). StringTable walks to the last "output" segment and feeds a different fallback chain (LT-22769). Drop FW_USE_REGISTRY_DIRS. It had no caller and no documentation outside the source, so nobody who needed it would find it. Fix two problems in the registry test. It computed its expectation as ../../DistFiles, re-encoding the fixed depth this change removes, and its cleanup called SetValue(name, null), which throws and would have masked whichever assertion actually failed. Add a test that fails if FieldWorks.sln stops being at the repo root. It is the only thing telling a source tree from an install, and three FwAvalonia fixtures also walk up to it, so a rename would break four things at once with no diagnostic. Correct TidyRootDir's doc comment, which described what it does to its callers rather than its own contract, and trim the remarks that narrated removed behaviour.
2b1098f to
94145a6
Compare
|
All ten items addressed in 1. The walk-up on a path with a space — confirmed by running itExactly as you said. (One correction: it is I did not simply call 3. Decided per site
2.
|
3 participants
A Debug build now reads the
DistFilesof the source tree it was built in. Before this, it read whichever tree the shared registry valueHKCU\SOFTWARE\SIL\FieldWorks\9\RootCodeDirhappened to name — on the machine that prompted this, a build in the main repo was loading parts, layouts, and configuration from an unrelated worktree under.tmp/worktrees/.The reviewer's question here is "what did this break for installed FieldWorks?", and the answer is nothing: the new probe only matches a directory that has both
DistFilesandFieldWorks.slnbeside it, which no install has. The question worth your time is whether making the source tree outrank the registry is the right default.Where to look
CodeDirectory/DataDirectorynow return before consulting the registry when the running assembly is inside a source tree — the deliberate behavior change. There is no opt-out:FW_USE_REGISTRY_DIRSwas dropped in review as undiscoverable and unused.FindDevDistFileswalks up instead of assuming<assembly>/../../DistFiles, so it also works fromOutput/Debug/x64and from a project's ownbinfolder. Three layouts pinned by tests.DistFilespresent, no solution file, resultnull.TidyRootDiris extracted fromGetDirectoryso both paths normalize trailing separators identically — the ~100Path.Combinecallers see no difference.ProjectsDirectory's code is untouched, but its behavior is not, and the earlier claim that it was untouched was wrong.FwDirectoryFinder.cs:541isGetDirectory(ksProjectsDir, Path.Combine(DataDirectory, ksProjects)), so its default derives fromDataDirectory, which this PR changes. Where the HKCU/HKLMProjectsDirvalue is set, nothing changes — which is most established dev machines. Where it is absent — a fresh dev box, a CI agent, a container — the projects directory moves from%ProgramData%\SIL\FieldWorks\Projectsto<tree>\DistFiles\Projects.Deliberately not here
Src/FwParatextLexiconPlugin/ParatextLexiconPluginDirectoryFinder.cskeeps its registry-only resolution; it runs inside Paratext, never from a source tree.RootCodeDiroverride stops applying.Build/mkall.targetsstill writesRootCodeDir/RootDataDir; a source-tree build now ignores them.Verification
.\build.ps1 -CommentHygiene -BuildTestssucceeded (0 warnings, 0 errors; comment-hygiene clean)..\test.ps1 -CommentHygiene -SkipNative -TestProject Src\Common\FwUtils\FwUtilsTests\FwUtilsTests.csproj— 407/407 passed, including 6 new cases. Not run: the full suite, native tests, installer validation. Not done: a manual two-worktree launch.Reading this a year from now — start here
This started as a question, not a bug report: "my local Debug build still looks at DistFiles — could it look at
Outputinstead?" The investigation said no to the literal request and yes to the problem behind it. Both halves are recorded below, because the rejected half is the one that will otherwise be re-proposed.There were no working documents to delete; the reasoning never existed anywhere but here.
Decisions, and why
The registry loses to the source tree, rather than the probe merely being fixed. Fixing the anchoring alone would have changed nothing on a real dev machine:
GetDevDistFilesPath()only ever feddefaultDir, andGetDirectoryreturns the registry value whenever it is non-empty. On a dev machine it is always non-empty.Build/mkall.targets(setKeysInHKCU) has historically written it, butmkall.targets:277carries aREVIEW (Hasso) 2026.03noting those targets "appear unused by the recently-modernized build process", so the live writers are more likely the MSI (FLExInstaller/Overrides.wxi:11-12) andResolve-FieldWorksDevRegistry.ps1:54-55, plus the winapp skill's launch script. The value is a single machine-wide slot shared by every worktree, so it names whichever tree ran last. That is the actual defect; the fixed-depth probe is a second, independent one.FieldWorks.slnas the tree marker. The probe needs something that exists in a source tree and never beside an install. The installer harvestsDistFiles\**\*— the contents, into the install root — so an install has neither aDistFilesfolder nor a solution file at that level. Requiring both makes the installed path unreachable by construction rather than by convention.No memoization. Each
CodeDirectoryget now walks up doingDirectory.Exists+File.Existsper level. The old path opened and read a registry key on every get, so this is not a regression. Revisit only with a measurement.Paths not taken
Pointing the code directory at
Output/<Configuration>— the literal request.Outputholds build artifacts only; the code/data payload exists solely inDistFiles. Counted in the tree at the time:Language Explorer10 entries inDistFilesvs absent fromOutput/Debug;Parts4 vs absent;Helps7 vs absent;Icu70present vs absent;Templates42 vs 1.FlexStylesPath,FlexFolder,TemplateDirectory, andEditorialChecksDirectorywould all have broken.An overlay that probes
Outputfirst, then falls back toDistFiles. This cannot be expressed through the current API:CodeDirectoryreturns one string that ~100 call sitesPath.Combineonto. An overlay needs aResolveCodeFile(relativePath)seam instead — a much larger change, for a duplication problem that does not currently exist (onlyTemplatesoverlaps at all, with one entry).Just fixing the registry and stopping there. That is what unblocked the reporter (
Resolve-FieldWorksDevRegistry.ps1 -Force, run before this branch existed), and it is what every worktree switch will need again tomorrow. It treats the symptom.Evidence
Precedence, before this change —
GetDirectory(RegistryKey, string, string)inSrc/Common/FwUtils/FwDirectoryFinder.cs:rootDiris read from the registry, anddefaultDiris used onlyif (string.IsNullOrEmpty(rootDir)).GetDevDistFilesPath()feddefaultDir. Hence: registry set → probe irrelevant.The shared slot —
HKCU\SOFTWARE\SIL\FieldWorks\$(FWMAJOR)holdsRootCodeDir,RootDataDirandProjectsDir.Build/mkall.targetstargetsetKeysInHKCUwrites them from$(dir-fwdistfiles), thoughmkall.targets:277says those targets appear unused now; the MSI (FLExInstaller/Overrides.wxi:11-12) andResolve-FieldWorksDevRegistry.ps1:54-55are the likelier live writers. Nothing inSrc/writes those two values at runtime (searching forSetValue("RootCodeDir"outside tests returns no hits), so the value persists from whichever tree last built or launched.Existing tests keep passing for a non-trivial reason —
FwDirectoryFinderTestssets the registry toUtilsAssemblyDir/../../DistFiles, andInitializeFwRegistryHelperAttributedoes the same. Under the new precedence those values are ignored, but the walk-up returns the same path for a test run out ofOutput/Debug, so the assertions still hold.New coverage —
FindDevDistFiles_InsideSourceTree_FindsTreeDistFiles(Output/Debug,Output/Debug/x64,Src/Common/FwUtils/bin/Debug/net8.0),FindDevDistFiles_OutsideSourceTree_ReturnsNull, andCodeAndDataDirectory_PreferSourceTreeOverRegistry(RootCodeDir,RootDataDir), which points the registry at a fabricated other worktree and asserts both directories still resolve to this tree.Preflight review details
The preflight found no Critical issues and two Important open questions, neither of which was put to the author (the author pre-authorized commit, push, and PR in the same instruction that requested the fix, so no interview was held). They are open questions for the reviewer, not dismissed findings:
RootCodeDirat a non-tree location silently loses that override. Not mitigated: there is no opt-out and no notification that the override stopped applying.FieldWorks.sln. If the solution is renamed or removed, every dev build silently falls back to the registry/Program Files path with no diagnostic. A second marker or a build-time assertion would harden this.Minor: no memoization (considered and rejected, see Decisions); the Paratext plugin's parallel finder now differs in policy and neither file mentions the other; the new precedence test restores a fixture-owned registry value and would throw if the fixture stopped setting it.
Build and test evidence is in the Verification section above. The first build attempt failed on an unrelated ILRepack file lock on
Output\Debug\SIL.LCModel.Core.dll.config; the rerun was clean.gitlint --commits HEAD~1..HEADis clean.This change is