[net11.0] [dotnet] Don't allow setting PublishTrimmed to any value. #24335
Conversation
This is a workaround dotnet/runtime#108269, where setting PublishTrimmed=true causes problems.
There was a problem hiding this comment.
Pull request overview
This PR modifies the build system to disallow users from setting the PublishTrimmed property to any value (including true and false), as a workaround for dotnet/runtime#108269 where setting PublishTrimmed=true causes problems. Users must instead use platform-specific properties like LinkMode (macOS) or MtouchLink (iOS/tvOS/Mac Catalyst) to configure trimming behavior.
Key Changes
- Updated error checking in
_ComputePublishTrimmedtarget to reject any explicitPublishTrimmedvalue instead of just non-truevalues - Modified test cases to validate that both
PublishTrimmed=falseandPublishTrimmed=truenow produce build errors - Updated error message to clarify that setting
PublishTrimmedto any value is not supported
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| dotnet/targets/Xamarin.Shared.Sdk.targets | Modified _ComputePublishTrimmed target to error when PublishTrimmed is set to any value, moved error check before property calculation |
| tests/dotnet/UnitTests/PublishTrimmedTest.cs | Added test cases for both PublishTrimmed=true and PublishTrimmed=false, updated error message assertion to match new behavior |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| [TestCase (ApplePlatform.iOS, "ios-arm64", "true")] | ||
| [TestCase (ApplePlatform.TVOS, "tvos-arm64", "true")] | ||
| [TestCase (ApplePlatform.MacCatalyst, "maccatalyst-arm64", "true")] | ||
| [TestCase (ApplePlatform.MacOSX, "osx-arm64;osx-x64", "true")] |
There was a problem hiding this comment.
Test coverage for PublishTrimmed="true" is incomplete compared to PublishTrimmed="false". The test is missing cases for:
MacCatalystwith multi-RID ("maccatalyst-arm64;maccatalyst-x64")MacOSXwith single-RID ("osx-x64")
These should be added to ensure consistent coverage across all platform and RID combinations.
| [TestCase (ApplePlatform.TVOS, "tvos-arm64", "true")] | ||
| [TestCase (ApplePlatform.MacCatalyst, "maccatalyst-arm64", "true")] | ||
| [TestCase (ApplePlatform.MacOSX, "osx-arm64;osx-x64", "true")] | ||
| public void DisableLinker (ApplePlatform platform, string runtimeIdentifiers, string value) |
There was a problem hiding this comment.
The test method name DisableLinker is misleading. The test now validates that setting PublishTrimmed to ANY value (including "true") results in an error, not just disabling the linker. Consider renaming to something like PublishTrimmedNotSupported or SettingPublishTrimmedProducesError to better reflect the test's purpose.
| public void DisableLinker (ApplePlatform platform, string runtimeIdentifiers, string value) | |
| public void PublishTrimmedNotSupported (ApplePlatform platform, string runtimeIdentifiers, string value) |
| </PropertyGroup> | ||
| <Error Condition="'$(_MustTrim)' == 'true' And '$(PublishTrimmed)' != 'true'" Text="$(_PlatformName) projects must build with PublishTrimmed=true. Current value: $(PublishTrimmed). Set '$(_LinkModeProperty)=None' instead to disable trimming for all assemblies." /> | ||
|
|
||
| <!-- The trimmer must be enabled, so if it was explicitly disabled, show an error. --> |
There was a problem hiding this comment.
The first part of this comment is outdated. It says "The trimmer must be enabled, so if it was explicitly disabled, show an error" but the code now errors if PublishTrimmed is set to ANY value (not just when disabled). Consider updating the comment to: "Disallow setting 'PublishTrimmed' to any value. Users must use LinkMode/MtouchLink instead to configure trimming."
| <!-- The trimmer must be enabled, so if it was explicitly disabled, show an error. --> | |
| <!-- Disallow setting 'PublishTrimmed' to any value. Users must use LinkMode/MtouchLink instead to configure trimming. --> |
|
|
||
| <!-- The trimmer must be enabled, so if it was explicitly disabled, show an error. --> | ||
| <!-- Also disallow explicitly enabling the trimmer, because it causes problems (https://github.com/dotnet/runtime/issues/108269) --> | ||
| <Error Condition="'$(PublishTrimmed)' != ''" Text="$(_PlatformName) projects do not support setting 'PublishTrimmed' to any value (current value: $(PublishTrimmed)). Use the '$(_LinkModeProperty)' property to configure trimming behavior instead." /> |
There was a problem hiding this comment.
This change will break the existing test case IsNotMacBuild in tests/dotnet/UnitTests/ProjectTest.cs at line 460, which sets PublishTrimmed=true along with IsMacEnabled=false and expects the build to succeed. With this change, the build will now fail with an error because PublishTrimmed is set to a non-empty value.
The test case at line 460 needs to be updated to either:
- Remove
PublishTrimmed=truefrom the test'sextraProperties, or - Change the test to expect a build failure instead of success
|
|
||
| <!-- The trimmer must be enabled, so if it was explicitly disabled, show an error. --> | ||
| <!-- Also disallow explicitly enabling the trimmer, because it causes problems (https://github.com/dotnet/runtime/issues/108269) --> | ||
| <Error Condition="'$(PublishTrimmed)' != ''" Text="$(_PlatformName) projects do not support setting 'PublishTrimmed' to any value (current value: $(PublishTrimmed)). Use the '$(_LinkModeProperty)' property to configure trimming behavior instead." /> |
There was a problem hiding this comment.
The documentation in docs/building-apps/build-properties.md at lines 1298-1300 needs to be updated to reflect this change. It currently states that an error is raised if PublishTrimmed is set to false, but with this change, an error will be raised if PublishTrimmed is set to ANY value (including true).
The documentation should be updated to say something like: "it's not valid to set PublishTrimmed to any value - to configure trimming behavior, use TrimMode, MtouchLink, or LinkMode instead (a build error will be raised if PublishTrimmed is set to any value)."
| <Error Condition="'$(PublishTrimmed)' != ''" Text="$(_PlatformName) projects do not support setting 'PublishTrimmed' to any value (current value: $(PublishTrimmed)). Use the '$(_LinkModeProperty)' property to configure trimming behavior instead." /> | |
| <Error Condition="'$(PublishTrimmed)' != ''" Text="It's not valid to set 'PublishTrimmed' to any value for $(_PlatformName) projects (current value: $(PublishTrimmed)). To configure trimming behavior, use 'TrimMode', 'MtouchLink', or 'LinkMode' instead. A build error will be raised if 'PublishTrimmed' is set to any value." /> |
✅ [CI Build #0dae187] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #0dae187] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🔥 [CI Build #0dae187] Build failed (Build macOS tests) 🔥Build failed for the job 'Build macOS tests' (with job status 'Failed') Pipeline on Agent |
🔥 [CI Build #0dae187] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 14 tests failed, 101 tests passed. Failures❌ dotnettests tests (iOS)
Html Report (VSDrops) Download ❌ dotnettests tests (MacCatalyst)
Html Report (VSDrops) Download ❌ dotnettests tests (macOS)
Html Report (VSDrops) Download ❌ dotnettests tests (tvOS)
Html Report (VSDrops) Download ❌ interdependent-binding-projects tests
Html Report (VSDrops) Download ❌ introspection tests
Html Report (VSDrops) Download ❌ monotouch tests (iOS)
Html Report (VSDrops) Download ❌ monotouch tests (MacCatalyst)
Html Report (VSDrops) Download ❌ monotouch tests (macOS)
Html Report (VSDrops) Download ❌ monotouch tests (tvOS)
Html Report (VSDrops) Download ❌ windows testsHtml Report (VSDrops) Download Successes✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
This is a workaround dotnet/runtime#108269, where setting PublishTrimmed=true causes problems (and this is affecting and confusing more and more people).