Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[macOS] Cannot use PublishTrimmed=true #19142

Closed
tipa opened this issue Oct 9, 2023 · 3 comments · Fixed by #19183
Closed

[macOS] Cannot use PublishTrimmed=true #19142

tipa opened this issue Oct 9, 2023 · 3 comments · Fixed by #19183
Labels
bug If an issue is a bug or a pull request a bug fix regression The issue or pull request is a regression
Milestone

Comments

@tipa
Copy link

tipa commented Oct 9, 2023

Steps to Reproduce

  1. dotnet new macos
  2. Add <PublishTrimmed>true</PublishTrimmed> to test.csproj
  3. dotnet publish

Expected Behavior

App builds and gets trimmed without errors

Actual Behavior

/usr/local/share/dotnet/sdk/8.0.100-rc.1.23463.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(212,5): error NETSDK1191: A runtime identifier for the property 'SelfContained' couldn't be inferred. Specify a rid explicitly. [/Users/me/GitHub/test/test.csproj]

Environment

macos 13.3.8825-net8-rc1/8.0.100-rc.1 SDK 8.0.100-rc.1

Example Project (If Possible)

test.zip

Comments

Until today, I was under the impression, that using TrimMode=full did trim out every unused code from my application. However, I was surprised that my app did not throw any error when using System.Json.Text in .NET8 without source generators. I expected it to throw an System.InvalidOperationException as documented (and I also saw that exception in my .NET8 Android project).
As a side-note / additional question: I also added PublishTrimmed=true to my iOS app, built and uploaded it to TestFlight. It also does not throw the mentioned/expected System.InvalidOperationException. Does the PublishTrimmed property even have any effect in iOS/macOS projects?

rolfbjarne added a commit to rolfbjarne/sdk that referenced this issue Oct 12, 2023
…meIdentifier.

This is identical to issue dotnet#33414 (allow PublishAot=true without RuntimeIdentifier), except for the SelfContained property instead of the PublishAot property.

This adds an escape hatch for a sanity check when trying to publish with SelfContained=true, but without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check if the developer sets SelfContained=true, and that fails the build.

Note that SelfContained defaults to true if PublishTrimmed=true, which means that just setting PublishTrimmed=true also triggers the sanity check.

Ref: xamarin/xamarin-macios#19142
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Oct 12, 2023
We couldn't do universal builds using NativeAOT, because .NET had a sanity
check that was a bit too eager and caused unnecessary build failures. We've
now been able to add an escape hatch to that sanity check, so let's use it.

This makes universal builds using NativeAOT work, so we can also enable/add
corresponding test variations in xharness.

Also ask the runtime to allow SelfContained without a RuntimeIdentifier (the
runtime has an overeager sanity check that doesn't apply to us, so we must
skip it). Fixes xamarin#19142.

Fixes xamarin#19142.
@rolfbjarne rolfbjarne added bug If an issue is a bug or a pull request a bug fix regression The issue or pull request is a regression labels Oct 12, 2023
@rolfbjarne rolfbjarne added this to the .NET 8 milestone Oct 12, 2023
@rolfbjarne
Copy link
Member

I can reproduce this, and a fix is in progress. Hopefully we'll be able to get it in the initial .NET 8 release.

Note that PublishTrimmed=true is redundant on macOS projects, because that's the default, so you can safely remove it.

However, I was surprised that my app did not throw any error when using System.Json.Text in .NET8 without source generators. I expected it to throw an System.InvalidOperationException as documented (and I also saw that exception in my .NET8 Android project).

This is because we've changed the default for JsonSerializerIsReflectionEnabledByDefault to true, to avoid breaking people.

If you do this in your project:

<PropertyGroup>
    <JsonSerializerIsReflectionEnabledByDefault>false</JsonSerializerIsReflectionEnabledByDefault>
</PropertyGroup>

you should get the InvalidOperationException.

@tipa
Copy link
Author

tipa commented Oct 12, 2023

Thanks. I always though that settings TrimMode=full would also set PublishTrimmed=true implicitly on iOS as well. Is this not the case? If not, what is the difference and does setting it to true explicitly result in any additional size reductions?
On .NET Android, PublishTrimmed is set to true automatically in Release mode under certain conditions (and that causes JsonSerializerIsReflectionEnabledByDefault to be set to false.): https://github.com/xamarin/xamarin-android/blob/58a81eb148acb2d2f5d3df91629bba3a3129e773/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets#L71-L72

Also, does settings JsonSerializerIsReflectionEnabledByDefault to false result in any app size reduction?

Tbh, I would prefer if the default JsonSerializerIsReflectionEnabledByDefault=false from would not be overwritten. It still can be overwritten in the MAUI SDK if it's needed there. It gives a nice error message that should help everyone who runs into it: System.InvalidOperationException: Reflection-based serialization has been disabled for this application. Either use the source generator APIs or explicitly configure the 'JsonSerializerOptions.TypeInfoResolver' property.
It is a bit confusing when there are different defaults for each build property on each platform :)

@rolfbjarne
Copy link
Member

We always set JsonSerializerIsReflectionEnabledByDefault=true:

https://github.com/xamarin/xamarin-macios/blob/1a05b370ef3905cc06c8c385fdaa2e920afd89e5/dotnet/targets/Xamarin.Shared.Sdk.props#L96C1-L97

Also, does settings JsonSerializerIsReflectionEnabledByDefault to false result in any app size reduction?

No idea, my guess is that it could go either way, so testing it on your app is the only way to know for sure.

Thanks. I always though that settings TrimMode=full would also set PublishTrimmed=true implicitly on iOS as well.

Note quite: we've hijacked the linker for our own purposes (in addition to what the linker usually does), and that means it's always executed (in fact we set PublishTrimmed=true, and if you try to tell us otherwise by setting PublishTrimmed=false yourself, you'll get a build error).

Then you can adjust exactly what's linked by setting the TrimMode property, or the old-style MtouchLink property (TrimMode takes precedence if you set both). You can even set MtouchLink=None, and the linker will still run, but not actually trim any assemblies (it'll only do what we hijacked it to do).

does setting it to true explicitly result in any additional size reductions?

Setting PublishTrimmed=true doesn't change anything.

Setting TrimMode=full will change the output, and likely save some space.

github-actions bot pushed a commit to dotnet/sdk that referenced this issue Oct 12, 2023
…meIdentifier.

This is identical to issue #33414 (allow PublishAot=true without RuntimeIdentifier), except for the SelfContained property instead of the PublishAot property.

This adds an escape hatch for a sanity check when trying to publish with SelfContained=true, but without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check if the developer sets SelfContained=true, and that fails the build.

Note that SelfContained defaults to true if PublishTrimmed=true, which means that just setting PublishTrimmed=true also triggers the sanity check.

Ref: xamarin/xamarin-macios#19142
rolfbjarne added a commit that referenced this issue Oct 16, 2023
…19183)

We couldn't do universal builds using NativeAOT, because .NET had a sanity
check that was a bit too eager and caused unnecessary build failures. We've
now been able to add an escape hatch to that sanity check, so let's use it.

This makes universal builds using NativeAOT work, so we can also enable/add
corresponding test variations in xharness.

Also ask the runtime to allow SelfContained without a RuntimeIdentifier (the
runtime has an overeager sanity check that doesn't apply to us, so we must
skip it). Fixes #19142.

Fixes #19142.
Fixes #19206.
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Oct 16, 2023
We couldn't do universal builds using NativeAOT, because .NET had a sanity
check that was a bit too eager and caused unnecessary build failures. We've
now been able to add an escape hatch to that sanity check, so let's use it.

This makes universal builds using NativeAOT work, so we can also enable/add
corresponding test variations in xharness.

Also ask the runtime to allow SelfContained without a RuntimeIdentifier (the
runtime has an overeager sanity check that doesn't apply to us, so we must
skip it). Fixes xamarin#19142.

Fixes xamarin#19142.
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Oct 16, 2023
We couldn't do universal builds using NativeAOT, because .NET had a sanity
check that was a bit too eager and caused unnecessary build failures. We've
now been able to add an escape hatch to that sanity check, so let's use it.

This makes universal builds using NativeAOT work, so we can also enable/add
corresponding test variations in xharness.

Also ask the runtime to allow SelfContained without a RuntimeIdentifier (the
runtime has an overeager sanity check that doesn't apply to us, so we must
skip it). Fixes xamarin#19142.

Fixes xamarin#19142.
rolfbjarne pushed a commit that referenced this issue Oct 16, 2023
… using NativeAOT. (#19214)

We couldn't do universal builds using NativeAOT, because .NET had a sanity
check that was a bit too eager and caused unnecessary build failures. We've
now been able to add an escape hatch to that sanity check, so let's use it.

This makes universal builds using NativeAOT work, so we can also enable/add
corresponding test variations in xharness.

Also ask the runtime to allow SelfContained without a RuntimeIdentifier (the
runtime has an overeager sanity check that doesn't apply to us, so we must
skip it). Fixes #19142.

Fixes #19142.
Fixes #19206.

Backport of #19183
rolfbjarne pushed a commit that referenced this issue Oct 16, 2023
…s when using NativeAOT. (#19213)

We couldn't do universal builds using NativeAOT, because .NET had a sanity
check that was a bit too eager and caused unnecessary build failures. We've
now been able to add an escape hatch to that sanity check, so let's use it.

This makes universal builds using NativeAOT work, so we can also enable/add
corresponding test variations in xharness.

Also ask the runtime to allow SelfContained without a RuntimeIdentifier (the
runtime has an overeager sanity check that doesn't apply to us, so we must
skip it). Fixes #19142.

Fixes #19142.
Fixes #19206.

Backport of #19183
github-actions bot pushed a commit to dotnet/sdk that referenced this issue Dec 6, 2023
…meIdentifier.

This is identical to issue #33414 (allow PublishAot=true without RuntimeIdentifier), except for the SelfContained property instead of the PublishAot property.

This adds an escape hatch for a sanity check when trying to publish with SelfContained=true, but without a RuntimeIdentifier, because the check is not valid when building universal apps for macOS and Mac Catalyst (the netX-macos and netX-maccatalyst target frameworks).

When building such an app, the project file will set RuntimeIdentifiers (plural):

    <TargetFramework>net8.0-macos</TargetFramework>
    <RuntimeIdentifiers>osx-x64;osx-arm</RuntimeIdentifiers>

and then during the build, the macOS SDK will run two inner builds, with RuntimeIdentifiers unset, and RuntimeIdentifier set to each of the rids (and at the end merge the result into a single app).

The problem is that the outer build, where RuntimeIdentifiers is set, but RuntimeIdentifier isn't, triggers the sanity check if the developer sets SelfContained=true, and that fails the build.

Note that SelfContained defaults to true if PublishTrimmed=true, which means that just setting PublishTrimmed=true also triggers the sanity check.

Ref: xamarin/xamarin-macios#19142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix regression The issue or pull request is a regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants