Consolidate many guidsForInstaller files into one - #81
Conversation
Code generated by Claude Code, checked and improved by Robin Munn. In the process of updating Mercurial4Chorus, we seem likely to end up with a hundred .guidsForInstaller.xml files scattered around the win/ directory tree. This is unwieldy, and it would be better if there could be a single file at the root of the directory tree containing all the GUIDs. The implementation keeps the existing .guidsForInstaller.xml file rather than deleting them; deleting them can be done manually once they are actually redundant. This allows a gradual move to a consolidated GUID file by first setting the ConsolidatedGuidFile property, then using it in a build, and finally committing the consolidated file and deleting the now-redundant scattered files.
|
Note that the new way of building Mercurial4Chorus that this PR would allow, when I tested it, ran the LibChorus test suite in 34:40 (34 minutes and 40 seconds) for the net8.0 target, as opposed to the 58:18 time it took to run the LibChorus test suite with the current version of the Mercurial4Chorus NuGet package. That's 1.68x faster: 3498 seconds ÷ 2080 seconds = 1.68173. This kind of dramatic speedup in the LibChorus test suite will probably translate to actual Send/Receive scenarios, too: the actual Send/Receive tests were producing speedups ranging from 1.50x to 1.80x faster. So this PR is definitely worth the effort of reviewing; it's the first step towards a noticeable speed increase for users of FieldWorks (and other software) doing a Send/Receive. |
|
Devin (https://app.devin.ai/review/sillsdev/SIL.BuildTasks/pull/81) flags a bug and two nitpicks that may be worth considering. |
This fixes the bug where running in CheckOnly mode would have still written to the consolidated database, if one was specified. Now the code that consolidates the GUIDs will still consolidate them into an in-memory database in CheckOnly mode (so that GetGuid can succeed) but will not write to disk when CheckOnly is true.
|
Devin's bug was a good catch, and is fixed by commit cad3f58. The two nitpicks are: 1) that the IMHO, the second nit is indeed a bug, technically — but it's not one that we're likely to hit since Mercurial directory trees are quite shallow and unlikely to ever become less shallow. Still, it's definitely worth asking Claude to look at it and determine what a fix would involve. (I'll note in passing that Claude raised the same concern, and mentioned that it's not a current concern because no Mercurial paths currently have the same last 50 characters). Now, if there are other repos besides Mercurial4Chorus that are using the MakeWixForDirTree task, then the risk of ID collisions might be more significant. But in the only case I actually know about, there's virtually no risk. Also worth noting, while I'm at it, that the possibility of file-ID collisions already exists in the current code; this PR doesn't actually change the logic that generates file IDs by truncating their paths. (And converting hyphens to underscores; Claude mentioned another possible-but-unlike file ID collision, if a repo ever had libssl-1_1.dll and libssl_1_1.dll in the same directory. Very unlikely to happen, though). P.S. While I was writing this, Claude came back with its conclusions. A fix would involve either: a) rewriting the collision-detection mechanism by including the file's full original path in a But my opinion is a) is too expensive and b) is too risky. Because while file ID collisions are theoretically possible, it's not actually likely to happen in the Mercurial code base, nor (as far as I know) any other code base. So I go for option c) do nothing, because this code has been running for years and years without ever running into the theoretical problem, and the likelihood of it ever happening is minuscule. We're far more likely to have to move off of WiX to some other install process (say, because it gets discontinued or something) than for any two file IDs to have the exact same last 50 characters and then a third one to get inserted in between them (or before the first one). P.S. I don't know if CodeRabbit is enabled for this repo, but let's try: |
|
@coderabbitai fullreview |
Two bugfixes for MakeWixForDirTree: - ImportMissingFrom now reports which .guidsForInstaller.xml file the missing GUID originally came from - CheckOnly now verifies that the consolidated GUID file actually has the GUIDs; previously it would have passed in consolidated-file mode even if the consolidated file was empty and the GUIDs were still in the individual .guidsForInstaller.xml files. Finally, one bugfix for a bug that pre-dated the PR: using CheckOnly would have deleted the generated .wxs files, because the deletion happened every time but the .wxs file was only written if CheckOnly was false. Now the deletion is also skipped in CheckOnly mode. Code written by Claude, then verbosity slightly toned down by Robin Munn. Commit message by Robin Munn because Claude was WAY too wordy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All Devin-reported bugs are fixed now; its one remaining flag is:
And that is, indeed, the intent. The idea is that you can run CheckOnly (with ConsolidatedGuidFile set), and get a list of file IDs throughout the whole project — which you can then double-check against what the consolidated GUID file contains, if you want. And the CheckOnly behavior hasn't changed from how it worked before this PR, except for fixing a bug where CheckOnly would have deleted a .wxs file. In other words, the behavior that Devin reported will only happen if you have set ConsolidatedGuidFile to something non-empty, otherwise CheckOnly behaves just like it did previously. |
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Inspired by Devin info flags; checked and authored by Claude; reviewed and edited by me; all non-blocking suggestions.
| guid = Guid.NewGuid().ToString(); | ||
| this[id] = guid; | ||
| Set(id, guid, _filename); | ||
| Write(); |
There was a problem hiding this comment.
Batch the writes to the consolidated file
Write() rewrites the entire file, and is called once per new GUID here and once per source file in ImportMissingFrom. Deleting the legacy files after migration retires the setup cost, but the per-GUID rewrite is permanent and grows with the file. Replace both call sites with a dirty flag and a single flush after ProcessDir returns, skipped when CheckOnly.
| _filename, count, string.Join(", ", sources))); | ||
| } | ||
|
|
||
| private void Write() |
There was a problem hiding this comment.
Make Write() atomic
The writer opens directly on _filename, so an interrupted or failed write now truncates the GUIDs for the whole tree rather than one directory's worth. Write to a temp file in the same directory, then File.Replace/File.Move over the target.
| } | ||
|
|
||
| SetupExclusions(); | ||
| SetupConsolidatedGuidFile(); |
There was a problem hiding this comment.
Move SetupConsolidatedGuidFile() inside the try
It parses XML and can write, but sits ahead of the try/catch (IOException), so an IO or parse failure throws out of the task instead of logging an error and returning false.
| /// <summary> | ||
| /// Copies in every entry this database does not already have, and saves if | ||
| /// anything was added. Used to consolidate the per-directory files into a | ||
| /// single one: File Ids encode the whole relative path (for example |
There was a problem hiding this comment.
Fix the doc comment's stated invariant
"File Ids encode the whole relative path ... so they are unique across the tree" isn't the guarantee. Ids are truncated to their last 50 characters, and uniqueness comes from the task-wide _suffixes counter.
| /// Opens the consolidated GUID file, if one was asked for, and seeds it from | ||
| /// any per-directory files left over from before the switch. | ||
| /// </summary> | ||
| private void SetupConsolidatedGuidFile() |
There was a problem hiding this comment.
Add tests
The task has no test file today. The merge and the CheckOnly paths are the parts worth covering.
| ConsolidatedGuidFile = Path.GetFullPath(ConsolidatedGuidFile); | ||
| _sharedGuidDatabase = IdToGuidDatabase.Create(ConsolidatedGuidFile, this); | ||
|
|
||
| foreach (var legacy in Directory.GetFiles(Path.GetFullPath(RootDirectory), |
There was a problem hiding this comment.
Apply the tree's exclusions to the legacy scan
The recursive scan descends into .svn, CVS, and Excluded directories that ProcessDir skips, so GUIDs for files that are never emitted as components get merged and written permanently -- outliving the legacy file deletion -- and they inflate the CheckOnly "missing N GUID(s)" count. Filter the results using the same rules ProcessDir uses. Low severity; only bites if an excluded directory actually holds a .guidsForInstaller.xml.
2 participants
Fixes #80.
Code generated by Claude Code, checked and improved by Robin Munn.
In the process of updating Mercurial4Chorus, we seem likely to end up with a hundred .guidsForInstaller.xml files scattered around the win/ directory tree. This is unwieldy, and it would be better if there could be a single file at the root of the directory tree containing all the GUIDs.
The implementation keeps the existing .guidsForInstaller.xml file rather than deleting them; deleting them can be done manually once they are actually redundant. This allows a gradual move to a consolidated GUID file by first setting the ConsolidatedGuidFile property, then using it in a build, and finally committing the consolidated file and deleting the now-redundant scattered files.
This change is