Skip to content

Commit

Permalink
[release/6.0.1xx-preview10] [net6] Fix ILStrip'ed apps to actually wo…
Browse files Browse the repository at this point in the history
…rk again (#13106)

* [net6] Fix ILStrip'ed apps to actually work again

- In a late minute change to the ILStrip PR (#12563) a change to support XVS support broke execution of Apps that were stripped
- Applications were broken because none of the stripped assemblies were actually copied into the bundle
- However, the tests still passed, because all assemblies that were there had no IL (zero assemblies total)

Now why did this happen?
- The stripped assemblies were changed to return via an msbuild Output Element
- Output Element can return an Property or ItemGroup, depending if you use the PropertyName or ItemName attributes
- Unfortunately I used PropertyName, when I expected an ItemGroup. So I silently had a property created instead.
- Thus zero items were added to the list of files to copy into the bundle
- Which was undetected as the test did not confirm files were copied in, and manual tests were not run so late into the PR (3 weeks after PR was opened)

How was it fixed?
- Correctly using ItemName on Output created a valid item group to reference
- However, that still failed with an absurdly confusing error:

 PATH/Microsoft.NET.Publish.targets(277,5): error MSB3024: Could not copy the file FILE to the destination file PATH, because the destination is a folder instead of a file. To copy the source file into a folder, consider using the DestinationFolder parameter instead of DestinationFiles.

- After a splunking through netcore targets, I found the metadata on these assemblies references really matters. Without it, they are not processed correctly at all.
- Thus, I updated ILStripBase to clone the existing metadata when changing the original assembly reference to the stripped path
- Finally, I corrected the test to assert that required files are copied in. I also manually ran our device test.

* Update tests/dotnet/UnitTests/PostBuildTest.cs

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

Co-authored-by: Chris Hamons <chris.hamons@xamarin.com>
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
  • Loading branch information
3 people authored Oct 26, 2021
1 parent 5d53aa6 commit 48f57c6
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 2 deletions.
2 changes: 1 addition & 1 deletion dotnet/targets/Xamarin.Shared.Sdk.targets
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@
</_AssembliesToBeStripped>
</ItemGroup>
<Xamarin.MacDev.Tasks.ILStrip Assemblies="@(_AssembliesToBeStripped)" SessionId="$(BuildSessionId)">
<Output TaskParameter="StrippedAssemblies" PropertyName="_StrippedAssemblies" />
<Output TaskParameter="StrippedAssemblies" ItemName="_StrippedAssemblies" />
</Xamarin.MacDev.Tasks.ILStrip>
<ItemGroup>
<ResolvedFileToPublish Remove="@(_AssembliesToBeStripped)" />
Expand Down
2 changes: 1 addition & 1 deletion msbuild/Xamarin.iOS.Tasks.Core/Tasks/ILStripBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public override bool Execute ()
{
foreach (var item in Assemblies)
{
stripedItems.Add (new TaskItem (item.GetMetadata("OutputPath")));
stripedItems.Add (new TaskItem (item.GetMetadata("OutputPath"), item.CloneCustomMetadata ()));
}
}

Expand Down
2 changes: 2 additions & 0 deletions tests/dotnet/UnitTests/PostBuildTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ public void AssemblyStripping (ApplePlatform platform, string runtimeIdentifiers
DotNet.AssertBuild (project_path, properties);

AssertBundleAssembliesStripStatus (appPath, shouldStrip);
Assert.That (Path.Combine (appPath, "MySimpleApp.dll"), Does.Exist, "Application Assembly");
Assert.That (Path.Combine (appPath, "Xamarin.iOS.dll"), Does.Exist, "Platform Assembly");
}

[Test]
Expand Down

5 comments on commit 48f57c6

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

❌ [CI Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Packages generated

View packages

Test results

1 tests failed, 217 tests passed.

Failed tests

  • dont link/Mac Catalyst [dotnet]/Release [dotnet]: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)

Pipeline on Agent XAMBOT-1036.BigSur'
[release/6.0.1xx-preview10] [net6] Fix ILStrip'ed apps to actually work again (#13106)

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Tests were not ran (VSTS: device tests tvOS). ⚠️

Results were skipped for this run due to provisioning problems Azure Devops. Please contact the bot administrator.

Pipeline on Agent
[release/6.0.1xx-preview10] [net6] Fix ILStrip'ed apps to actually work again (#13106)

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Tests were not ran (VSTS: device tests iOS). ⚠️

Results were skipped for this run due to provisioning problems Azure Devops. Please contact the bot administrator.

Pipeline on Agent
[release/6.0.1xx-preview10] [net6] Fix ILStrip'ed apps to actually work again (#13106)

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅ Tests passed on macOS M1 - Mac Big Sur (11.5) ✅

Tests passed

All tests on macOS X M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
[release/6.0.1xx-preview10] [net6] Fix ILStrip'ed apps to actually work again (#13106)

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅ Tests passed on macOS Mac Mojave (10.14) ✅

Tests passed

All tests on macOS X Mac Mojave (10.14) passed.

Pipeline on Agent
[release/6.0.1xx-preview10] [net6] Fix ILStrip'ed apps to actually work again (#13106)

Please sign in to comment.