Skip to content

[Extensibility] Resizetizer: Enable external backend processing - #36653

Merged
kubaflo merged 24 commits into
net11.0from
kubaflo/35022-resizetizer-extensibility
Jul 30, 2026
Merged

[Extensibility] Resizetizer: Enable external backend processing#36653
kubaflo merged 24 commits into
net11.0from
kubaflo/35022-resizetizer-extensibility

Conversation

@kubaflo

@kubaflo kubaflo commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

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!

Fixes #35022

Summary

Adds a coherent Resizetizer contract for external backends. A backend sets ResizetizerPlatformType and invokes the standard processing targets from its build graph. Images use generic desktop DPI and icon fallbacks, while built-in platform injection remains isolated to the built-in platforms.

MSBuild contract

  • ResizetizerPlatformType opts an external backend into the compatible processing pipeline.
  • MauiProcessedImage, MauiProcessedFont, and MauiProcessedAsset expose processed outputs.
  • ResizetizerAfterImageProcessingTargets, ResizetizerAfterFontProcessingTargets, and ResizetizerAfterAssetProcessingTargets provide post-processing packaging hooks through matching targets.
  • Custom assets default to Link metadata. Item collection is available independently of the built-in platform list.
  • ProcessMauiAssetsDependsOnTargets includes ResizetizeCollectItems for backends that set ResizetizerPlatformType at evaluation time (early opt-in), ensuring project-reference MauiAsset items are collected before ProcessMauiAssets runs.
  • Both early (.props/project) and late (.targets) opt-in paths collect referenced images, fonts, and assets before processing.
  • Incremental image fallback reads the persisted Resizetizer output list only when the current image set is non-empty, so backend artifacts are excluded and removing the final image deletes stale outputs.
  • Font processing self-heals partial cache restores by invalidating mauifont.stamp when an expected deduplicated copied font is missing, without re-exposing removed stale fonts.

Compatibility

Built-in platform DPI definitions, output injection item groups, and incremental input/output stamp names are preserved. Built-in injection was moved verbatim into _ResizetizeInjectPlatformItems; external backends receive no built-in output items.

Incremental builds

In the up-to-date path _ResizetizerCollectedImages / MauiProcessedImage are now restored from the persisted @(_ResizetizerOutputs) list instead of a wildcard glob over $(_MauiIntermediateImages). The wildcard is retained only for stale-file detection and deletion, preventing backend-written artefacts (e.g. *.items files) from being surfaced as processed images on incremental rebuilds.

External app icons

App-icon outputs continue to flow through MauiProcessedImage; backends with platform-specific icon packaging (for example GTK hicolor or macOS .icns) can retain their existing MauiIcon target until a separate processed-icon provenance contract is designed. Ordinary image, font, and asset adoption is fully covered here.

Tests

  • Added unknown-platform DPI fallback and custom-platform image-task coverage.
  • Added Resizetizer integration coverage for both normal and late-import backend opt-in, project-reference resource collection, and no built-in output injection.
  • Added incremental-rebuild regression test: makes the stamp appear stale, triggers the up-to-date path, and asserts backend-written *.items artefacts are absent from MauiProcessedImage.
  • Added deterministic partial-cache regressions using controlled future timestamps: a missing image output list reruns image processing, and a missing copied font reruns font processing and restores MauiProcessedFont.
  • Added early opt-in project-reference asset test (CustomBackendEarlyOptInCollectsReferencedAssets): verifies that a backend setting ResizetizerPlatformType at evaluation time receives referenced-project MauiAsset items in MauiProcessedAsset after ProcessMauiAssets.
  • The current head passes 362 focused DpiPathTests/ResizetizeImagesTests cases. The full suite has 577 passing and the same 20 missing-baseline SkiaSharpAppIconToolsTests.Resize.BasicTest failures that reproduce on origin/net11.0 (570 passing, identical 20 failures), so they are unrelated to this PR.

Pre-update validation (2eefa6df)

  • Rebuilt and reinstalled the local 11.0.0-dev package.
  • CustomBackendProcessesImagesWithoutBuiltInOutputInjection and CustomBackendLateImportCollectsReferencedResourcesWithoutBuiltInOutputInjection pass 2/2 after formatting.
  • All review threads are resolved; current-head improved review and CI are retriggered.

Updated-head validation (4f7ec2f6)

  • The update commit has the exact tested head plus net11.0 cbb408ff as parents.
  • The Resizetizer target and integration-test files are byte-for-byte unchanged from 2eefa6df; git diff --check passes.
  • Fresh improved review and CI builds 1530350 / 1530356 / 1530363 are active.

Current-head review fix (881f5cacbd)

  • Font cache self-healing now also invalidates mauifont.stamp when an iOS/MacCatalyst MauiInfo.plist side artifact is missing while MauiFont items remain, restoring both the plist and PartialAppManifest contract.
  • Rebuilt the 11.0.0-dev Resizetizer package and passed CustomBackendLateImportCollectsReferencedResourcesWithoutBuiltInOutputInjection 1/1, including selective plist-loss recovery.
  • The current-head Copilot review is clean with zero unresolved threads; replacement CI builds 1530550 / 1530552 / 1530554 are active.

Timestamp sentinel hardening (26f818ea0e, 8145ebaadd)

  • Future-dated incremental-test sentinels use a one-hour margin so slow package restore/build work cannot overtake them.
  • Image-output-list and iOS plist recovery assertions now compare against the timestamp re-read from disk after SetLastWriteTimeUtc, preserving correctness on filesystems with coarse timestamp granularity.
  • CustomBackendProcessesImagesWithoutBuiltInOutputInjection and CustomBackendLateImportCollectsReferencedResourcesWithoutBuiltInOutputInjection pass 2/2 through the integration-test runner with a locally packed 11.0.0-dev Resizetizer package.
  • All review threads are resolved; current-head improved review and full CI are active.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 66f84348-6476-4097-8b7f-f240338e85c3
Copilot AI review requested due to automatic review settings July 18, 2026 14:46
@kubaflo
kubaflo temporarily deployed to copilot-pat-pool July 18, 2026 14:46 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36653

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36653"

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enables external (non-built-in) backends to participate in the Resizetizer pipeline by introducing an opt-in MSBuild contract, adding generic desktop DPI/icon fallbacks, and exposing processed output item groups/hook targets so backends can package outputs without inheriting built-in platform injection.

Changes:

  • Add ResizetizerPlatformType opt-in and split image processing from built-in output injection; introduce MauiProcessedImage/Font/Asset and ResizetizerAfter*Processing hook targets.
  • Add generic desktop DPI and app-icon fallbacks in DpiPath for unknown/custom platforms (including null-safety).
  • Add unit/integration test coverage for unknown-platform fallbacks and the custom-backend MSBuild contract.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/TestUtils/src/Microsoft.Maui.IntegrationTests/ResizetizerTests.cs Adds an integration test validating custom backends receive processed images and do not get built-in output injection.
src/SingleProject/Resizetizer/test/UnitTests/ResizetizeImagesTests.cs Adds a task-level test ensuring custom platform image processing uses the generic desktop fallback outputs.
src/SingleProject/Resizetizer/test/UnitTests/DpiPathTests.cs Updates tests to assert fallback behavior for unknown platforms (original/dpis/app icon dpis).
src/SingleProject/Resizetizer/src/nuget/buildTransitive/Microsoft.Maui.Resizetizer.After.targets Introduces external-backend opt-in, processed output item groups, after-processing hooks, and isolates built-in platform injection.
src/SingleProject/Resizetizer/src/DpiPath.cs Adds Generic DPI definitions and makes GetOriginal/GetDpis/GetAppIconDpis return fallbacks (and handle null platform).

@kubaflo
kubaflo temporarily deployed to copilot-pat-pool July 18, 2026 14:50 — with GitHub Actions Inactive
@github-actions github-actions Bot added the area-tooling XAML & C# Hot Reload, XAML Editor, Live Visual Tree, Live Preview, Debugging label Jul 18, 2026
@kubaflo
kubaflo temporarily deployed to copilot-pat-pool July 18, 2026 14:50 — with GitHub Actions Inactive
@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 18, 2026
Register processed images in the processing target so external backends receive the same clean tracking as built-in platform injection paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 66f84348-6476-4097-8b7f-f240338e85c3
Copilot AI review requested due to automatic review settings July 18, 2026 15:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

@MauiBot MauiBot added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-gate-failed AI could not verify tests catch the bug s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Jul 18, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 2 findings

See inline comments for details.

Comment thread src/SingleProject/Resizetizer/src/DpiPath.cs
@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 18, 2026
kubaflo pushed a commit that referenced this pull request Jul 18, 2026
…ILED

Two robustness fixes to verify-tests-fail.ps1 so genuine agent/infrastructure
failures never surface as a false FAILED gate:

1. Native shared-library load failure (DllNotFoundException / "Unable to load
   shared library"/"Unable to load DLL") → EnvError/INCONCLUSIVE. When the gate
   detects a PR test at CLASS level and runs the whole class on an agent missing
   a native dependency (e.g. libSkiaSharp on a Linux/android gate agent), the
   image-rasterization tests fail in BOTH the without-fix and with-fix runs — a
   false FAILED, even though the PR's own logic tests pass and real maui-pr CI
   (Windows Helix Unit Tests) passes them. A native-load error means the test
   could not run, so nothing about the fix was verified. SAFE: a genuine "fix
   does not work" surfaces as an assertion diff, never a missing native library.
   (build 14699033, PR #36653 [Build] Resizetizer external backend.)

2. Device/simulator boot failure → exit 3 (INCONCLUSIVE), not exit 1 (FAILED).
   A device that will not boot fails BEFORE the PR's code is built or run, so it
   can never be caused by the fix. Every other env failure in this script exits
   3; this path relied on the caller's fragile "missing report after a non-zero
   exit" heuristic to reclassify, which a partial/prior report without the
   ENV ERROR marker would break into a false FAILED. Keeps the literal "Failed
   to boot device" phrase so Review-PR.ps1 fallback diagnostics still match.
   (PR #35668 iOS: CoreSimulatorService wedge — "No iPhone simulator found"
   after the full create/enroll recovery — must be non-blocking INCONCLUSIVE.)

Adds 3 Pester regression tests (libSkiaSharp DllNotFound, Windows
Unable-to-load-DLL, and a negative test that a genuine assertion failure is NOT
misclassified as a native-lib env error). All 23 tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
Cover null platform fallbacks and report a clear assertion when the custom backend output list is not produced.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 66f84348-6476-4097-8b7f-f240338e85c3
@kubaflo

This comment has been minimized.

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/TestUtils/src/Microsoft.Maui.IntegrationTests/ResizetizerTests.cs:302

  • This test sets the font stamp only 1 minute into the future, then later asserts the rerun caused the stamp to become earlier than that value. On slower machines where the rebuild takes >1 minute, the new stamp time can be after the sentinel and fail even though the rerun happened. Use a sentinel further in the future.
		File.SetLastWriteTimeUtc(fontStampFile, DateTime.UtcNow.AddMinutes(1));

src/TestUtils/src/Microsoft.Maui.IntegrationTests/ResizetizerTests.cs:335

  • Same flakiness risk as above: a 1-minute future sentinel can be exceeded by a slow build, making the "stamp < sentinel" check fail even when recovery succeeded. Use a sentinel further in the future.
		var plistRecoverySentinel = DateTime.UtcNow.AddMinutes(1);

src/TestUtils/src/Microsoft.Maui.IntegrationTests/ResizetizerTests.cs:165

  • The invalidation sentinel is only 1 minute in the future. If the build/run takes longer than that, the stamp rewrite time can end up after the sentinel even when invalidation works, making this assertion flaky. Use a further-future timestamp (e.g., +1 hour) to keep the "stamp < sentinel" check robust on slow CI machines.

This issue also appears in the following locations of the same file:

  • line 302
  • line 335
		var invalidationSentinel = DateTime.UtcNow.AddMinutes(1);

Use one-hour future timestamps so slow integration builds cannot overtake invalidation sentinels and produce false failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 66f84348-6476-4097-8b7f-f240338e85c3
@kubaflo

This comment has been minimized.

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up 26f818ea0e moves all Resizetizer future timestamp sentinels from +1 minute to +1 hour, preventing slow rebuilt-package integration runs from overtaking the sentinel and failing a correct invalidation. The integration test project builds cleanly with the exact preview7 SDK; fresh full CI is running.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • The explanatory comment for WriteLinesToFile hard-codes approximate line numbers ("line ~671" / "line ~663"), which are already out of date after the refactor and will quickly drift again. This makes the guidance harder to follow and maintain; refer to the Outputs attribute / recovery path without embedding line references.
        <!-- NOTE: do NOT set WriteOnlyWhenDifferent="true" here. This file is listed in the
             target's Outputs (line ~671), so its mtime is the target's up-to-date signal.
             The file's content is only the SET of output filenames, so editing an existing
             MauiImage's pixels leaves the set (and thus the file) unchanged — with
             WriteOnlyWhenDifferent the file wouldn't be rewritten, its mtime would stay behind

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Current-head CI evidence (builds still active): device build 1531544 currently has failed MacCatalyst CoreCLR, iOS Mono, and iOS CoreCLR legs. Current net11.0 baseline device build 1531485 has the exact same three failed job names. This is current-base evidence that the device reds are unrelated to the Resizetizer changes; remaining jobs are still pending.

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Updated current-head classification: UI build 1531542 failed Controls Label before tests because vstsagenttools.blob.core.windows.net could not resolve while provisioning Appium tooling; no TRX was produced. Device 1531544 continues to accumulate the same failed job names as current net11.0 baseline 1531485. Both builds remain active; UI is a justified rerun after completion.

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Final UI 1531542 classification: Material3 API36, API30 Shell, and API30 CollectionView match the current net11.0 baseline 1531376; the Editor/Effects group failed across unrelated sibling PRs; Controls Label never ran tests because Appium tooling DNS failed. None correlate with the Resizetizer build-target diff. A UI-only replacement run is justified.

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/azp run maui-pr-uitests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial re-review — Round 4 (HEAD 26f818ea "Harden Resizetizer timestamp sentinels")

Methodology: 3 independent reviewers with adversarial consensus, plus orchestrator empirical MSBuild probing. Fresh diff re-fetched at HEAD; every reviewer ran a real synthetic dotnet build probe (this is incremental-build code where reading the targets is not sufficient). COMMENT only — no approval implied. CI status is out of scope.

Scope of 26f818ea

Verified: the commit changes only ResizetizerTests.cs (+3/-3). It swaps three future-dating sentinels DateTime.UtcNow.AddMinutes(1)AddHours(1) on the three <-"reran" assertions (L165 invalidationSentinel, L302 font stamp, L335 plistRecoverySentinel). The line‑148 no-op steady-state sentinel is correctly left at AddMinutes(1) — its assertion is Assert.Equal (stamp unchanged = skip), which is build-duration-independent. No production targets were touched, no [Fact] removed, no assertion loosened, no scenario deleted.

Sentinel hardening — verdict (adjudicating the round‑3 note)

The round‑3 💡 was: "AddMinutes(1) is a 60s magic constant a cold CI build could exceed." Verified in both directions:

  • (a)(i) Slow/cold build false-FAILURE — ✅ RESOLVED. The < assertion requires the heal-triggered rebuild to finish within the margin. Reviewer 3 empirically reproduced the old failure mode (3s margin + 5s in-target delay → a correct rerun asserted RED), then confirmed AddHours(1) (3600s headroom) turns it GREEN. This is exactly the cold-CI flake the commit set out to kill, and it does.
  • (a)(ii) Coarse-FS granularity / clock skew false-PASS — not a practical risk; unchanged by this commit. All three reviewers independently reasoned that the only way < passes without a rerun is a filesystem that truncates SetLastWriteTimeUtc below the in-memory value (FAT 2s / legacy HFS+ 1s). On NTFS/ext4/APFS (the actual CI backing stores) the round-trip is exact (orchestrator + reviewers measured delta = 0), so the gate holds. Crucially, the truncation gap is margin-independentAddHours neither introduces nor worsens nor fixes it; it is a pre-existing test-robustness subtlety (see 💡‑1).
  • (b) No production wall-clock dependence — ✅ CONFIRMED. grep -iE 'AddHours|AddMinutes|DateTime|UtcNow|sentinel' over the HEAD targets file returns zero matches. All sentinel arithmetic lives purely in the integration test; production up-to-date logic is MSBuild Inputs/Outputs mtime + Exists() only. Nothing breaks on coarse or networked/mounted filesystems.
  • (c) Gate strength — ✅ NOT WEAKENED (proven by revert). Reviewers 1 and 3 each modeled the fixed vs reverted-fix targets for all three hardened sites. In every reverted-fix case the target skips, the stamp stays exactly equal to the sentinel, and stamp < sentinel correctly flips RED. Widening 1min→1h does not move the reverted-case equality, so no real regression is masked.

Mandatory incremental-build probes (real dotnet build, 3/3 + orchestrator)

Probe Result
1. No perpetual rebuild fresh → RAN; 5–6 consecutive no-change builds → SKIP every time ✅
2. Content-edit recovery edit one image byte (same output filename set) → exactly one ResizeImages rerun → then SKIP ✅
3. Font/plist heal delete only MauiInfo.plist → one rerun regenerates it → SKIP; delete only a .ttf copy → one recovery rerun → SKIP ✅
4. No false heal on removal remove a font from the project → heal never fires, processed-font contract correctly empty, settles to SKIP (no resurrection, no perpetual loop) ✅
5. Round-3 A/B/C intact (A) mauiimage.outputs rewritten every run; (B) .ttf heal via the @(MauiFont)→expected-path predicate; (C) iOS MauiInfo.plist heal entry — all reproduced ✅

Prior findings — status

# Finding (round raised) Status at HEAD 26f818ea
A mauiimage.outputs written with WriteOnlyWhenDifferent → incremental regression ✅ Resolved (re-confirmed)
B .ttf heal missing → processed-font path exposed after partial restore ✅ Resolved (re-confirmed)
C iOS MauiInfo.plist not healed on plist-only partial restore ✅ Resolved by 881f5cac (re-confirmed)
💡 AddMinutes(1) 60s sentinel margin ✅ Addressed by 26f818ea (a)(i)

Non-blocking follow-ups

  • 💡 Sentinel consistency (L165, L335) — see the inline comment. Two of the three hardened < sentinels compare an FS-read stamp against the in-memory DateTime sentinel, whereas the font-copy site (L318) compares against a value re-read from disk at L303. On a coarse-granularity filesystem a truncated-on-write stamp could make a missed rerun read as green. Pre-existing (only the margin changed here) and inert on CI's sub-second filesystems; worth mirroring L303's read-back pattern for cross-FS robustness. Non-blocking.
  • 💡 Image content-edit → steady-state-skip test gap — still OPEN. No integration test edits an existing image's content (same output filename set) then asserts one rerun + next-build SKIP. Reviewer 3 empirically showed the existing L163–175 test passes identically with or without the WriteOnlyWhenDifferent regression (it deletes the outputs file wholesale, and WriteLinesToFile always recreates a missing file), so it does not gate that regression. 26f818ea added no such test. Production logic is correct (probe 2); the gap is purely missing coverage.
  • 💡 Pre-existing packaging ghost-font — still OPEN, unchanged. _MauiFontCopied Include="$(_MauiIntermediateFonts)*" (targets ~L599) has no stale-copy <Delete> (unlike the image path's delete step), so a removed font's physical copy keeps bundling until a clean. Predates the PR; the processed-font extensibility contract itself stays clean (probe 4). Untouched by 26f818ea.

Verdict

26f818ea is a correct, minimal, test-only hardening: it closes a real cold-CI false-failure without weakening any regression gate (proven RED-on-revert across all three hardened sites), adds no production wall-clock dependence, and regresses none of the round‑3 resolutions. No blocking findings. The three carried-forward items are non-blocking and unchanged in severity by this commit. Mergeable on code merit.

Comment thread src/TestUtils/src/Microsoft.Maui.IntegrationTests/ResizetizerTests.cs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 66f84348-6476-4097-8b7f-f240338e85c3
@kubaflo

This comment has been minimized.

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

@kubaflo

kubaflo commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Review follow-up 8145ebaaddb hardens both future timestamp sentinels by re-reading the filesystem-rounded value after SetLastWriteTimeUtc. The focused Resizetizer integration tests pass 2/2 through the repository integration-test runner using a locally packed 11.0.0-dev Resizetizer package. The review thread is resolved; exact improved review and full CI are active.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/TestUtils/src/Microsoft.Maui.IntegrationTests/ResizetizerTests.cs:335

  • fontStampFile is deleted (line 329) and then immediately used with File.SetLastWriteTimeUtc without re-asserting it was recreated by the build. If the build regresses and the stamp isn’t produced, this will fail with a less-informative FileNotFoundException rather than the intended assertion message. Add an explicit File.Exists assertion before touching the stamp.
		File.SetLastWriteTimeUtc(fontStampFile, DateTime.UtcNow.AddHours(1));

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 5 re-review — HEAD 8145ebaa "Harden Resizetizer timestamp comparisons". Methodology: 3 independent reviewers with adversarial consensus, plus orchestrator empirical MSBuild + timestamp probing. Event: comment only (CI status out of scope — the net11.0 base is red for unrelated reasons).

Scope of 8145ebaa

Test-only (+4/-4 in ResizetizerTests.cs). Production Microsoft.Maui.Resizetizer.After.targets is byte-identical to the round-4 HEAD (26f818ea) — no production behavior change in this commit. So this round is a gate-strength / test-integrity verification, not a production-risk review.

Round-4 💡 (coarse-FS false-green sentinel) → ✅ RESOLVED at root cause

Round 4 flagged that the two <-reran sentinels compared a disk-truncated stamp against a never-truncated in-memory DateTime, so on a coarse-granularity filesystem (FAT 2s / legacy HFS+ 1s) a genuinely missed rerun (stamp untouched) could still satisfy stamp < sentinel and pass — a silently-green gate. The recommended fix was to re-read the sentinel from disk after the Set.

8145ebaa implements exactly that at both sites — invalidationSentinel (L166) and plistRecoverySentinel (L336) now SetLastWriteTimeUtc(…AddHours(1)) then var sentinel = File.GetLastWriteTimeUtc(stamp). The third <-site (the .ttf case, L318) already used an on-disk read-back baseline (noOpFontStampWriteTime, L303) and is correctly left unchanged.

All three reviewers independently modeled this in C# with simulated 1s/2s truncation. It eliminates the truncation dependence rather than moving it: on revert, assertion-time Get(stamp) and the read-back sentinel are two reads of the same untouched on-disk byte ⇒ exactly equal ⇒ strict < is false ⇒ RED on every filesystem. On fix-present, Touch rewrites the stamp to real-now (~1h below the future sentinel) ⇒ < true ⇒ GREEN.

FS granularity Old in-memory (reverted) Read-back (reverted) Read-back (fixed)
ns (APFS/ext4/NTFS) RED RED GREEN
1s (HFS+) false-GREEN RED GREEN
2s (FAT) false-GREEN RED GREEN

No new failure mode: both sides use …Utc (no local/UTC skew), Get-immediately-after-Set returned the stored value in every probe, and the +1h margin is ~1800–3600 coarse buckets away from real "now", so a fix-present false-RED would require a single build step lasting ~1 hour. On real CI filesystems both patterns were already RED-on-revert, so this changes no CI behavior — it's pure hardening for coarse-FS dev machines.

Gate-strength (fixed-vs-reverted, real MSBuild) — all three <-sites remain live

Site Assertion Fixed Reverted-fix
image outputs-file heal L172 < invalidationSentinel GREEN (reran) RED (stamp untouched)
.ttf copy heal L318 < noOpFontStampWriteTime GREEN (reran) RED (stamp untouched)
iOS MauiInfo.plist heal L340 < plistRecoverySentinel GREEN (reran) RED (stamp untouched)

None of the hardened comparisons became vacuous/always-true — each still flips RED when its production fix is reverted. (Calibration note: the correct revert model for the image site is dropping the outputs file from the target's Outputs= list, not toggling WriteOnlyWhenDifferent, since a deleted outputs file is always rewritten regardless of write-mode. Under the correct model the gate is genuine.)

Incremental-build probes at HEAD — all pass

  1. No perpetual rebuild — fresh → RAN; 5–6 consecutive no-change builds → SKIP every time.
  2. Content-edit recovery — edit an image byte → exactly one ResizeImages rerun → then steady-state SKIP.
  3. Heal correctness — delete only MauiInfo.plist → one rerun regenerates it → SKIP; delete only a .ttf copy → one recovery rerun → SKIP.
  4. No false heal — remove a font from the project → not resurrected into the contract, no perpetual heal loop.
  5. The four previously-certified fixes are intact: (A) mauiimage.outputs rewritten every ResizeImages run; (B) .ttf heal via the @(MauiFont)→expected-path predicate; (C) iOS MauiInfo.plist heal (881f5cac); (D) AddHours(1) sentinel margins (26f818ea).

Test integrity

No [Fact]/[Theory] removed, no assertion loosened, no scenario dropped — the diff is exactly the two described read-back hunks; Fact/Assert counts are unchanged from round 4. The line-148 no-op steady-state sentinel correctly stays AddMinutes(1) (its Assert.Equal is build-duration-independent).

Still-open non-blocking items (unchanged by 8145ebaa)

  • 💡 Image content-edit → steady-state-skip test gap (open): no integration test mutates an existing image's pixel content and asserts the next build skips. Production behaves correctly (probe 2), but a future regression re-introducing WriteOnlyWhenDifferent on mauiimage.outputs — the exact hazard the code comment warns about — would not be caught by a test. Consider adding edit → one-rerun → steady-state-skip coverage.
  • 💡 Pre-existing packaging ghost-font (out of scope, predates this PR): _MauiFontCopied Include="$(_MauiIntermediateFonts)*" (After.targets ~L599) is a wildcard glob with no stale-copy Delete, so a removed font's old copy keeps getting bundled until a clean (unlike the image path's explicit cleanup). Not this PR's lines.

Verdict

8145ebaa correctly resolves the round-4 💡, closing a real coarse-filesystem gate-strength hole without touching production or introducing new failure modes; all three <-gates remain RED-on-revert and every incremental-build probe passes. No new blocking findings — merge-ready on code merit (3/3 reviewers), with the two 💡 items above as optional non-blocking follow-ups.

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial code review

Two new correctness findings survived adversarial validation. PR #36653 merged at the same reviewed HEAD (8145ebaa) while this review was running, so these are follow-up findings against the merged code.

❌ Findings

  1. Legacy AfterTargets="ResizetizeImages" ordering regressed — 2/3 reviewers after dispute independently reproduced that a consumer target declared before the package import now runs before _ResizetizeInjectPlatformItems and sees empty built-in platform item groups. Previously those items were populated inside ResizetizeImages, before any AfterTargets consumer could run.
  2. External splash processing is silently incomplete — 2/3 reviewers after dispute confirmed external opt-in marks the project compatible and schedules ProcessMauiSplashScreens, but all generators are conditioned on built-in platforms. The target writes its stamp while producing no splash output or backend hook, despite issue #35022 explicitly requiring MauiSplashScreen processing through the shared pipeline.

Existing open non-blocking feedback

  • 💡 ResizetizerPlatformType is absent from the persisted image input manifest, so changing only the effective platform selector can reuse platform-shaped image outputs from the previous value. This was already raised in an earlier review and remains open; 2/3 current reviewers reconfirmed it.
  • 💡 The integration tests still lack the discriminating content-edit sequence (edit an existing image, rebuild once, then assert the next unchanged build skips) that gates the WriteOnlyWhenDifferent perpetual-rerun fix. This was already posted previously; all current reviewers who adjudicated it agreed it remains open.
  • The pre-existing _MauiFontCopied stale-copy packaging behavior remains outside this PR's provenance.

What looks right

The partial-cache healing and timestamp hardening from the later remediation commits remain sound: independent real MSBuild probes confirmed fresh-build/no-op behavior, one-shot image/font/plist recovery, no perpetual heal on legitimate font removal, and RED-on-revert timestamp gates.

Metadata reconciliation

The title is accurate, and the description is detailed, but the final implementation does not satisfy the linked issue's splash-screen success criterion or its claim that existing platform behavior remains identical for consumers of AfterTargets="ResizetizeImages".

Methodology: 3 independent reviewers with adversarial consensus + MAUI repo domain specialist. CI status intentionally not evaluated.

DependsOnTargets="$(ResizetizerAfterImageProcessingTargets)"
Condition="'$(EnableMauiImageProcessing)' == 'true' And '$(ResizetizerAfterImageProcessingTargets)' != ''" />

<Target Name="_ResizetizeInjectPlatformItems"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression — Preserve the existing AfterTargets="ResizetizeImages" completion contract

Flagged by: 2/3 reviewers after dispute

This extraction makes _ResizetizeInjectPlatformItems a sibling AfterTargets="ResizetizeImages" target. Real MSBuild probes reproduced that sibling AfterTargets targets run in declaration/import order, so an existing consumer target declared in the project before the package import now executes first and observes empty BundleResource, ContentWithTargetPath, LibraryResourceDirectories, etc. Before this PR, injection happened inside ResizetizeImages, so every AfterTargets="ResizetizeImages" consumer necessarily ran after those items existed.

Keep ResizetizeImages as the compatibility completion target: move processing to a private inner target, run the new backend hook and built-in injection from that inner target, and let the public ResizetizeImages target complete only after injection. That preserves the new seam without changing legacy observer ordering.

</PropertyGroup>

<!-- External backends opt in by setting ResizetizerPlatformType in their targets. -->
<PropertyGroup Condition="'$(_ResizetizerIsCompatibleApp)' != 'True' And '$(ResizetizerPlatformType)' != ''">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic — External opt-in schedules splash processing but silently produces no output

Flagged by: 2/3 reviewers after dispute

This new compatibility branch also activates the existing ResizetizeDependsOnTargets, which includes ProcessMauiSplashScreens. For an external ResizetizerPlatformType, however, every splash generator in that target is conditioned on a built-in platform flag; none runs, yet the target still touches _MauiSplashStampFile. There is also no MauiProcessedSplashScreen or post-processing hook. The result is a successful-looking build that drops the configured splash output.

Issue #35022 explicitly lists "MauiSplashScreen processed by the same pipeline" as a success criterion. Add a generic splash-processing/output contract and backend hook with regression coverage, or avoid marking external projects compatible with this stage and narrow the linked issue/PR contract accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-tooling XAML & C# Hot Reload, XAML Editor, Live Visual Tree, Live Preview, Debugging s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-gate-passed AI verified tests catch the bug (fail without fix, pass with fix) s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants