Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Aug 28, 2020

Backport of #41400 to release/5.0
Fixes #33125

/cc @aik-jahoda

Customer Impact

Construction of WebRequest, ServicePoint and WebClient will produce a warning:
SYSLIB0014 Use HttpClient instead.

The warning will hit any customer who didn't migrate to HttpClient yet.
The impact is reduced by obsoleting the factory methods, and not the classes. It means the usage of the classes will be without warning, but customer will have enough information where to start with migration.

Testing

Usage of the api produce the compile time warning.

Risk

Customers with warning as errors needs to do a code change/project change. Customer will have to migrate or suppress this specific warning.

@ghost
Copy link

ghost commented Aug 28, 2020

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

@aik-jahoda aik-jahoda requested review from a team and ericstj August 28, 2020 18:34
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

I have some changes requested to be more consistent with the other obsoletions and to add the obsoletion to our list of obsoletions.

Before we can merge this into release/5.0 (RC1), we also need to do a few extra steps:

  1. Treat this as a breaking change
  2. Once the breaking change document is merged into master, socialize it internally (ping me offline and I can share the list of people/teams that should be notified)

The feedback on this PR will of course need to be applied to master first too.

internal const string CodeBaseMessage = "Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility. Use Assembly.Location instead.";
internal const string CodeBaseDiagId = "SYSLIB0012";

internal const string WebRequestMessage = "Use HttpClient instead.";
Copy link
Member

Choose a reason for hiding this comment

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

We just updated the CodeBaseMessage to provide more context on the message. Please expand this message in a similar fashion. This will of course need to be updated throughout the ref assemblies as well.

The need for this becomes more obvious when adding the the obsoletion to the list of obsoletions, which also needs to be done.

<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DefineConstants>$(DefineConstants);NETSTANDARD</DefineConstants>
<IgnoreForCI Condition="'$(TargetOS)' == 'Browser'">true</IgnoreForCI>
<NoWarn>$(NoWarn);SYSLIB0014</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: add a comment indicating what SYSLIB0014 is. We tried to do that with the other SYSLIB obsoletions.

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Reflection.MetadataLoadContext/tests/System.Reflection.MetadataLoadContext.Tests.csproj#L5-L6

<PropertyGroup>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<NoWarn>$(NoWarn);SYSLIB0014</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: leave a comment here; same as above.

<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
<DefineConstants>$(DefineConstants);NETSTANDARD</DefineConstants>
<NoWarn>$(NoWarn);SYSLIB0014</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: leave a comment

@jeffhandley jeffhandley mentioned this pull request Aug 28, 2020
4 tasks
@ericstj
Copy link
Member

ericstj commented Aug 28, 2020

This change will require upstack teams to react. Are we prepared to do that in time for RC1?

@jeffhandley
Copy link
Member

This change will require upstack teams to react. Are we prepared to do that in time for RC1?

The only way to know for sure is to survey the impact. We have been doing that for other late breaking changes.

  1. Searching across all dotnet org repos for usage of the APIs
  2. Reaching out to impacted partner teams to make sure they'll be able to absorb any impact
  3. Following the breaking change process and sending out internal announcements of the breaking change, showing the expected impact and how it will be absorbed

I'll be happy to share with @aik-jahoda some more details if we want to consider working through that to get this in.

@ericstj
Copy link
Member

ericstj commented Aug 28, 2020

I've chatted with a few folks and we're of the mind that this is too late to be adding this into 5.0. Let's still keep it in 6.0 and make sure it is complete early to give folks the message in previews.

@karelz karelz added this to the 5.0.0 milestone Aug 28, 2020
@karelz
Copy link
Member

karelz commented Aug 28, 2020

Closing the PR as we decided to Won't Fix it in 5.0.
The original change in 6.0 will be kept.
Thanks everyone!

@karelz karelz closed this Aug 28, 2020
@jkotas jkotas deleted the backport/pr-41400-to-release/5.0 branch September 1, 2020 14:20
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants