Fix LT-22509: Newtonsoft.Json Method not found error by jasonleenaylor · Pull Request #879 · sillsdev/FieldWorks

jasonleenaylor · 2026-05-12T19:24:29Z

Newtonsoft.Json doesn't change assembly versions on minor version bumps. 13.0.4 added a method and we started using it in libpalaso. If a version less than that is installed in the GAC our local version would not be loaded. This patch enforces using our local copy by our application.

This change is

github-actions · 2026-05-12T19:35:25Z

NUnit Tests

1 files ±0 1 suites ±0 11m 33s ⏱️ + 3m 22s
4 205 tests ±0 4 134 ✅ ±0 71 💤 ±0 0 ❌ ±0
4 214 runs ±0 4 143 ✅ ±0 71 💤 ±0 0 ❌ ±0

Results for commit 63c5255. ± Comparison against base commit 2f97608.

♻️ This comment has been updated with latest results.

- Newtonsoft.Json doesn't change assembly versions on minor version bumps. 13.0.4 added a method and we started using it in libpalaso. If a version less than that is installed in the GAC our local version would not be loaded. This patch enforces using our local copy by our application. Change-Id: I9b7cdfbe792fda13b3dcc570c2c5dc8d19e6d596

Change-Id: I8255b5a9f413d76bb053a9a552ba6b08ad74b2fa

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Addresses LT-22509 by ensuring the installer ships and “owns” Newtonsoft.Json.dll so an older GAC-installed copy can’t cause runtime “method not found” failures.

Changes:

Author explicit WiX components for Newtonsoft.Json.dll (private app copy + optional GAC install) behind <?ifdef MASTERBUILDDIR?>.
Add Heat harvest exclusion lists and extend KeyPathFix.xsl to drop excluded single-file Heat components and their dangling ComponentRefs.
Update both WiX 6 and WiX 3 (legacy PatchableInstaller) build flows/documentation to keep harvest + patch behavior consistent.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
FLExInstaller/wix6/Shared/Common/CustomComponents.wxi	Adds authored `Newtonsoft.Json.dll` components for WiX 6 builds (app + GAC).
FLExInstaller/wix6/Shared/Base/heat-exclude.xml	Defines WiX 6 Heat exclusions (currently `Newtonsoft.Json.dll`).
FLExInstaller/wix6/Shared/Base/KeyPathFix.xsl	Filters out excluded Heat components and cleans dangling refs; keeps `KeyPath` adjustments.
FLExInstaller/wix6/Shared/Base/Framework.wxs	Wires new Newtonsoft components into `Feature Complete` when `MASTERBUILDDIR` is set.
FLExInstaller/wix6/FieldWorks.Installer.wixproj	Documents/clarifies Heat usage; mostly formatting.
FLExInstaller/PatchableInstallerHeatExclude.xml	Defines legacy WiX 3 Heat exclusions for PatchableInstaller.
FLExInstaller/CustomFeatures.wxi	Adds guarded `ComponentRef`s for Newtonsoft components in legacy feature authoring.
FLExInstaller/CustomComponents.wxi	Adds authored Newtonsoft components into legacy (WiX 3) component authoring.
FLExInstaller/AGENTS.md	Documents Heat exclusion + `-fv` servicing behavior and the guarding pattern.
Build/Installer.targets	Whitespace/indentation-only change.
Build/Installer.legacy.targets	Copies legacy heat-exclude.xml into PatchableInstaller tree before harvest/build.

+	<xsl:template match="*[local-name()='ComponentRef']" priority="2">
+		<xsl:variable name="rid" select="@Id"/>
+		<xsl:variable name="comp" select="//*[local-name()='Component'][@Id = $rid]"/>


+				<xsl:variable name="isExcluded">
+					<xsl:for-each select="document('heat-exclude.xml')">
+						<xsl:value-of select="count(/*[local-name()='HeatHarvestExcludes']/*[local-name()='Exclude'][@Name and translate(@Name, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz') = translate($base, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz')])"/>
+					</xsl:for-each>
+				</xsl:variable>


+		<Component Id="NewtonsoftJsonGac" Guid="*">
+			<File Id="NewtonsoftJsonFileGac"
+				Name="Newtonsoft.Json.dll"
+				Source="$(var.MASTERBUILDDIR)\Newtonsoft.Json.dll"
+				KeyPath="yes"
+				Assembly=".net" />
+		</Component>


+			<_FieldWorksPatchableInstallerHeatExclude>$([MSBuild]::NormalizePath('$(fwrt)', 'FLExInstaller', 'PatchableInstallerHeatExclude.xml'))</_FieldWorksPatchableInstallerHeatExclude>
+			<_PatchableInstallerHeatExcludeDest>$([MSBuild]::NormalizePath('$(fwrt)', 'PatchableInstaller', 'BaseInstallerBuild', 'heat-exclude.xml'))</_PatchableInstallerHeatExcludeDest>
 		</PropertyGroup>
+		<Copy
+			SourceFiles="$(_FieldWorksPatchableInstallerHeatExclude)"
+			DestinationFiles="$(_PatchableInstallerHeatExcludeDest)"
+			SkipUnchangedFiles="false"
+			Condition="Exists('$(_FieldWorksPatchableInstallerHeatExclude)')"
+		/>


johnml1135 · 2026-05-14T00:44:41Z

Initial AI review (unfiltered):

Assessment

The WiX 6 GAC-servicing comment is the important one. The PR adds a GAC assembly component in CustomComponents.wxi:73-78, but the WiX 6 project still only shows cabinet-cache linker options in FieldWorks.Installer.wixproj:34. I did not find a visible WiX 6 equivalent of -fv / SetMsiAssemblyNameFileVersion, so Copilot’s concern there is valid.
The bigger problem is actually cross-repo drift in the legacy path. Installer.legacy.targets:159-166 copies PatchableInstallerHeatExclude.xml into PatchableInstaller/BaseInstallerBuild/heat-exclude.xml, and AGENTS.md:13-14 says legacy buildMsi.bat passes -fv. But in the external genericinstaller repo, the actual base MSI and patch scripts do not pass -fv, and the checked-in legacy KeyPathFix.xsl files only rewrite KeyPath; they do not load heat-exclude.xml. So that copy step is currently inert unless genericinstaller changes too.
The MASTERBUILDDIR guard pattern is sensible only if the intent is “full/base installer fix, not patch fix.” The external patch flow builds master and update from separate defines, so those guards intentionally exclude the new Newtonsoft components from the update side. If LT-22509 must be fixed via MSP, this is still a real gap.
The two XSL comments on KeyPathFix.xsl:39-85 are reasonable but non-blocking. The O(N^2) lookup and duplicated exclusion logic are maintainability/perf issues, not the main correctness risk.
The MakeDir comment on Installer.legacy.targets:159-166 is low value. Adding it is harmless hardening, but it is not the real issue. The real issue is that the external legacy repo does not currently consume the copied file.

Recommendations

Keep the WiX 6 MsiAssemblyName/FileVersion comment open until there is proof. Either add the WiX 6 setting explicitly or inspect the built MSI tables and confirm the GAC assembly row includes file-version metadata.
Treat the legacy PatchableInstaller path as incomplete in this PR alone. Either land matching changes in the external genericinstaller repo, or remove/soften the claims in AGENTS.md:13-14 and in review discussion so the PR does not imply legacy behavior that does not exist.
Decide whether the supported fix path is full MSI / major upgrade only, or MSP patch too. If patch support matters, the current MASTERBUILDDIR-guarded component pattern is not enough.
Defer the two XSL refactors and the MakeDir tweak unless someone is already touching those files. They are follow-up cleanup, not merge blockers.

Net: Copilot’s WiX 6 servicing comment is the one to act on, the MakeDir comment is minor, and the external genericinstaller inspection exposed a larger issue Copilot did not call out: the legacy installer behavior described by this PR is not implemented in the installer repo it depends on.

I can do a manual review after this.

jasonleenaylor marked this pull request as draft May 12, 2026 19:26

jasonleenaylor force-pushed the bugfix/LT-22509 branch 3 times, most recently from b96a01d to bd5e06c Compare May 12, 2026 21:42

jasonleenaylor force-pushed the bugfix/LT-22509 branch from bd5e06c to 7908ff0 Compare May 12, 2026 21:43

jasonleenaylor force-pushed the bugfix/LT-22509 branch from 7908ff0 to 05ba6cc Compare May 13, 2026 23:10

jasonleenaylor marked this pull request as ready for review May 13, 2026 23:32

Installer: Newtonsoft refs in CustomFeatures.wxi only

63c5255

Change-Id: I8255b5a9f413d76bb053a9a552ba6b08ad74b2fa

jasonleenaylor force-pushed the bugfix/LT-22509 branch from 05ba6cc to 63c5255 Compare May 13, 2026 23:58

johnml1135 requested review from Copilot and johnml1135 May 14, 2026 00:26

Copilot AI reviewed May 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix LT-22509: Newtonsoft.Json Method not found error#879

Fix LT-22509: Newtonsoft.Json Method not found error#879
jasonleenaylor wants to merge 3 commits into
mainfrom
bugfix/LT-22509

jasonleenaylor commented May 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 12, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

johnml1135 commented May 14, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jasonleenaylor commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NUnit Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

johnml1135 commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jasonleenaylor commented May 12, 2026 •

edited

Loading

github-actions Bot commented May 12, 2026 •

edited

Loading

johnml1135 commented May 14, 2026 •

edited

Loading