Skip to content

Commit 0e3cae3

Browse files
RedthCopilot
andcommitted
[Resizetizer] Confine stale detection to the intermediate folder
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>
1 parent eef0107 commit 0e3cae3

6 files changed

Lines changed: 405 additions & 101 deletions

File tree

src/SingleProject/Resizetizer/src/DetectStaleOutputFilesTask.cs

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,20 @@ namespace Microsoft.Maui.Resizetizer
1111
/// build can delete files left over from a previous build without ever deleting a file it just wrote.
1212
/// </summary>
1313
/// <remarks>
14+
/// <para>
1415
/// The two item lists reach this task through different code paths: <see cref="Files"/> comes from an
1516
/// MSBuild wildcard rooted at the project directory, while <see cref="KnownOutputs"/> comes back from
1617
/// a task that resolved the same directory itself. A plain <c>Remove</c> compares those item specs
1718
/// textually, so a symbolic link or junction anywhere in the project path is enough to make every
1819
/// generated file look stale. Comparing canonical paths makes the difference independent of spelling
1920
/// while the returned items keep their original item spec and metadata.
21+
/// </para>
22+
/// <para>
23+
/// A recursive MSBuild wildcard descends through directory links, so it can name a file that lives
24+
/// outside <see cref="Root"/>, and <c>&lt;Delete&gt;</c> would then remove that outside file rather
25+
/// than the link. Anything whose resolved directory escapes <see cref="Root"/> is therefore never
26+
/// reported as stale.
27+
/// </para>
2028
/// </remarks>
2129
public class DetectStaleOutputFilesTask : Task
2230
{
@@ -26,6 +34,10 @@ public class DetectStaleOutputFilesTask : Task
2634
/// <summary>The files the build expects to be there.</summary>
2735
public ITaskItem[] KnownOutputs { get; set; }
2836

37+
/// <summary>The only directory whose contents this task is allowed to report as stale.</summary>
38+
[Required]
39+
public string Root { get; set; }
40+
2941
/// <summary>The members of <see cref="Files"/> that are safe to delete.</summary>
3042
[Output]
3143
public ITaskItem[] StaleFiles { get; set; }
@@ -38,23 +50,42 @@ public override bool Execute()
3850
return true;
3951

4052
var canonicalizer = new PathCanonicalizer();
41-
var keep = canonicalizer.CreateSet(KnownOutputs?.Select(i => i.ItemSpec) ?? Enumerable.Empty<string>());
53+
54+
var root = canonicalizer.CanonicalizeDirectory(Root);
55+
if (string.IsNullOrEmpty(root))
56+
{
57+
Log.LogMessage(MessageImportance.Low, $"Skipping stale file detection because the root '{Root}' could not be resolved.");
58+
return true;
59+
}
60+
61+
var keep = new HashSet<string>(PathCanonicalizer.Comparer);
62+
foreach (var known in KnownOutputs ?? Enumerable.Empty<ITaskItem>())
63+
{
64+
var key = canonicalizer.GetComparisonKey(known?.ItemSpec);
65+
if (key is not null)
66+
keep.Add(key);
67+
}
4268

4369
var stale = new List<ITaskItem>();
4470

4571
foreach (var file in Files)
4672
{
47-
if (file is null || string.IsNullOrWhiteSpace(file.ItemSpec))
73+
var key = canonicalizer.GetComparisonKey(file?.ItemSpec);
74+
if (key is null)
4875
continue;
4976

50-
if (keep.Contains(canonicalizer.Canonicalize(file.ItemSpec)))
77+
if (!PathCanonicalizer.IsUnder(key, root))
78+
{
79+
Log.LogMessage(MessageImportance.Low, $"Leaving '{file.ItemSpec}' alone because it resolves to '{key}', which is outside '{root}'.");
5180
continue;
81+
}
5282

53-
stale.Add(file);
54-
}
83+
if (keep.Contains(key))
84+
continue;
5585

56-
foreach (var file in stale)
5786
Log.LogMessage(MessageImportance.Low, $"Detected stale output file '{file.ItemSpec}'.");
87+
stale.Add(file);
88+
}
5889

5990
StaleFiles = stale.ToArray();
6091

src/SingleProject/Resizetizer/src/PathCanonicalizer.cs

Lines changed: 92 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@
77
namespace Microsoft.Maui.Resizetizer
88
{
99
/// <summary>
10-
/// Produces a canonical spelling of a file system path so that two differently spelled paths
11-
/// which point at the same file compare as equal.
10+
/// Produces a canonical spelling of a file system path so that two differently spelled paths which
11+
/// point at the same file compare as equal.
1212
/// </summary>
1313
/// <remarks>
1414
/// <para>
@@ -20,13 +20,18 @@ namespace Microsoft.Maui.Resizetizer
2020
/// MSBuild items and task outputs wrongly reports freshly written files as stale.
2121
/// </para>
2222
/// <para>
23-
/// The canonical form is only ever used for comparison. Callers keep the original item spec so the
24-
/// paths surfaced to the rest of the build stay in the spelling the user provided.
23+
/// Only the <em>directory</em> part of a path is link resolved. The file name is kept verbatim, so two
24+
/// different names in one directory never collapse into one even when one of them is a link to the
25+
/// other. Resolving the leaf as well would let a stale alias masquerade as a live output and survive
26+
/// cleanup forever.
2527
/// </para>
2628
/// <para>
27-
/// Canonicalization resolves the links of each path segment that already exists and appends the
28-
/// segments that do not, so paths for files which have not been created yet can be canonicalized
29-
/// without the failure a plain <c>realpath</c> would produce.
29+
/// Directories that do not exist yet are appended unresolved, so a path for a file the build has not
30+
/// written can be canonicalized without the failure a plain <c>realpath</c> would produce.
31+
/// </para>
32+
/// <para>
33+
/// The canonical form is only ever used for comparison. Callers keep the original item spec so the
34+
/// paths surfaced to the rest of the build stay in the spelling the user provided.
3035
/// </para>
3136
/// </remarks>
3237
internal sealed class PathCanonicalizer
@@ -44,114 +49,144 @@ internal sealed class PathCanonicalizer
4449
? StringComparer.Ordinal
4550
: StringComparer.OrdinalIgnoreCase;
4651

52+
/// <summary>The <see cref="StringComparison"/> matching <see cref="Comparer"/>.</summary>
53+
public static StringComparison Comparison { get; } =
54+
RuntimeInformation.IsOSPlatform(OSPlatform.Linux)
55+
? StringComparison.Ordinal
56+
: StringComparison.OrdinalIgnoreCase;
57+
4758
static readonly MethodInfo ResolveDirectoryLinkTarget =
4859
typeof(Directory).GetMethod("ResolveLinkTarget", BindingFlags.Public | BindingFlags.Static, null, new[] { typeof(string), typeof(bool) }, null);
4960

50-
static readonly MethodInfo ResolveFileLinkTarget =
51-
typeof(File).GetMethod("ResolveLinkTarget", BindingFlags.Public | BindingFlags.Static, null, new[] { typeof(string), typeof(bool) }, null);
52-
53-
readonly Dictionary<string, string> directoryCache = new Dictionary<string, string>(Comparer);
61+
// Cache keys are exact spellings. Two directories whose names differ only by case can be two
62+
// different directories on a case sensitive volume, so a case insensitive key could hand back
63+
// another directory's resolved target.
64+
readonly Dictionary<string, string> directoryCache = new Dictionary<string, string>(StringComparer.Ordinal);
5465

5566
/// <summary>
56-
/// Returns a canonical spelling of <paramref name="path"/>, or the input unchanged when it
57-
/// cannot be canonicalized. Never throws.
67+
/// Returns the key to compare <paramref name="path"/> by: its link resolved directory plus its
68+
/// file name unchanged. Returns <see langword="null"/> when the path cannot be interpreted.
5869
/// </summary>
59-
public string Canonicalize(string path)
70+
public string GetComparisonKey(string path)
6071
{
6172
if (string.IsNullOrWhiteSpace(path))
62-
return path;
73+
return null;
6374

6475
string full;
6576
try
6677
{
67-
full = Path.GetFullPath(path);
78+
full = TrimTrailingSeparators(Path.GetFullPath(path));
6879
}
6980
catch (Exception)
7081
{
71-
// An item spec can contain characters that are not valid in a path; leave it alone.
72-
return path;
82+
// An item spec can contain characters that are not valid in a path.
83+
return null;
7384
}
7485

75-
return CanonicalizeFullPath(TrimTrailingSeparators(full), MaxLinkHops);
86+
var parent = Path.GetDirectoryName(full);
87+
var name = Path.GetFileName(full);
88+
89+
// A bare root such as "/" or "C:\" has no file name to keep.
90+
if (string.IsNullOrEmpty(parent) || string.IsNullOrEmpty(name))
91+
return CanonicalizeDirectory(full);
92+
93+
var directory = CanonicalizeDirectory(parent);
94+
95+
return directory is null ? null : Path.Combine(directory, name);
7696
}
7797

7898
/// <summary>
79-
/// Builds a set of canonical paths that can be probed with <see cref="Canonicalize"/> results.
99+
/// Returns <paramref name="directory"/> with every link in it resolved, or <see langword="null"/>
100+
/// when it cannot be interpreted. Segments that do not exist are kept as they are.
80101
/// </summary>
81-
public HashSet<string> CreateSet(IEnumerable<string> paths)
102+
public string CanonicalizeDirectory(string directory)
82103
{
83-
var set = new HashSet<string>(Comparer);
104+
if (string.IsNullOrWhiteSpace(directory))
105+
return null;
84106

85-
if (paths is not null)
107+
string full;
108+
try
109+
{
110+
full = TrimTrailingSeparators(Path.GetFullPath(directory));
111+
}
112+
catch (Exception)
86113
{
87-
foreach (var path in paths)
88-
{
89-
if (!string.IsNullOrWhiteSpace(path))
90-
set.Add(Canonicalize(path));
91-
}
114+
return null;
92115
}
93116

94-
return set;
117+
return Canonicalize(full, MaxLinkHops);
95118
}
96119

97-
string CanonicalizeFullPath(string full, int hops)
120+
/// <summary>
121+
/// Returns whether <paramref name="key"/> names something inside <paramref name="root"/>. Both
122+
/// must already be comparison keys.
123+
/// </summary>
124+
public static bool IsUnder(string key, string root)
98125
{
99-
var parent = Path.GetDirectoryName(full);
100-
var name = Path.GetFileName(full);
126+
if (string.IsNullOrEmpty(key) || string.IsNullOrEmpty(root))
127+
return false;
101128

102-
// A root such as "/" or "C:\" has nothing left to resolve.
103-
if (string.IsNullOrEmpty(parent) || string.IsNullOrEmpty(name))
104-
return full;
129+
if (Comparer.Equals(key, root))
130+
return true;
131+
132+
if (key.Length <= root.Length || !key.StartsWith(root, Comparison))
133+
return false;
105134

106-
return ResolveLink(Path.Combine(CanonicalizeDirectory(parent, hops), name), hops);
135+
// A root that already ends in a separator, such as "/" or "C:\", has no separator to skip.
136+
var last = root[root.Length - 1];
137+
if (last == Path.DirectorySeparatorChar || last == Path.AltDirectorySeparatorChar)
138+
return true;
139+
140+
// Guard against "…/r-backup" being treated as living inside "…/r".
141+
var next = key[root.Length];
142+
return next == Path.DirectorySeparatorChar || next == Path.AltDirectorySeparatorChar;
107143
}
108144

109-
string CanonicalizeDirectory(string directory, int hops)
145+
string Canonicalize(string full, int hops)
110146
{
111-
directory = TrimTrailingSeparators(directory);
112-
113-
if (directoryCache.TryGetValue(directory, out var cached))
147+
if (directoryCache.TryGetValue(full, out var cached))
114148
return cached;
115149

116-
var canonical = CanonicalizeFullPath(directory, hops);
117-
directoryCache[directory] = canonical;
150+
var parent = Path.GetDirectoryName(full);
151+
var name = Path.GetFileName(full);
152+
153+
// A root resolves to itself, which also terminates the walk.
154+
var canonical = string.IsNullOrEmpty(parent) || string.IsNullOrEmpty(name)
155+
? full
156+
: ResolveDirectoryLink(Path.Combine(Canonicalize(parent, hops), name), hops);
157+
158+
directoryCache[full] = canonical;
118159
return canonical;
119160
}
120161

121-
string ResolveLink(string path, int hops)
162+
string ResolveDirectoryLink(string path, int hops)
122163
{
123164
if (hops <= 0)
124165
return path;
125166

126-
var target = GetLinkTarget(path);
167+
var target = GetDirectoryLinkTarget(path);
127168
if (target is null)
128169
return path;
129170

130171
var resolved = TrimTrailingSeparators(target.FullName);
131172
if (Comparer.Equals(resolved, path))
132173
return path;
133174

134-
// The target itself may live under directories that are links, so canonicalize it too.
135-
return CanonicalizeFullPath(resolved, hops - 1);
175+
// The target itself may live under directories that are links.
176+
return Canonicalize(resolved, hops - 1);
136177
}
137178

138-
static FileSystemInfo GetLinkTarget(string path)
179+
static FileSystemInfo GetDirectoryLinkTarget(string path)
139180
{
140-
// Directory/File.ResolveLinkTarget only exist on .NET 6 and later. This assembly targets
181+
// Directory.ResolveLinkTarget only exists on .NET 6 and later. This assembly targets
141182
// netstandard2.0 so that it can also load into MSBuild.exe on .NET Framework, where link
142-
// resolution is simply unavailable and comparison falls back to the lexical full path.
143-
var resolve = Directory.Exists(path)
144-
? ResolveDirectoryLinkTarget
145-
: File.Exists(path)
146-
? ResolveFileLinkTarget
147-
: null;
148-
149-
if (resolve is null)
183+
// resolution is unavailable and comparison falls back to the lexical full path.
184+
if (ResolveDirectoryLinkTarget is null || !Directory.Exists(path))
150185
return null;
151186

152187
try
153188
{
154-
return resolve.Invoke(null, new object[] { path, /* returnFinalTarget: */ true }) as FileSystemInfo;
189+
return ResolveDirectoryLinkTarget.Invoke(null, new object[] { path, /* returnFinalTarget: */ true }) as FileSystemInfo;
155190
}
156191
catch (Exception)
157192
{

src/SingleProject/Resizetizer/src/nuget/buildTransitive/Microsoft.Maui.Resizetizer.After.targets

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -815,9 +815,12 @@
815815
</ItemGroup>
816816

817817
<!-- Compare canonical paths rather than item specs. The wildcard above and the collected images
818-
can legitimately spell the same file differently (symlinked project paths, separators,
819-
casing), and a textual Remove would then classify every generated image as stale. -->
818+
can legitimately spell the same file differently (symlinked project paths, casing), and a
819+
textual Remove would then classify every generated image as stale. Root also confines the
820+
delete list to the intermediate folder, because the recursive wildcard descends through any
821+
directory link it finds inside it. -->
820822
<DetectStaleOutputFilesTask
823+
Root="$(_MauiIntermediateImagesFullPath)"
821824
Files="@(_ResizetizerExistingImages->'%(FullPath)')"
822825
KnownOutputs="@(_ResizetizerCollectedImages)">
823826
<Output TaskParameter="StaleFiles" ItemName="_ResizetizerImagesToDelete" />

0 commit comments

Comments
 (0)