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.

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

[Xamarin.Android.Build.Tasks] Add XA1031 error for AndroidHttpClientHandlerType #7448

Merged
merged 6 commits into from Dec 12, 2022

Conversation

dellis1972
Copy link
Contributor

Fixes #7326

We should check the value of the AndroidHttpClientHandlerType property during the build process. This property can have a number of different values depending on what version of Xamarin.Android is being used.

Under Classic a valid value is Xamarin.Android.Net.AndroidClientHandler, however under .NET 6+ the valid values are

  • Xamarin.Android.Net.AndroidMessageHandler.
  • System.Net.Http.SocketsHttpHandler, System.Net.Http.

We should raise an error if the user is using an invalid value.

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

The actual code changes look good. But maybe we should adjust some of the user-facing messages?

Documentation/guides/messages/xa1031.md Outdated Show resolved Hide resolved
src/Xamarin.Android.Build.Tasks/Properties/Resources.resx Outdated Show resolved Hide resolved
@jonpryor
Copy link
Member

jonpryor commented Oct 18, 2022

WIP commit message:

[Xamarin.Android.Build.Tasks] Add XA1031 error (#7448)

Fixes: https://github.com/xamarin/xamarin-android/issues/7326

Commit d3e87674 added a requirement that, under .NET 6+, the
`$(AndroidHttpClientHandlerType)` MUST inherit from
[`System.Net.Http.HttpMessageHandler`][0].  This was unfortunately a
change from Classic, which allowed/required that
[`System.Net.Http.HttpClientHandler`][1] be the base class.
Allowing `HttpClientHandler` to be in the inheritance hierarchy for
`$(AndroidHttpClientHandlerType)` would result in a
`NullReferenceException` at runtime under .NET 6.

Commit d3e87674 contained a TODO:

> TODO: [Emit a build-time warning][2] if
> `$(AndroidHttpClientHandlerType)` is set to an invalid value

Implement this TODO: add a build-time check that verifies that
`$(AndroidHttpClientHandlerType)` is a valid type for the target,
e.g. is an `HttpMessageHandler` subclass on .NET 6.

If `$(AndroidHttpClientHandlerType)` is not a valid type, then an
XA1031 error is raised:

	error XA1031: The 'AndroidHttpClientHandlerType' has an invalid value of '{0}' please check your project settings.

Additionally, if `$(AndroidHttpClientHandlerType)` is
`System.Net.Http.SocketsHttpHandler, System.Net.Http`, *and*
`$(UseNativeHttpHandler)` isn't set, then
set `$(UseNativeHttpHandler)` to false.  This is necessary to ensure
that `SocketsHttpHandler` is preserved by the linker.

[0]: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpmessagehandler?view=net-6.0
[1]: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler?view=net-6.0
[2]: https://github.com/xamarin/xamarin-android/issues/7326

@dellis1972 dellis1972 marked this pull request as ready for review November 11, 2022 14:26
@dellis1972 dellis1972 force-pushed the Issue7326 branch 4 times, most recently from 54e00f8 to 4fb4ac1 Compare November 16, 2022 16:05
@@ -95,6 +95,7 @@
<HttpActivityPropagationSupport Condition="'$(HttpActivityPropagationSupport)' == ''">false</HttpActivityPropagationSupport>
<InvariantGlobalization Condition="'$(InvariantGlobalization)' == ''">false</InvariantGlobalization>
<StartupHookSupport Condition="'$(StartupHookSupport)' == ''">false</StartupHookSupport>
<UseNativeHttpHandler Condition=" $(AndroidHttpClientHandlerType.Contains ('System.Net.Http.SocketsHttpHandler')) And '$(UseNativeHttpHandler)' == '' ">false</UseNativeHttpHandler>
Copy link
Member

Choose a reason for hiding this comment

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

@dellis1972 : this feels like something that should be called out in the commit message. What's the rationale here?

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 left over. But we got info from the runtime team that if a user defines System.Net.Http.SocketsHttpHandler' as the handler type they MUST use set the UseNativeHttpHandler msbuild property. Otherwise it doesn't work.

This is where the conversaton took place https://discord.com/channels/732297728826277939/732297837953679412/1029777522897993748

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove it ("this is left over") or keep it ("extract rationale from Discord")?

…HandlerType`

Fixes xamarin#7326

We should check the value of the `AndroidHttpClientHandlerType` property
during the build process. This property can have a number of
different values depending on what version of Xamarin.Android is being
used.

Under Classic a valid value is `Xamarin.Android.Net.AndroidClientHandler`,
however under .NET 6+ the valid values are

- `Xamarin.Android.Net.AndroidMessageHandler`.
- `System.Net.Http.SocketsHttpHandler, System.Net.Http`.

We should raise an error if the user is using an invalid
value.
@jonpryor jonpryor merged commit 311b41e into xamarin:main Dec 12, 2022
@dellis1972 dellis1972 deleted the Issue7326 branch December 12, 2022 20:26
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 4, 2023
* main:
  [Xamarin.Android.Build.Tasks] use %(TrimmerRootAssembly.RootMode) All (xamarin#7651)
  Bump to dotnet/installer@47a747f 8.0.100-alpha.1.22616.7 (xamarin#7647)
  Bump to dotnet/installer@167a4ed 8.0.100-alpha.1.22611.1 (xamarin#7630)
  Bump to xamarin/java.interop@f8d77fa (xamarin#7638)
  [ci] Fix Designer test paths (xamarin#7635)
  [Xamarin.Android.Build.Tasks] perf improvements for LlvmIrGenerator (xamarin#7626)
  [Xamarin.Android.Build.Tasks] AutoImport `*.webp` files (xamarin#7601)
  Localized file check-in by OneLocBuild Task (xamarin#7632)
  Bump $(ProductVersion) to 13.2.99
  Bump to xamarin/monodroid@c0c933b7 (xamarin#7633)
  [Xamarin.Android.Build.Tasks] Fix aapt_rules.txt corruption (xamarin#7587)
  [Xamarin.Android.Build.Tasks] Add XA1031 error (xamarin#7448)
  Bump to xamarin/java.interop@149d70f (xamarin#7625)
  [CODEOWNERS] More updates to CODEOWNERS (xamarin#7628)
  [Xamarin.Android.Build.Tasks] avoid `File.Exists()` check (xamarin#7621)
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit a warning if AndroidHttpClientHandlerType is invalid
3 participants