[illink][ios] Explicitly setting `PublishTrimmed=true` changes the default feature switch configuration

ivanpovazan commented 2 months ago

Description

As called out in: https://github.com/dotnet/runtime/issues/108108#issuecomment-2375220090 iOS apps built with PublishTrimmed=true set on a project level end up with different set of feature switches configured during the build. For example consider the difference in the input to the trimmer with and without setting the property value explicitly:

--feature Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability false->true
--feature System.ComponentModel.DefaultValueAttribute.IsSupported ->false
--feature System.ComponentModel.Design.IDesignerHost.IsSupported ->false
--feature System.Resources.UseSystemResourceKeys true->false
--feature System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported false->true
--feature System.Runtime.InteropServices.Marshalling.EnableGeneratedComInterfaceComImportInterop ->false

Problem

The problem comes from the order of importing MSBuild targets and property evaluation:

if it is not set on the project level, we will use feature switch configuration as set in the Xamarin SDK early in Xamarin.Shared.Sdk.targets: https://github.com/xamarin/xamarin-macios/blob/ec50934897ef1b98c1cd73ca780cdf5a3750faaa/dotnet/targets/Xamarin.Shared.Sdk.targets#L121-L158 The SDK later sets: PublishTrimmed to true so that we run the linker in: https://github.com/xamarin/xamarin-macios/blob/ec50934897ef1b98c1cd73ca780cdf5a3750faaa/dotnet/targets/Xamarin.Shared.Sdk.targets#L298
if it is set on project level, Microsoft.NET.ILLink.targets will set its own feature switch configuration in: https://github.com/dotnet/runtime/blob/a16e0d58e92fe5f3bcda9e75e9afd1ee0f766967/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L34-L61 as it is imported before Xamarin.Shared.Sdk.targets

This is specifically problematic in case of for example: DynamicCodeSupport, which should not be true on iOS platforms if interpreter is not enabled.

Proposal

We should investigate setting the feature switch defaults (and possibly other trimmer setting) in an MSBuild target so that mobile SDKs can provide different defaults earlier in the build.

ivanpovazan commented 2 months ago

/cc: @vitek-karas @rolfbjarne

vitek-karas commented 2 months ago

@sbomer, @agocke as fyi

This is probably a bit larger discussion as it's effectively about feature switch MSBuild property behavior. I vaguely remember that we wanted the defaults to be set early on, so that other parts of the build can make decisions based on them, but I don't remember the specific use cases.

dotnet-policy-service[bot] commented 2 months ago

Tagging subscribers to 'os-ios': @vitek-karas, @kotlarmilos, @ivanpovazan, @steveisok, @akoeplinger See info in area-owners.md if you want to be subscribed.

sbomer commented 2 months ago

The defaults need to be set before the RuntimeHostConfigurationOption section here: https://github.com/dotnet/sdk/blob/c3a8f72c3a5491c693ff8e49e7406136a12c3040/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L514-L770.

If the defaults move to an MSBuild target, this section needs to move to a target as well. As an alternative, could the Xamarin defaults be set in an earlier import?

ivanpovazan commented 2 months ago

As an alternative, could the Xamarin defaults be set in an earlier import?

Xamarin plugs its platform targets - Microsoft.iOS.Sdk.targets, that in turn import Xamarin.Shared.Sdk.targets which set feature switch configuration, through AfterMicrosoftNETSdkTargets property which gets imported last in: https://github.com/dotnet/sdk/blob/afab26398e223c8de604acacca346e1e4306de0f/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1480 or in other words, after ILLink targets have already been imported: https://github.com/dotnet/sdk/blob/afab26398e223c8de604acacca346e1e4306de0f/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1439

I think that changing the order of imports here is too risky. However, what might be possible is to relocate only the feature switch configuration from Xamarin.Shared.Sdk.targets into Xamarin.Shared.Sdk.props @rolfbjarne what do you think?

Lastly, even if we would be able to implement the mentioned workaround it wont/shouldn't be a long-term solution as other sdks (Android?) might have the same problem.

MichalStrehovsky commented 2 months ago

Did we consider moving the defaults to Microsoft.NET.Sdk.targets? Instead of ILLink/ILCompiler targets setting these, we could delay giving them a default value until right before the value is needed (i.e. we could set it right before Defaults @(RuntimeHostConfigurationOption) items based on MSBuild properties. in the file).

dotnet / runtime