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

Jump to bottom

Set TrimMode partial by default #7132

Merged
merged 7 commits into from
Jul 15, 2022
Merged

Set TrimMode partial by default #7132

merged 7 commits into from
Jul 15, 2022

Conversation

agocke
Copy link
Member

@agocke agocke commented Jun 28, 2022

Companion to dotnet/linker#2856

@agocke @jonathanpeppers
Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/…
860f12f 
…Microsoft.Android.Sdk.DefaultProperties.targets

Co-authored-by: Jonathan Peppers <jonathan.peppers@gmail.com>
@agocke
Copy link
Member Author

agocke commented Jun 28, 2022

Note, this trimmode will only be understood once the linker change is merged, so we'll have to wait for that.

@agocke
Copy link
Member Author

agocke commented Jun 28, 2022

Worryingly, this passed, so I suspect we're not actually running any trim tests, or that I missed another setting of TrimMode -- I'll double check that.

@agocke
Copy link
Member Author

agocke commented Jun 28, 2022

Ah, I see no tests are run here.

@jonathanpeppers
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jun 28, 2022 •
edited
Loading

So when the change in dotnet/linker#2856 flows all the way to dotnet/installer, we can try this.

Our Maestro cadence right now is weekly, but I can do it faster if that change lands. Let me know.

@agocke
Copy link
Member Author

agocke commented Jun 28, 2022

Yeah, once this goes in I think we'll want to flow through everywhere ASAP to flush out problems. Thanks for your help.

@jonathanpeppers jonathanpeppers mentioned this pull request Jul 1, 2022
Closed
@agocke
Copy link
Member Author

agocke commented Jul 11, 2022

SDK change has just moved through been merged, so we probably need to merge it in for this to update.

@agocke
Copy link
Member Author

agocke commented Jul 13, 2022

Updated the linker version manually so this should be ready and merged ASAP

@agocke
Copy link
Member Author

agocke commented Jul 13, 2022

Note: the SDK change is in p7 so this needs to be in p7 as well

@jonpryor
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

@agocke : any links to useful documentation about what $(TrimMode)=partial means, what it does, how to investigate breakage, etc.?

jonathanpeppers
jonathanpeppers reviewed Jul 14, 2022
View reviewed changes
eng/Versions.props Outdated
<MicrosoftNETILLinkTasksPackageVersion>7.0.100-1.22351.1</MicrosoftNETILLinkTasksPackageVersion>
<MicrosoftNETILLinkTasksPackageVersion>7.0.100-1.22362.3</MicrosoftNETILLinkTasksPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Changing this number manually, actually doesn't update the dotnet/linker we are testing against. It only updates the reference assembly this project builds against:

https://github.com/xamarin/xamarin-android/blob/8d22eed397dc56d1ede51010dd192ed65123707f/src/Microsoft.Android.Sdk.ILLink/Microsoft.Android.Sdk.ILLink.csproj#L12

To get a newer dotnet/linker, we bump dotnet/installer.

This one doesn't have your change either (#7162), let me try to get newer Maestro bumps in.

@jonathanpeppers
Copy link
Member

Related to #7132 (comment), I'd like to add a test here for at least TrimMode=partial, and TrimMode=full. Is this the equivalent of our old Xamarin settings AndroidLinkMode=SdkOnly and AndroidLinkMode=Full?

@jonathanpeppers
Copy link
Member

@agocke I think this one would have the changes from dotnet/linker that you need: #7170

@agocke
Copy link
Member Author

agocke commented Jul 14, 2022 •
edited
Loading

I see, in that case I'll revert my changes.

We haven't had a chance to update docs yet. TrimMode=partial only trims things that are marked IsTrimmable (so mostly the SDK), and full trims everything, unless it's opted out in some way (like setting IsTrimmable=false on a particular assembly).

@agocke
Copy link
Member Author

agocke commented Jul 14, 2022

@jonathanpeppers Happy to add a test, but I think android will be broken if we don't merge this soon. The SDK will default to trimming everything unless told otherwise.

@agocke
Copy link
Member Author

agocke commented Jul 14, 2022

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 7132 in repo xamarin/xamarin-android

@jonathanpeppers
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonathanpeppers
Copy link
Member

I think we have an existing test for both modes:

https://github.com/xamarin/xamarin-android/blob/6401f2f7da54379b907295b4fe62b46dffea1a54/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs#L333-L334

AndroidLinkMode=SdkOnly should map to TrimMode=partial, and AndroidLinkMode=Full map to TrimMode=link:

https://github.com/xamarin/xamarin-android/blob/d4f94249a1a8c67747f1551627a3796610619c70/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets#L68-L69

This is probably good enough for now, as this test runs for Xamarin.Android and .NET 7+.

@jonathanpeppers
Copy link
Member

I'm going to merge this into #7170, as it is giving some test failures that show TrimMode=link is on by default.

@jonathanpeppers jonathanpeppers changed the base branch from main to darc-main-1a7fcdae-fd29-44ca-936b-bdbc39f54705 July 15, 2022 16:59
@jonathanpeppers jonathanpeppers merged commit e62dcb0 into dotnet:darc-main-1a7fcdae-fd29-44ca-936b-bdbc39f54705 Jul 15, 2022
@agocke agocke deleted the set-trimmode-partial branch July 17, 2022 06:09
jonathanpeppers added a commit that referenced this pull request Jul 20, 2022
@agocke @jonathanpeppers
Set TrimMode partial by default (#7132)
e8a4d47 
Companion to dotnet/linker#2856

* Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
* Remove more usages of TrimMode
* Update linker versions manually

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
jonathanpeppers added a commit that referenced this pull request Jul 22, 2022
@dotnet-maestro @agocke @jonathanpeppers
Bump to dotnet/installer@a9c056c 7.0.100-rc.1.22368.2 (#7170)
938b2cb 
Changes: dotnet/installer@85a0482...a9c056c
Changes: dotnet/linker@ef2d0f2...d27ff61
Changes: dotnet/runtime@206dccb...072eda8
Changes: dotnet/emsdk@40e7c62...11a9acf

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 7.0.100-preview.7.22361.1 to 7.0.100-rc.1.22368.2
* Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22354.1 to 7.0.100-1.22365.1
* Microsoft.NETCore.App.Ref: from 7.0.0-preview.6.22356.1 to 7.0.0-rc.1.22366.5
* Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-preview.7.22330.1 to 7.0.0-rc.1.22362.2

~~ Set `$(TrimMode)` `partial` by default (#7132) ~~

Companion to dotnet/linker#2856

* Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
* Remove usages of `%(TrimMode)` for `$(AndroidLinkMode)` of `Full`
* Update .apkdesc files

~~ Set `$(LangVersion)` to 10 in Mono.Android.csproj ~~

There appears to be a C# 11 IL size regression in:

dotnet/roslyn#62832

We can use C# 10 for now to avoid this.

* Fixed `Mono.Android.dll` size in `.apkdesc` files

Co-authored-by: Andy Gocke <andy@commentout.net>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
jonathanpeppers added a commit that referenced this pull request Jul 22, 2022
@dotnet-maestro @agocke @jonathanpeppers
Bump to dotnet/installer@dbdda95 7.0.100-preview.7.22370.3 (#7181)
31fe82b 
Changes: dotnet/installer@d2fff6d...dbdda95
Changes: dotnet/linker@ef2d0f2...33a76b8
Changes: dotnet/runtime@206dccb...db5d4df
Changes: dotnet/emsdk@40e7c62...7d27778

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 7.0.100-preview.7.22362.31 to 7.0.100-preview.7.22370.3
* Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22354.1 to 7.0.100-1.22362.1
* Microsoft.NETCore.App.Ref: from 7.0.0-preview.6.22356.1 to 7.0.0-preview.7.22369.4
* Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-preview.7.22330.1 to 7.0.0-preview.7.22361.2

~~ Set `$(TrimMode)` `partial` by default (#7132) ~~

Companion to dotnet/linker#2856

* Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
* Remove usages of `%(TrimMode)` for `$(AndroidLinkMode)` of `Full`
* Update .apkdesc files

~~ Set `$(LangVersion)` to 10 in Mono.Android.csproj ~~

There appears to be a C# 11 IL size regression in:

dotnet/roslyn#62832

We can use C# 10 for now to avoid this.

* Fixed `Mono.Android.dll` size in `.apkdesc` files

Co-authored-by: Andy Gocke <andy@commentout.net>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

@dellis1972 dellis1972 Awaiting requested review from dellis1972

@pjcollins pjcollins Awaiting requested review from pjcollins

@jonpryor jonpryor Awaiting requested review from jonpryor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agocke @jonathanpeppers @jonpryor