[Resizetizer] Stop deleting generated images behind symlinked paths - #37859
[Resizetizer] Stop deleting generated images behind symlinked paths#37859Redth wants to merge 3 commits into
Conversation
The ResizetizeImages target deleted every image it had just generated when any segment of the project path was a symbolic link or junction, which is the normal shape of a macOS temp path (/tmp -> /private/tmp, /var/folders -> /private/var/folders). The stale-file step built the delete list by taking a wildcard over the intermediate image folder and removing the task's CopiedResources from it. Those two lists reach the target through different code paths: * MSBuild resolves %(FullPath) against $(MSBuildProjectDirectory), which keeps the spelling the build was started with. * The task resolved its relative IntermediateOutputPath with Path.GetFullPath, which roots against the process working directory that Unix reports with every link already resolved. The two spellings differ behind a link, the textual Remove matched nothing, and every freshly written image was classified as stale and deleted. Fixes it on both sides: * $(_MauiIntermediateImagesFullPath) roots the intermediate image folder against $(MSBuildProjectDirectory) so the paths the tasks emit match the paths MSBuild emits. ResizetizerIntermediateOutputAbsolutePath, which feeds the Tizen TizenTpkSubDir calculation, now uses the same spelling instead of [System.IO.Path]::GetFullPath. * DetectStaleOutputFilesTask replaces the textual Remove and compares canonical paths, so links, separators and casing can no longer make a generated file look stale. PathCanonicalizer resolves the links of every path segment that exists and appends the ones that do not, so outputs that have not been written yet can still be compared, and it degrades to the plain full path where link resolution is unavailable. Adds unit tests for the canonicalizer and the new task, plus end-to-end tests that run the real target through MSBuild from both an ordinary and a symlinked directory and cover first build, no-op rebuild, source change, deleted output, stale file cleanup and clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37859Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37859" |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Resizetizer incremental-build cleanup bug where generated images could be incorrectly classified as “stale” (and deleted) when the project/intermediate paths differ textually due to symlinks/junctions (common on macOS temp paths). It does this by making intermediate output paths consistent with MSBuild’s path spelling and by computing stale-file differences using canonicalized paths rather than textual item-spec subtraction, with new unit tests covering the symlinked-path scenarios end-to-end.
Changes:
- Add a canonical path comparison utility (
PathCanonicalizer) and a new MSBuild task (DetectStaleOutputFilesTask) to compute stale outputs safely. - Update
Microsoft.Maui.Resizetizer.After.targetsto pass a project-rooted absolute intermediate output path to tasks and to replace item-specRemove-based stale detection withDetectStaleOutputFilesTask. - Add unit tests covering path canonicalization, stale output detection, and an end-to-end incremental MSBuild run (including a symlinked entry directory).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/SingleProject/Resizetizer/test/UnitTests/SymbolicLink.cs | Adds a helper to create directory symlinks/junction equivalents for tests, with graceful “not supported” handling. |
| src/SingleProject/Resizetizer/test/UnitTests/ResizetizeImagesIncrementalTests.cs | End-to-end MSBuild-driven incremental tests verifying generated images are not deleted under symlinked paths and that cleanup still removes truly stale files. |
| src/SingleProject/Resizetizer/test/UnitTests/PathCanonicalizerTests.cs | Unit coverage for canonicalization across symlinks, missing segments, relative paths, and normalization behaviors. |
| src/SingleProject/Resizetizer/test/UnitTests/DetectStaleOutputFilesTaskTests.cs | Validates canonical set-difference behavior and metadata preservation in stale-file detection. |
| src/SingleProject/Resizetizer/src/PathCanonicalizer.cs | Introduces canonicalization logic (including best-effort link resolution) for safe path comparisons across differing spellings. |
| src/SingleProject/Resizetizer/src/nuget/buildTransitive/Microsoft.Maui.Resizetizer.After.targets | Roots intermediate image output path consistently and replaces textual item subtraction with canonical stale-file detection via a task. |
| src/SingleProject/Resizetizer/src/DetectStaleOutputFilesTask.cs | Adds a new MSBuild task to compute stale outputs by canonical path comparison while returning original items. |
… shapes Extends the coverage added with the fix so the properties it relies on are asserted rather than assumed. Deletion behind links, from every direction: * The project directory is reached through a link (already covered). * Only the intermediate output directory is a link, which is what a redirected obj\ looks like. * Only the source image directory is a link. Ordinary cleanup is not weakened. Verified with two mutations of DetectStaleOutputFilesTask: never reporting a file stale fails 10 tests (8 unit, 2 end to end), so the cleanup path is genuinely guarded. Deletes cannot escape the intermediate root. StaleFiles is returned from the input Files verbatim, never as the canonical spelling, so <Delete> only ever sees paths under the wildcard's root: * StaleFilesAreReturnedVerbatimEvenWhenCanonicalizationChangesTheSpelling * StaleFilesAreAlwaysASubsetOfTheInputFiles * ALinkPointingOutsideTheDirectoryIsReportedByItsInsidePath, which covers a link inside the intermediate directory that points outside it. MSBuild deletes the link and never the file it points at. Returning the canonical path instead fails 8 tests, so this is pinned too. Unix and Windows semantics, each asserted where the code supports it: * Case sensitivity follows the platform, and the comparer choice itself is asserted. On Linux two spellings that differ only by case stay distinct; elsewhere they are the same file, which can only ever keep a stale file rather than delete a live one. * Windows keeps its drive root and accepts both separator characters. * Windows junctions resolve like directory links. .NET cannot create one, so the reparse point is written through DeviceIoControl, and the test skips where junctions are unavailable. * Backslashes stay ordinary characters in Unix paths inside the canonicalizer, while MSBuild normalizes separators before the task ever sees an item spec, which is now asserted rather than assumed. Also covers chained links, idempotence and canonicalizing a bare root. All 14 end-to-end scenarios fail against the previous targets and pass with them. Note that the non-linked variants fail too: Path.GetTempPath on macOS returns a path under /var, which is itself a link to /private/var, so any build from a temp directory reproduced the bug. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/SingleProject/Resizetizer/test/UnitTests/ResizetizeImagesIncrementalTests.cs:120
- Same as above: setting the source image mtime to DateTime.UtcNow can still collide with the previous build's timestamps on coarse-resolution file systems, causing the target to be skipped and the stale-file cleanup assertion to flake. Make the source mtime definitively newer than the generated output.
// Touch the source image so the target is not skipped as up to date.
File.SetLastWriteTimeUtc(Path.Combine(project.ImagesDirectory, ImageName), DateTime.UtcNow);
| var source = Path.Combine(project.ImagesDirectory, ImageName); | ||
| File.Copy(Path.Combine(AppContext.BaseDirectory, "images", "camera_color.png"), source, overwrite: true); | ||
| // File.Copy carries the source timestamp over, but an edit would bump it. | ||
| File.SetLastWriteTimeUtc(source, DateTime.UtcNow); |
Review of the previous two commits turned up three real problems. All three are reproducible, and each fix is pinned by a mutation of the code it guards. Deletes could escape the intermediate folder. A recursive MSBuild wildcard walks through a directory link, so `$(_MauiIntermediateImages)\**\*` can name a file that only appears to live inside obj. <Delete> then follows the link and removes the real file rather than the link. Verified directly: a glob over a folder containing a link to an outside directory deleted the outside file. Returning the original item spec was not enough, because the spec itself already pointed through the link. DetectStaleOutputFilesTask now takes the intended Root and never reports anything whose resolved directory falls outside it. Deleting the previous claim's guard fails 3 tests, one of which drives the real target end to end. A stale alias of a live output survived cleanup forever. Canonicalizing the whole path meant a leftover link named orphan.png pointing at the current camera.png compared equal to it and was kept, and on Android the whole intermediate tree is exposed through LibraryResourceDirectories, so a removed resource stayed visible. Only the directory part of a path is link resolved now; the file name is kept verbatim. Canonicalizing the leaf again fails 3 tests. The resolution cache could answer for the wrong directory. It was keyed with the platform comparer, so on a case sensitive volume two directories whose names differ only by case shared an entry and one could be handed the other's resolved target. Cache keys are exact spellings now, and the case insensitive comparer is used only for the final comparison. Also adds IsUnder, which rejects a sibling whose name merely starts with the root's name, and treats an unresolvable Root as "detect nothing" rather than "delete everything". All 15 end-to-end scenarios still fail against the previous targets and pass with these. Full Resizetizer suite: 738 passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/SingleProject/Resizetizer/test/UnitTests/ResizetizeImagesIncrementalTests.cs:78
- The test forces an incremental rebuild by touching the source image, but setting LastWriteTimeUtc to DateTime.UtcNow can be a no-op on file systems with coarse timestamp resolution, causing MSBuild to still treat the target as up-to-date and making this test flaky. Bump the timestamp relative to an existing output (generated image) by a couple seconds to guarantee it becomes newer.
var source = Path.Combine(project.ImagesDirectory, ImageName);
File.Copy(Path.Combine(AppContext.BaseDirectory, "images", "camera_color.png"), source, overwrite: true);
// File.Copy carries the source timestamp over, but an edit would bump it.
File.SetLastWriteTimeUtc(source, DateTime.UtcNow);
src/SingleProject/Resizetizer/test/UnitTests/ResizetizeImagesIncrementalTests.cs:120
- This "touch" uses DateTime.UtcNow, which can fail to advance the timestamp on file systems with 1-second granularity (or when operations occur within the same tick), making the build still skip ResizetizeImages and causing intermittent failures. Prefer bumping relative to an existing output timestamp.
// Touch the source image so the target is not skipped as up to date.
File.SetLastWriteTimeUtc(Path.Combine(project.ImagesDirectory, ImageName), DateTime.UtcNow);
src/SingleProject/Resizetizer/test/UnitTests/ResizetizeImagesIncrementalTests.cs:216
- Same timestamp-granularity issue: DateTime.UtcNow may not advance LastWriteTimeUtc enough to invalidate MSBuild's up-to-date check, which can make this test flaky. Use an output timestamp + a small offset.
// Touch the source image so the target is not skipped as up to date.
File.SetLastWriteTimeUtc(Path.Combine(project.ImagesDirectory, ImageName), DateTime.UtcNow);
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
ResizetizeImagesdeletes every image it just generated when any segment of the project path is a symbolic link or junction. That is the normal shape of a macOS temp path (/tmp→/private/tmp,/var/folders/…→/private/var/folders/…), so every build from a temp directory on macOS reproduces it — which is how it was found while externalizing the Tizen build tasks.Root cause
The stale-file step globbed the intermediate image folder and removed the task's
CopiedResourcesfrom it:The two lists reach the target through different code paths and, behind a link, spell the same file differently:
%(FullPath)on the wildcard$(MSBuildProjectDirectory)— logical/var/folders/…/r/drawable/camera.pngPath.GetFullPathinside the task/private/var/folders/…/r/drawable/camera.pngRemovecompares item specs textually, matched nothing, and<Delete>removed everything the task had just written.$([System.IO.Path]::GetFullPath(…))in the targets had the same problem, which is why the TizenTizenTpkSubDircalculation used a third spelling.Fix
$(_MauiIntermediateImagesFullPath)roots the intermediate folder against$(MSBuildProjectDirectory)with[MSBuild]::NormalizePath, so the absolute paths the tasks emit match the ones MSBuild emits. It is passed toResizetizeImagesand now backsResizetizerIntermediateOutputAbsolutePath, replacing[System.IO.Path]::GetFullPath.DetectStaleOutputFilesTaskreplaces the textualRemoveand diffs on canonical paths. Returned items keep their original item spec and metadata, so user-facing paths are untouched.PathCanonicalizerresolves links for the segments of a path that exist and appends the ones that do not, so an output that has not been written yet can still be compared (a plainrealpathwould fail). It never throws and degrades to the lexical full path where link resolution is unavailable — e.g. MSBuild.exe on .NET Framework, which is today's behaviour.Hardening from review
Three further issues were found in review, each reproduced first and each pinned by a mutation of the code that guards it:
**\*can name a file that only appears to be insideobj, and<Delete>then removes the real file instead of the link. Verified directly — a glob over a folder containing a link to an outside directory deleted the outside file. The task now takes the intendedRootand never reports anything whose resolved directory falls outside it. Removing that guard fails 3 tests, one of which drives the real target end to end.orphan.pnglink pointing at the currentcamera.pngcompare equal to it, so it was kept — and on Android the whole intermediate tree is exposed viaLibraryResourceDirectories, so a removed resource stayed visible. Only the directory is link resolved now; the file name is kept verbatim. Canonicalizing the leaf again fails 3 tests.Path comparison is case-insensitive everywhere except Linux. On a case-sensitive macOS volume that can only ever keep a stale file rather than delete a live one, and Resizetizer already rejects output names that differ only by case.
Issues Fixed
Fixes the Resizetizer path-normalization bug found while externalizing the Tizen
Build.Tasks. Downstream,Maui.Tizencan drop the workaround that canonicalizes the generated test project / intermediate directory (arealpath-style resolution of thePath.GetTempPath()root) before invoking the build.Validation
ResizetizeImagesIncrementalTests— runs the realResizetizeImagestarget through MSBuild. Covers first build, no-op rebuild (asserting the target is actually skipped), source-image change, deleted output, stale-file cleanup and_CleanResizetizer+ rebuild, each as a[Theory]over an ordinary and a symlinked project directory; plus a symlinked intermediate output directory, symlinked input images, and the directory-link escape case. 15/15 fail against the original targets, 15/15 pass with these.PathCanonicalizerTests— linked vs physical equivalence, chained links, idempotence, not-yet-created files, bare roots, relative paths, redundant segments and trailing separators, invalid input, leaf links keeping their own key,IsUndercontainment and sibling rejection, per-platform comparer, Windows drive/separator handling, Windows junctions (created throughDeviceIoControl, skipped where unavailable), and case-sensitive cache separation.DetectStaleOutputFilesTaskTests— linked directories, metadata preservation, verbatim item specs, subset invariant, root containment, stale aliases, unresolvable root, and that genuinely stale files are still deleted.Resizetizer.UnitTests: 738 passed, 0 failed.dotnet formatclean. TizenTizenTpkSubDirverified asres/under symlinked paths and paths containing spaces.