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.

Sign up for GitHub

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

Fix non-NativeAOT builds with PublishAot=true in the project file #18727

Merged
merged 3 commits into from
Aug 16, 2023
Merged

Fix non-NativeAOT builds with PublishAot=true in the project file #18727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cdd6d52
Fix non-NativeAOT builds with PublishAot=true in the project file
filipnavara Aug 14, 2023
8d8ab6f
Add test case
rolfbjarne Aug 15, 2023
84a0aa6
Add workaround for PublishAotUsingRuntimePack issue in .NET SDK
filipnavara Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions dotnet/targets/Xamarin.Shared.Sdk.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@
</PropertyGroup>

<!-- Various options when using NativeAOT -->
<PropertyGroup Condition="'$(PublishAot)' == 'true'">
<!-- This turns off some NativeAOT logic we don't want nor need -->
<NativeCompilationDuringPublish>false</NativeCompilationDuringPublish>
</PropertyGroup>
<PropertyGroup Condition="'$(PublishAot)' == 'true' And '$(_IsPublishing)' == 'true'">
<!-- Disable our own assembly IL stripping logic, because ILC does that already -->
<EnableAssemblyILStripping>false</EnableAssemblyILStripping>
Expand All @@ -151,8 +155,5 @@

<!-- We must find the BCL libraries using the runtime pack instead of using the built-in NativeAOT BCL -->
<PublishAotUsingRuntimePack>true</PublishAotUsingRuntimePack>

<!-- This turns off some NativeAOT logic we don't want nor need -->
<NativeCompilationDuringPublish>false</NativeCompilationDuringPublish>
</PropertyGroup>
</Project>
29 changes: 26 additions & 3 deletions dotnet/targets/Xamarin.Shared.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,13 +464,36 @@
<IlcCompileDependsOn>Compile;_ComputeLinkerArguments;_ComputeManagedAssemblyToLink;SetupOSSpecificProps;PrepareForILLink;_XamarinComputeIlcCompileInputs</IlcCompileDependsOn>
</PropertyGroup>

<!--
Workaround for SDK issue where setting PublishAot=true during build cannot resolve correct
runtime pack and IL compiler pack.

Setting PublishAotUsingRuntimePack=true unconditionally results in the runtime pack label to
be updated to "NativeAOT" instead of "Mono" and trying to build against NativeAOT runtime
pack instead of the Mono one.

Setting PublishAotUsingRuntimePack=false causes the NuGet resolution to check for known
ILCompiler packs and these don't exist for iOS-like platforms.

The workaround ensure that PublishAotUsingRuntimePack is false when SDK updates the runtime
pack labels, and thus the "Mono" label is used. We then set it to true right before
ProcessFrameworkReferences target to skip downloading the target ILCompiler pack.
-->
<Target Name="_WorkaroundAotRuntimePackResolution" BeforeTargets="ProcessFrameworkReferences" Condition="'$(PublishAot)' == 'true' And '$(_UseNativeAot)' != 'true'">
<PropertyGroup>
<PublishAotUsingRuntimePack>true</PublishAotUsingRuntimePack>
</PropertyGroup>
</Target>

Comment on lines +467 to +487
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct fix here: dotnet/sdk#34640

If it's accepted and ingested then we should remove this workaround and unconditionally set <PublishAotUsingRuntimePack>true</PublishAotUsingRuntimePack> when PublishAot=true in dotnet/targets/Xamarin.Shared.Sdk.props.

Copy link
Member

Choose a reason for hiding this comment

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

Filed to #18741 to not forget to remove the workaround.

<Target Name="_ComputeLinkerFeatures">
<PropertyGroup Condition="'$(PublishAot)' == 'true'">
<!-- Yep, we want to run ILLink as well, because we need our custom steps to run (NativeAOT sets this to false, so set it back to true) -->
<RunILLink>true</RunILLink>
</PropertyGroup>

<PropertyGroup Condition="'$(_UseNativeAot)' == 'true'">
<!-- The one and only registrar is 'managed-static' when using NativeAOT -->
<Registrar Condition="'$(Registrar)' == ''">managed-static</Registrar>

<!-- Yep, we want to run ILLink as well, because we need our custom steps to run (NativeAOT sets this to false, so set it back to true) -->
<RunILLink>true</RunILLink>
</PropertyGroup>

<PropertyGroup>
Expand Down
17 changes: 17 additions & 0 deletions tests/dotnet/UnitTests/ProjectTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1354,6 +1354,23 @@ public void BuildAndExecuteAppWithWinExeOutputType (ApplePlatform platform, stri
Assert.AreEqual ($"WinExe is not a valid output type for macOS", errors [0].Message, "Error message");
}

[Test]
[TestCase (ApplePlatform.iOS, "iossimulator-x64")]
[TestCase (ApplePlatform.MacOSX, "osx-arm64")]
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-x64")]
public void PublishAotDuringBuild (ApplePlatform platform, string runtimeIdentifiers)
{
var project = "MySimpleApp";
Configuration.IgnoreIfIgnoredPlatform (platform);
Configuration.AssertRuntimeIdentifiersAvailable (platform, runtimeIdentifiers);

var project_path = GetProjectPath (project, runtimeIdentifiers: runtimeIdentifiers, platform: platform, out var appPath);
Clean (project_path);
var properties = GetDefaultProperties (runtimeIdentifiers);
properties ["PublishAot"] = "true";
DotNet.AssertBuild (project_path, properties);
}

void AssertThatDylibExistsAndIsReidentified (string appPath, string dylibRelPath)
{
var dylibPath = Path.Join (appPath, "Contents", "MonoBundle", dylibRelPath);
Expand Down