Skip to content

[mobile] Add extra HttpClientHandler linker substitution #64818

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
Merged
merged 2 commits into from
Feb 7, 2022
Merged

[mobile] Add extra HttpClientHandler linker substitution #64818

merged 2 commits into from
Feb 7, 2022

Conversation

steveisok
Copy link
Member

The linker substitution for HttpClientHandler.IsNativeHandlerEnabled only worked when in a project was set to false. Since the default for MAUI projects is true, this caused us to carry more assemblies than needed.

Fixes #64361

[mobile] Add extra HttpClientHandler linker substitution
7de11a4 
The linker substitution for HttpClientHandler.IsNativeHandlerEnabled only worked when <UseNativeHttpHandler> in a project was set
to false.  Since the default for MAUI projects is true, this caused us to carry more assemblies than needed.

Fixes dotnet#64361
@ghost ghost added area-System.Net.Http linkable-framework Issues associated with delivering a linker friendly framework labels Feb 4, 2022
@ghost ghost assigned steveisok Feb 4, 2022
@steveisok steveisok requested a review from eerhardt February 4, 2022 17:44
@ghost
Copy link

ghost commented Feb 4, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

The linker substitution for HttpClientHandler.IsNativeHandlerEnabled only worked when in a project was set to false. Since the default for MAUI projects is true, this caused us to carry more assemblies than needed.

Fixes #64361

Author: steveisok
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

eerhardt
eerhardt reviewed Feb 4, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Any way we can add a test for this? Maybe adding a https://github.com/dotnet/runtime/tree/main/src/libraries/System.Net.Http/tests/TrimmingTests ?

You can set feature switches in the trimming test following this pattern:

runtime/src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj

Lines 18 to 20 in 02ac5c3

<TestConsoleAppSourceFiles Include="InvariantGlobalizationTrue.cs">
<EnabledFeatureSwitches>System.Globalization.Invariant;System.Globalization.PredefinedCulturesOnly</EnabledFeatureSwitches>
</TestConsoleAppSourceFiles>

cc @jonathanpeppers

@eerhardt eerhardt added the size-reduction Issues impacting final app size primary for size sensitive workloads label Feb 4, 2022
@ghost
Copy link

ghost commented Feb 4, 2022

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

The linker substitution for HttpClientHandler.IsNativeHandlerEnabled only worked when in a project was set to false. Since the default for MAUI projects is true, this caused us to carry more assemblies than needed.

Fixes #64361

Author: steveisok
Assignees: steveisok
Labels:

area-System.Net.Http, linkable-framework, size-reduction
Milestone: -

@eerhardt eerhardt removed the linkable-framework Issues associated with delivering a linker friendly framework label Feb 4, 2022
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 4, 2022
@ghost
Copy link

ghost commented Feb 4, 2022

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

The linker substitution for HttpClientHandler.IsNativeHandlerEnabled only worked when in a project was set to false. Since the default for MAUI projects is true, this caused us to carry more assemblies than needed.

Fixes #64361

Author: steveisok
Assignees: steveisok
Labels:

area-System.Net.Http, linkable-framework, size-reduction
Milestone: -

eerhardt
eerhardt approved these changes Feb 4, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This LGTM. Thanks for getting this fixed! It should really help app size in Maui.

Rethinking the trimming test - it isn't currently possible to write the test I mention above because we don't run the trimming tests on android or ios. See #39274

@steveisok
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2022

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1800182990

@github-actions github-actions bot mentioned this pull request Feb 5, 2022
Merged
@marek-safar marek-safar merged commit 179ffff into dotnet:main Feb 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

@eerhardt eerhardt eerhardt approved these changes

@marek-safar marek-safar marek-safar approved these changes

@simonrozsival simonrozsival simonrozsival approved these changes

Assignees

@steveisok steveisok

Labels
area-System.Net.Http linkable-framework Issues associated with delivering a linker friendly framework size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

UseNativeHttpHandler=true is not trimming correctly on mobile apps
6 participants
@steveisok @marek-safar @simonrozsival @jonathanpeppers @eerhardt @karelz