[ci] Remove trimming workaround#33624
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes a workaround that disabled trim analysis for MAUI Controls builds. The workaround was originally added to address an upstream issue in xamarin-macios that has since been resolved.
Changes:
- Removed conditional property group that disabled
EnableTrimAnalyzerand enabledSuppressTrimAnalysisWarnings
|
/azp run maui-pr-devicetests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
When integration tests fail to build, extract and display errors from the binlog file directly to the console. This makes errors visible in Azure DevOps task logs without requiring artifact downloads. - Add OutputBuildErrorsFromBinLog() method to BuildWarningsUtilities - Call it automatically when Build() or Publish() fails in DotnetInternal - Limits output to 50 errors by default for readability
This makes Console.WriteLine output from tests visible in Azure DevOps task logs, including the build errors extracted from binlogs when a build fails.
- Add ITestOutputHelper parameter to all utility methods (DotnetInternal, ToolRunner, XHarness, Adb, Emulator, Simulator) - Replace Console.WriteLine with output?.WriteLine for proper xUnit test output - Add ShowLiveOutput=true to csproj for CI visibility - All parameters are optional to maintain backward compatibility
Updated all call sites in integration tests to pass _output to: - DotnetInternal.Build() - DotnetInternal.New() - DotnetInternal.Publish() - DotnetInternal.Run() - DotnetInternal.RunForOutput() - XHarness.RunAppleForTimeout() This enables ITestOutputHelper output to flow through to Azure DevOps pipeline logs via ShowLiveOutput=true, making build errors visible directly in CI logs instead of only in binlog artifacts.
- Update ToolRunner.cs to use ITestOutputHelper instead of Console.WriteLine - Update DotnetInternal.cs to pass ITestOutputHelper, add OutputBuildErrorsFromBinLog - Update XHarness.cs to pass ITestOutputHelper to all methods - Update Adb.cs and Emulator.cs (Android) to use ITestOutputHelper - Update Simulator.cs (Apple) to store ITestOutputHelper as field - Update BuildWarningsUtilities.cs with new OutputBuildErrorsFromBinLog method - Update all test files to pass output: _output to DotnetInternal and XHarness calls - Add ShowLiveOutput property to Microsoft.Maui.IntegrationTests.csproj Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Port logging improvements from #33624 to make Integration Test output visible in CI. `Console.WriteLine` output is not captured by Azure DevOps; `ITestOutputHelper` is. ## Changes ### Utility Classes - **ToolRunner.cs, DotnetInternal.cs, XHarness.cs** - Add optional `ITestOutputHelper? output` parameter, replace `Console.WriteLine` with `output?.WriteLine()` - **BuildWarningsUtilities.cs** - Add `OutputBuildErrorsFromBinLog()` to extract/display build errors on failure - **Adb.cs, Emulator.cs, Simulator.cs** - Propagate `ITestOutputHelper` through Android/Apple utilities ### Test Files - All test files pass `output: _output` to `DotnetInternal.New()`, `DotnetInternal.Build()`, `XHarness.Run*()` calls ### Project - Add `ShowLiveOutput=true` to csproj ## Example ```csharp // Before Assert.True(DotnetInternal.Build(projectFile, config, properties: BuildProps), $"Project failed to build."); // After Assert.True(DotnetInternal.Build(projectFile, config, properties: BuildProps, output: _output), $"Project failed to build."); ``` **Excluded:** `src/Controls/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.Common.targets` as requested. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `ghcr.io` > - Triggering command: `/tmp/dependabot-cli/dependabot/dependabot /tmp/dependabot-cli/dependabot/dependabot graph nuget org/repo --local /home/REDACTED/work/maui/maui --branch main --directory src/TestUtils/src/Microsoft.Maui.IntegrationTests --proxy-cert /home/REDACTED/work/_temp/runtime-logs/mkcert/rootCA.pem --updater-env NODE_EXTRA_CA_CERTS=/usr/local/share/ca-certificates/dbot-ca.crt git /bin/test --local credential.helpe/home/dependabot/dependabot-updater/bin/main.ps1 /usr/bin/git basename` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/dotnet/maui/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > Create apr with all the logging Fixes from this pr #33624 which should be everything except this file src/Controls/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.Common.targets </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
|
/rebase |
|
@simonrozsival can it be merged? |
|
/review -b feature/refactor-copilot-yml |
|
/review -b feature/refactor-copilot-yml -p ios |
This comment has been minimized.
This comment has been minimized.
|
/review -b feature/enhanced-reviewer -p android |
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@rmarinho — new AI review results are available based on this last commit:
4a2f66e.
Merge branch 'main' into fix-trimming To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
UI Tests
Full UI test matrix will run (no specific categories detected from PR changes).
Pre-Flight — Context & Validation
Issue: N/A - No linked issue detected
PR: #33624 - [Housekeeping] Remove EnableTrimAnalyzer from Microsoft.Maui.Controls.Common.targets
Platforms Affected: All platforms consuming Microsoft.Maui.Controls.Common.targets; testing platform requested: android
Files Changed: 1 implementation/build target, 0 test
Key Findings
- PR removes a temporary MSBuild workaround that disabled
EnableTrimAnalyzerand suppressed trim analysis warnings when_MauiDoNotConfigureTrimAnalyzer != true,PublishAot != true, andTrimMode != full. - No tests were detected by the pre-run gate; author should consider adding coverage or a validation scenario for trim analyzer behavior.
- Impact is build-time analyzer configuration, not runtime UI behavior. No UI test categories were identified.
Code Review Summary
Verdict: NEEDS_DISCUSSION
Confidence: medium
Errors: 0 | Warnings: 1 | Suggestions: 0
Key code review findings:
⚠️ CI was not green at review time, so LGTM was not appropriate even though no code findings were identified.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #33624 | Remove the temporary EnableTrimAnalyzer=false / SuppressTrimAnalysisWarnings=true workaround entirely. |
src/Controls/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.Common.targets |
Original PR |
Code Review — Deep Analysis
Code Review — PR #33624
Independent Assessment
What this changes: Removes the Microsoft.Maui.Controls.Common.targets workaround that set EnableTrimAnalyzer=false and SuppressTrimAnalysisWarnings=true for non-AOT/non-full-trim builds.
Inferred motivation: The deleted block was marked as temporary for xamarin-macios#21351; removing it restores normal trim analyzer behavior for MAUI consumers.
Reconciliation with PR Narrative
Author claims: Cleanup after the related Xamarin/macios issue was resolved.
Agreement/disagreement: Matches the diff. I verified dotnet/macios#21351 was merged.
Findings
No code findings. Expert review produced no inline findings; inline-findings.json contains [].
⚠️ Warning — CI is not green
Latest checks for head 4a2f66e show maui-pr and Build Analysis failing, with multiple integration-test failures/cancellations and one check still in progress. I did not find evidence tying failures directly to this one-line MSBuild cleanup, but LGTM is not appropriate while required CI is red.
Devil's Advocate
The main risk is that re-enabling trim diagnostics could expose warnings in consumer builds. That appears intentional and consistent with the upstream macios fix, but CI should validate it.
Verdict: NEEDS_DISCUSSION
Confidence: medium
Summary: Code change looks sound and narrowly scoped, with no actionable implementation findings. Verdict is held at NEEDS_DISCUSSION because CI is currently not green.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Restore suppression except when PublishTrimmed=true |
✅ PASS | 1 file | Evaluated Android properties matched the intended split, but ordinary builds still hide trim analyzer warnings. Not better than PR deletion. |
| 2 | try-fix | Restore suppression only for iOS/MacCatalyst | ✅ PASS | 1 file | Evaluated Android/iOS properties matched the intended platform split, but it retains a stale Apple workaround after upstream resolution. Not better than PR deletion. |
| 3 | try-fix | Set IsAotCompatible=false for ordinary executable builds |
✅ PASS | 1 file | Evaluated Android properties matched the intended ordinary-vs-trimmed split, but expert self-review found a major design risk from changing a broad SDK signal in a transitive MAUI target. |
| PR | PR #33624 | Remove the workaround entirely so MAUI no longer forces EnableTrimAnalyzer=false or SuppressTrimAnalysisWarnings=true. |
1 file | Original PR; code review found no implementation issues but suggested tests/CI validation. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 2 | No | NO NEW IDEAS: PR deletion is cleanest; alternatives either keep hiding analyzer warnings or risk changing broader SDK behavior. |
Exhausted: Yes
Selected Fix: PR's fix — It is the cleanest and least risky option. Candidate 1 and 2 pass focused validation but keep some form of suppression; Candidate 3 passes focused validation but changes IsAotCompatible, which has broader SDK implications and received a major self-review finding.
Report — Final Recommendation
Comparative Report — PR #33624
Candidates compared
| Rank | Candidate | Result | Assessment |
|---|---|---|---|
| 1 | pr |
Best candidate. It removes the stale workaround entirely, restoring normal trim analyzer behavior without adding new build-property coupling. | |
| 2 | pr-plus-reviewer |
Functionally identical to pr; expert reviewer found no actionable feedback to apply and produced no inline findings. |
|
| 3 | try-fix-2 |
✅ PASS | Android-focused behavior is acceptable because Android no longer receives suppression, but it keeps an iOS/MacCatalyst workaround that appears stale after the upstream macios resolution. |
| 4 | try-fix-1 |
✅ PASS | Preserves suppression for ordinary builds and only avoids it for PublishTrimmed=true; this passes focused property validation but continues hiding analyzer warnings in common builds. |
| 5 | try-fix-3 |
✅ PASS | Keeps analyzer suppression removed, but changes IsAotCompatible from a buildTransitive MAUI target. That broad SDK signal has wider effects than the original workaround and was flagged as a major design risk. |
Regression-test ordering
No candidate failed regression tests. The three try-fix candidates passed focused MSBuild property checks. The PR and pr-plus-reviewer did not have detected tests, so their validation status is weaker; however, the requested rule only requires failed candidates to rank lower than passed candidates, and there were no failures.
Winning candidate
Winner: pr
The raw PR fix and pr-plus-reviewer are equivalent because the expert reviewer found no actionable changes. I select pr as the single winner because it is the submitted fix, is the simplest implementation, removes the stale suppression completely, and avoids the new risks introduced by the try-fix alternatives. The author should still consider adding focused MSBuild/build validation for Android trim analyzer behavior because the gate detected no tests.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
### Description of Change This pull request removes a workaround in the `Microsoft.Maui.Controls.Common.targets` file that previously disabled the trim analyzer and suppressed trim analysis warnings under certain build conditions. This change cleans up the build configuration as the related upstream issue has been resolved. Build configuration cleanup: * Removed the conditional property group that disabled the trim analyzer and suppressed trim analysis warnings, as the related Xamarin issue has been addressed. ---------
Description of Change
This pull request removes a workaround in the
Microsoft.Maui.Controls.Common.targetsfile that previously disabled the trim analyzer and suppressed trim analysis warnings under certain build conditions. This change cleans up the build configuration as the related upstream issue has been resolved.Build configuration cleanup: