Skip to content

[net6] Fix ILStrip'ed apps to actually work again - #13098

Merged
chamons merged 2 commits into
dotnet:mainfrom
chamons:ilstrip_fix
Oct 25, 2021
Merged

[net6] Fix ILStrip'ed apps to actually work again#13098
chamons merged 2 commits into
dotnet:mainfrom
chamons:ilstrip_fix

Conversation

@chamons

@chamons chamons commented Oct 22, 2021

Copy link
Copy Markdown
Contributor

Fixes: #13094

  • In a late minute change to the ILStrip PR ([msbuild] Add ILStrip'ing for net6 applications. Fixes #11445. #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.

- In a late minute change to the ILStrip PR (dotnet#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.
@chamons chamons added the not-notes-worthy Ignore for release notes label Oct 22, 2021
@chamons
chamons requested review from emaf and mauroa as code owners October 22, 2021 22:20
@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

❌ [PR 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

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 131 tests passed.

Failed tests

  • monotouch-test/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Tests run: 2703 Passed: 2500 Inconclusive: 35 Failed: 3 Ignored: 200)

Pipeline on Agent XAMBOT-1100.BigSur'
Merge c47f4bd into 560d777

Comment thread tests/dotnet/UnitTests/PostBuildTest.cs Outdated
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@chamons

chamons commented Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Test failure was good old https://github.com/xamarin/maccore/issues/2443

@chamons
chamons merged commit ab5c0c6 into dotnet:main Oct 25, 2021
@chamons
chamons deleted the ilstrip_fix branch October 25, 2021 16:31
@rolfbjarne

Copy link
Copy Markdown
Member

/sudo backport release/6.0.1xx-preview10

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

Backport Job to branch release/6.0.1xx-preview10 Created! The magic is happening here

@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5367047 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

not-notes-worthy Ignore for release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

net6 test app crashses in Release Device with ILStripping enabled

8 participants