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

[msbuild] Add ILStrip'ing for net6 applications. Fixes #11445. #12563

Merged
merged 29 commits into from
Oct 5, 2021
Merged

[msbuild] Add ILStrip'ing for net6 applications. Fixes #11445. #12563

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b4bad89
[msbuild] Add ILStrip'ing for net6 applications
chamons Aug 24, 2021
87c5c96
Apply suggestions from code review
chamons Aug 26, 2021
fb4fef5
Rework EnableAssemblyILStripping conditions
chamons Aug 26, 2021
7b355ed
Merge branch 'main' into il_strip_net6
chamons Aug 26, 2021
0f13eb3
Remove questionable set
chamons Aug 26, 2021
cf78956
Merge branch 'il_strip_net6' of github.com:chamons/xamarin-macios int…
chamons Aug 26, 2021
aea669e
Code review comments
chamons Aug 26, 2021
cce0c87
Update tests/dotnet/UnitTests/PostBuildTest.cs
chamons Aug 27, 2021
80bc6f1
Apply suggestions from code review
chamons Aug 27, 2021
1c30a7f
Extend BuildIpaTest to include strip testing
chamons Aug 27, 2021
8abf282
Fix makedir missing windows tags
chamons Aug 27, 2021
40b0985
Implement XVS redirect
chamons Aug 30, 2021
c96c815
Fix build
chamons Aug 30, 2021
2a07c73
Merge branch 'main' into il_strip_net6
chamons Sep 17, 2021
2d922d5
Make AssertBundleAssembliesStripStatus recursive
chamons Sep 17, 2021
0ac7fbf
Add ILStrip to merge targets and bump to RC2 with fix
chamons Sep 20, 2021
bca5bc3
Merge branch 'main' into il_strip_net6
chamons Sep 20, 2021
de175fd
Add some comments
chamons Sep 20, 2021
7be9298
Add sessionid that started all of this
chamons Sep 20, 2021
1e02818
Actually use correct ILStrip and fix XVS arg
chamons Sep 21, 2021
ffa97b9
Use full name of ILStrip to work around other being loaded first
chamons Sep 21, 2021
1922f7d
Merge branch 'main' into il_strip_net6
rolfbjarne Sep 23, 2021
7d5cdda
Set ShouldCopyToBuildServer on ITaskCallback
chamons Sep 27, 2021
53a7a38
Merge branch 'il_strip_net6' of github.com:chamons/xamarin-macios int…
chamons Sep 27, 2021
7a9e22e
Merge branch 'main' into il_strip_net6
chamons Sep 27, 2021
a027e74
Fix unit test after main merge
chamons Sep 27, 2021
f215ae8
Merge branch 'main' into il_strip_net6
chamons Sep 28, 2021
ca0cab7
Merge branch 'main' into il_strip_net6
rolfbjarne Sep 30, 2021
f5c2845
Merge branch 'main' into il_strip_net6
chamons Oct 4, 2021
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
35 changes: 35 additions & 0 deletions dotnet/targets/Xamarin.Shared.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,8 @@
_ComputeFrameworkFilesToPublish;
_ComputeDynamicLibrariesToPublish;
ComputeFilesToPublish;
_ComputeStripAssemblyIL;
_StripAssemblyIL;
_LoadLinkerOutput;
_CompileNativeExecutable;
_LinkNativeExecutable;
Expand Down Expand Up @@ -594,6 +596,39 @@
</ItemGroup>
</Target>

<Target Name="_ComputeStripAssemblyIL" Condition=" '$(EnableAssemblyILStripping)' == '' " DependsOnTargets="_ComputeVariables;ComputeFilesToPublish">
<PropertyGroup>
<!-- Don't strip IL by default -->
<EnableAssemblyILStripping>false</EnableAssemblyILStripping>

<!-- Strip if we are AOT and Release -->
<EnableAssemblyILStripping Condition="'$(_RunAotCompiler)' == 'true' And '$(Configuration)' == 'Release'">true</EnableAssemblyILStripping>

<!-- Don't strip if we are running the interpreter -->
<EnableAssemblyILStripping Condition="'$(MtouchInterpreter)' != ''">false</EnableAssemblyILStripping>
</PropertyGroup>
</Target>

<Target Name="_StripAssemblyIL" Condition="'$(EnableAssemblyILStripping)' == 'true'" DependsOnTargets="_ComputeStripAssemblyIL">
rolfbjarne marked this conversation as resolved.
Show resolved Hide resolved
<PropertyGroup>
<_StrippedAssemblyDirectory>$(DeviceSpecificIntermediateOutputPath)\stripped</_StrippedAssemblyDirectory>
</PropertyGroup>
<MakeDir Directories="$(_StrippedAssemblyDirectory)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the SessionId parameter and the IsMacEnabled condition, so when building from Windows this will only be executed on Windows. Like this:

xamarin-macios/msbuild/Xamarin.iOS.Tasks.Core/Xamarin.iOS.Common.targets

Line 364 in 74b161a

<MakeDir SessionId="$(BuildSessionId)" Condition="'$(IsMacEnabled)' == 'true'" Directories="$(_AppResourcesPath)" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<ItemGroup>
<_AssembliesToBeStripped Include="@(ResolvedFileToPublish)" Condition="'%(Extension)' == '.dll'">
<OutputPath>$(_StrippedAssemblyDirectory)\%(Filename)%(Extension)</OutputPath>
</_AssembliesToBeStripped>
<_StrippedAssemblies Include="@(_AssembliesToBeStripped->'%(OutputPath)')" >
</_StrippedAssemblies>
</ItemGroup>
<ILStrip Assemblies="@(_AssembliesToBeStripped)" />
Copy link
Contributor

@emaf emaf Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When building from Windows this won't be executed remotely because it's missing the SessionId property. Does this need to be executed on a Mac? Do we own this task?

Copy link
Member

Choose a reason for hiding this comment

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

The runtime team owns it: https://github.com/dotnet/runtime/pull/57359/files

And yes, this has to be executed on the Mac (it happens after the AOT compiler has executed, but before we sign the final .app)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this won't work then, for executing a task remotely we need to add the SessionId property and the logic to execute it remotely based on that value. We had the same problem with ILLink, and we ended up inheriting from the original task to add support for remoting it, see https://github.com/xamarin/xamarin-macios/blob/main/msbuild/Xamarin.iOS.Tasks/Tasks/ILLink.cs and https://github.com/xamarin/xamarin-macios/blob/main/msbuild/Xamarin.iOS.Tasks.Core/Tasks/ILLinkBase.cs. The base class is adding input and output properties for all the files that need to be copied to the Mac and the output ones that need to be created on Windows. I'm not sure if there's a better way for doing this.

Copy link
Member

Choose a reason for hiding this comment

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

@emaf I think @chamons can try to implement this but can you help us testing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking a stab at this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dalexsoto I'll be out next week, but @mauroa should be able to help with any questions. Please keep in mind he has many things in his plate already, but he'll try to test this from Windows once the changes are ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fixed.

<ItemGroup>
<ResolvedFileToPublish Remove="@(_AssembliesToBeStripped)" />
<ResolvedFileToPublish Include="@(_StrippedAssemblies)" />
</ItemGroup>
</Target>


<Target Name="_LoadLinkerOutput" DependsOnTargets="ComputeFilesToPublish">
<!-- Load _MainFile -->
<ReadItemsFromFile SessionId="$(BuildSessionId)" File="$(_LinkerItemsDirectory)/_MainFile.items" Condition="Exists('$(_LinkerItemsDirectory)/_MainFile.items')">
Expand Down
7 changes: 7 additions & 0 deletions msbuild/Xamarin.MacDev.Tasks.Core/Tasks/ParseBundlerArgumentsTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ public abstract class ParseBundlerArgumentsTaskBase : XamarinTask {
[Output]
public string Registrar { get; set; }

[Output]
public string NoStrip { get; set; }

[Output]
public int Verbosity { get; set; }

Expand Down Expand Up @@ -130,6 +133,10 @@ public override bool Execute ()
xml = new List<string> ();
xml.Add (value);
break;
case "nostrip":
// Output is EnableAssemblyILStripping so we enable if --nostring=false and disable if true
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: nostring -> nostrip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

NoStrip = ParseBool (value) ? "false" : "true";
break;
default:
Log.LogMessage (MessageImportance.Low, "Skipping unknown argument '{0}' with value '{1}'", name, value);
break;
Expand Down
1 change: 1 addition & 0 deletions msbuild/Xamarin.Shared/Xamarin.Shared.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,7 @@ Copyright (C) 2018 Microsoft. All rights reserved.
<Output TaskParameter="Registrar" PropertyName="_BundlerRegistrar" />
<Output TaskParameter="Verbosity" PropertyName="_BundlerVerbosity" />
<Output TaskParameter="XmlDefinitions" ItemName="_BundlerXmlDefinitions" />
<Output TaskParameter="NoStrip" PropertyName="EnableAssemblyILStripping" />
Copy link
Member

Choose a reason for hiding this comment

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

This task should take EnableAssemblyILStripping as input as well, otherwise it won't be possible for users to override the default by setting EnableAssemblyILStripping to something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is correct.

I hacked up a test project: https://gist.github.com/chamons/abe82c128d909df53a1780c792394dab to check

In it, I invoke ParseBundlerArguments with some _BundlerArguments and EnableAssemblyILStripping preset. I print before and after, and I don't think it overrides (with 0f13eb3).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on ^?

</ParseBundlerArguments>

<PropertyGroup>
Expand Down
6 changes: 5 additions & 1 deletion tests/dotnet/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ build-dotnet: $(TARGETS)
$(DOTNET6) build size-comparison/MySingleView/dotnet/MySingleView.csproj --runtime ios-arm64 $(COMMON_ARGS) /bl:$@.binlog $(MSBUILD_VERBOSITY)

run-dotnet: $(TARGETS)
$(DOTNET6) build -t:Run size-comparison/MySingleView/dotnet/MySingleView.csproj --runtime ios-arm64 $(COMMON_ARGS)
$(DOTNET6) build -t:Run size-comparison/MySingleView/dotnet/MySingleView.csproj --runtime ios-arm64 $(COMMON_ARGS) /bl:$@.binlog $(MSBUILD_VERBOSITY)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convince mostly. Writing out log is almost free, and being able to test sim was nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Target strip-dotnet: can also be removed now :)

and the README updated to document how to preserve the IL for analysis (it's already done for legacy)


run-dotnet-sim: $(TARGETS)
$(DOTNET6) build -t:Run size-comparison/MySingleView/dotnet/MySingleView.csproj /p:Configuration=Release --runtime iossimulator-x64 /p:Platform=iPhoneSimulator /bl:$@.binlog


# this will break the signature, so app won't run anymore. Use it only to compare final size w/legacy
# https://github.com/xamarin/xamarin-macios/issues/11445
Expand Down
34 changes: 34 additions & 0 deletions tests/dotnet/UnitTests/PostBuildTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

using Microsoft.Build.Framework;
using Microsoft.Build.Logging.StructuredLogger;
using Mono.Cecil;

namespace Xamarin.Tests {
[TestFixture]
Expand Down Expand Up @@ -68,6 +69,39 @@ public void BuildIpaTest (ApplePlatform platform, string runtimeIdentifiers)
Assert.That (pkgPath, Does.Exist, "pkg creation");
}

[Test]
[TestCase (ApplePlatform.iOS, "ios-arm64", true)]
[TestCase (ApplePlatform.iOS, "ios-arm64", false)]
public void AssemblyStripping (ApplePlatform platform, string runtimeIdentifiers, bool shouldStrip)
{
var project = "MySimpleApp";
Configuration.IgnoreIfIgnoredPlatform (platform);

var project_path = GetProjectPath (project, runtimeIdentifiers: runtimeIdentifiers, platform: platform, out var appPath);
Clean (project_path);
var properties = GetDefaultProperties (runtimeIdentifiers);

// Force EnableAssemblyILStripping since we are building debug which never will by default
properties ["EnableAssemblyILStripping"] = shouldStrip ? "true" : "false";

DotNet.AssertBuild (project_path, properties);

var assemblies = Directory.GetFiles (appPath, "*.dll");
var assembliesWithOnlyEmptyMethods = new List<String>();
chamons marked this conversation as resolved.
Show resolved Hide resolved
foreach (var assembly in assemblies) {
ModuleDefinition definition = ModuleDefinition.ReadModule(assembly, new ReaderParameters { ReadingMode = ReadingMode.Deferred });
chamons marked this conversation as resolved.
Show resolved Hide resolved

bool onlyHasEmptyMethods = definition.Assembly.MainModule.Types.All(t =>
chamons marked this conversation as resolved.
Show resolved Hide resolved
t.Methods.Where(m => m.HasBody).All(m => m.Body.Instructions.Count == 1));
chamons marked this conversation as resolved.
Show resolved Hide resolved
if (onlyHasEmptyMethods) {
assembliesWithOnlyEmptyMethods.Add (assembly);
}
}

// Some assemblies, such as Facades, will be completely empty even when not stripped
Assert.That(assemblies.Length == assembliesWithOnlyEmptyMethods.Count, Is.EqualTo(shouldStrip), $"Unexpected stripping status: of {assemblies.Length} assemblies {assembliesWithOnlyEmptyMethods.Count} were empty.");
chamons marked this conversation as resolved.
Show resolved Hide resolved
}

rolfbjarne marked this conversation as resolved.
Show resolved Hide resolved
[Test]
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-arm64")]
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-arm64;maccatalyst-x64")]
Expand Down