Download Charis SIL test font instead of committing it#945
Conversation
Render comparison artifactsRender snapshot failures were reported in b3f93e10ff1c run 27309998687.1, but the latest run 372e2a9cbdcf run 27360050809.1 is clean. This comment will be replaced if a future run produces render snapshot failures again. |
There was a problem hiding this comment.
Pull request overview
This PR removes the committed Charis SIL 5.000 test font used by the Views native shaping tests and replaces it with a build-time download of Charis SIL 6.200 from a pinned GitHub release, updating the native tests to load the new font path and adjusting the OpenType metrics assertion to use smcp.
Changes:
- Added a pinned Charis SIL download + unzip step to
Build/PackageRestore.targets(download-once intoDownloads/). - Updated
TestViews.vcxproj/.filtersto stop including committed font artifacts and to copy the downloadedCharisSIL-Regular.ttfinto the test output under a version-less folder. - Updated
TestUniscribeEngine.hto load the font from the new output path and to validate metrics changes viasmcpinstead ofliga.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Build/PackageRestore.targets | Adds a pinned Charis SIL 6.200 ZIP download and unzip into Downloads/CharisSIL for the native Views tests. |
| Src/views/Test/TestViews.vcxproj | Removes committed font items and adds a post-build copy step to place the downloaded font beside TestViews.exe. |
| Src/views/Test/TestViews.vcxproj.filters | Removes the committed Charis SIL font artifacts from the VS filters list. |
| Src/views/Test/TestUniscribeEngine.h | Updates the runtime font path and switches the metrics test from liga to smcp. |
| Src/views/Test/TestData/Fonts/CharisSIL-5.000/README.txt | Removes the previously committed Charis SIL 5.000 README from the repo. |
| Src/views/Test/TestData/Fonts/CharisSIL-5.000/OFL.txt | Removes the previously committed Charis SIL 5.000 OFL license text from the repo. |
| <Target Name="CopyCharisSilTestFont" AfterTargets="Build"> | ||
| <ItemGroup> | ||
| <CharisSilTestFont Include="$(FwRoot)\Downloads\CharisSIL\**\CharisSIL-Regular.ttf" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
I see no reason not to tie this to a specific version. We want deterministic behavior more than anything else.
There was a problem hiding this comment.
Agreed — determinism first. Pinned in 59bb6aa: both the Charis copy here (TestViews.vcxproj) and the analogous Scheherazade copy (RootSiteTests.csproj, from #943) now reference the exact version folder (CharisSIL-6.200 / ScheherazadeNew-4.500) via a property kept in sync with the *Version in PackageRestore.targets, instead of a ** glob. The copy destination stays version-less, so the runtime font paths are unchanged.
There was a problem hiding this comment.
Fixed in 59bb6aa — replaced the recursive ** glob with the exact pinned-version path, so a second extracted version under Downloads/ can no longer be picked up nondeterministically.
johnml1135
force-pushed
the
claude/views-charis-download
branch
from
48b840b to
4ef9c78
Compare
The Views shaping tests bundled CharisSIL 5.000 in the repo. Instead, download Charis SIL 6.200 from a pinned GitHub release at build time into the git-ignored Downloads/ and copy it beside TestViews.exe; remove the committed font. The OpenType metrics test moves from liga to smcp: in Charis 6.x the fi/ffi ligatures keep the component advance widths, so liga changes the rendered glyphs (still covered by the pixel tests) without changing segment metrics, whereas small caps do change advance width. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review feedback: the AfterBuild copy targets used a recursive glob (CharisSIL\**, ScheherazadeNew\**) that could match more than one extracted version in Downloads/, making the copied test font nondeterministic. Tie each copy to the exact pinned version (kept in sync with the *Version property in PackageRestore.targets). The destination stays version-less, so the runtime font paths are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jasonleenaylor
force-pushed
the
claude/views-charis-download
branch
from
4ef9c78 to
59bb6aa
Compare
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
@johnml1135 reviewed 8 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on jasonleenaylor).
Summary
The Views native shaping tests (
TestViews/TestUniscribeEngine.h) relied on a copy of Charis SIL 5.000 committed underSrc/views/Test/TestData/Fonts/. This removes the committed font and instead downloads Charis SIL 6.200 from a pinned GitHub release (silnrsi/font-charis) at build time — mirroring how other build dependencies (GeckofxHtmlToPdf, pc-parse) are already pulled inPackageRestore.targets.What changed
PackageRestore.targets— downloadCharisSIL-6.200.zipfrom the pinned release into the git-ignoredDownloads/and unzip it, guarded by!Exists(download-once). Version is a singleCharisSilVersionproperty.TestViews.vcxproj/.filters— drop the committed-font<None>items;CopyCharisSilTestFontcopies the downloaded regular face to a version-lessTestData\Fonts\CharisSIL\. The copy source is tied to the exact pinned version folder (CharisSIL-6.200) via aCharisSilTestFontVersionproperty kept in sync withCharisSilVersion— not a wildcard — so the test font selection is deterministic even ifDownloads/holds more than one extracted version.TestUniscribeEngine.h— point the runtime font path atCharisSIL\CharisSIL-Regular.ttf, and switch the OpenType metrics test fromligatosmcp. In Charis 6.x the fi/ffi ligatures keep the component advance widths, soligachanges the rendered glyphs (still covered by the pixel tests) without changing segment metrics; small caps reliably change advance width, and the pixel tests already usesmcp.RootSiteTests.csproj— applied the same exact-version pinning to the analogous Scheherazade New font copy (from Fix flaky render snapshot tests caused by blinking caret #943), per review feedback, for the same deterministic-selection reason.The font family name is unchanged (
Charis SILthrough v6.x; v7 renames it toCharis), so no test face-name changes were needed.Versions
v6.200CharisSilVersion(PackageRestore.targets) /CharisSilTestFontVersion(TestViews.vcxproj)v4.500ScheherazadeNewVersion(PackageRestore.targets) /ScheherazadeNewTestFontVersion(RootSiteTests.csproj)Verification
TestViews(Charis shaping):[301-0-0]pass (was[300-1-0]before theliga→smcpchange).RootSiteTestsRenderBenchmark(incl.rtl-script/multi-wsvia Scheherazade): 38/38 pass.Reviewer notes
Downloads/and the build output — nothing font-related is committed.FLExInstaller/...) is intentionally untouched.🤖 Generated with Claude Code
This change is