Add net461 TFM to netstandard2.0 projects #25094

wtgodbe · 2020-08-20T20:25:30Z

Resolves #22853

Adds net461 to all netstandard2.0 projects, except analyzers, tests, test assets, and Razor SDK projects

wtgodbe · 2020-08-20T20:30:13Z

current issue:

C:\code\aspnet\aspnetcore\src\SignalR\samples\ClientSample\Tcp\TcpConnection.cs(58,40): error CS1503: Argument 1: cannot convert from 'System.Net.EndPoint' to 'System.Net.Sockets.SocketAsyncEventArgs' [C:\code\aspnet\aspnetcore\src\SignalR\samples\ClientSample\ClientSample.csproj]

aspnetcore/src/SignalR/samples/ClientSample/Tcp/TcpConnection.cs

Line 58 in 4ecde5f

await _socket.ConnectAsync(_endPoint);

ClientSample always targeted net461 and net50 - not sure what I changed that would have caused the failure. It appears it wants to call this overload: https://github.com/dotnet/runtime/blob/885bb524674a22ac90979e64dd71394262ff2530/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketTaskExtensions.cs#L17-L18

dougbu

In addition, suggest adding $(DefaultNetFxTargetFramework) to more of the integration and functional test projects that exercise the updated projects.

dougbu · 2020-08-20T21:26:44Z

src/Analyzers/Analyzers/src/Microsoft.AspNetCore.Analyzers.csproj

    <IsShippingPackage>false</IsShippingPackage>

-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


Not sure whether adding a .NET Framework assembly for analyzers is useful. @pranavkm❔

Yeah, I don't think this should be necessary. The ns2.0 version works in VS and CLI, so unless something's broken there, I wouldn't change these.

dougbu · 2020-08-20T21:27:38Z

....Razor.Extensions.Version1_X/src/Microsoft.AspNetCore.Mvc.Razor.Extensions.Version1_X.csproj

  <PropertyGroup>
    <Description>ASP.NET Core design time hosting infrastructure for the Razor view engine.</Description>
-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


Changes w/in the Razor SDK probably aren't useful. @NTaylorMullen❔

Yup. These extensions are packaged in the SDK. Unless we are shipping the netfx versions for some reason, I'd avoid this change

dougbu · 2020-08-20T21:30:08Z

...Tools/Extensions.ApiDescription.Client/src/Microsoft.Extensions.ApiDescription.Client.csproj

    <PackageId>$(MSBuildProjectName)</PackageId>
    <PackageTags>Build Tasks;MSBuild;Swagger;OpenAPI;code generation;Web API client;service reference</PackageTags>
-    <TargetFrameworks>netstandard2.0</TargetFrameworks>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


Revert changes in this file. The project creates an msbuild task that's always loaded from the netstandard2.0 assembly.

pranavkm

What's the benefit of this change?

pranavkm · 2020-08-20T21:48:54Z

src/Analyzers/Analyzers/src/Microsoft.AspNetCore.Analyzers.csproj

    <IsShippingPackage>false</IsShippingPackage>

-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


Yeah, I don't think this should be necessary. The ns2.0 version works in VS and CLI, so unless something's broken there, I wouldn't change these.

pranavkm · 2020-08-20T21:49:28Z

...yzers/Microsoft.AspNetCore.Analyzer.Testing/src/Microsoft.AspNetCore.Analyzer.Testing.csproj

  <PropertyGroup>
    <Description>Helpers for writing tests for Roslyn analyzers.</Description>
-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


Same thing. This is a test only project, so unless something requires it, I wouldn't cross-compile it.

pranavkm · 2020-08-20T21:52:47Z

...lization/testassets/ResourcesClassLibraryNoAttribute/ResourcesClassLibraryNoAttribute.csproj


  <PropertyGroup>
-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


I would leave testassets be (unless something requires it to be cross-compiled)

pranavkm · 2020-08-20T21:53:35Z

....Razor.Extensions.Version1_X/src/Microsoft.AspNetCore.Mvc.Razor.Extensions.Version1_X.csproj

  <PropertyGroup>
    <Description>ASP.NET Core design time hosting infrastructure for the Razor view engine.</Description>
-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0</TargetFrameworks>


Yup. These extensions are packaged in the SDK. Unless we are shipping the netfx versions for some reason, I'd avoid this change

pranavkm · 2020-08-20T21:54:48Z

...or/test/Microsoft.AspNetCore.Razor.Test.Common/Microsoft.AspNetCore.Razor.Test.Common.csproj

    <DefineConstants Condition="'$(GenerateBaselines)'=='true'">$(DefineConstants);GENERATE_BASELINES</DefineConstants>
    <DefineConstants>$(DefineConstants);__RemoveThisBitTo__GENERATE_BASELINES</DefineConstants>
-    <TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0;net461</TargetFrameworks>


Can you remove the net461 from this?

pranavkm · 2020-08-20T21:54:57Z

...oft.AspNetCore.Razor.Test.ComponentShim/Microsoft.AspNetCore.Razor.Test.ComponentShim.csproj


  <PropertyGroup>
-    <TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
+    <TargetFrameworks>$(DefaultNetFxTargetFramework);netstandard2.0;net461</TargetFrameworks>


net461 appears twice here

dougbu · 2020-08-20T22:36:57Z

What's the benefit of this change?

We have a requirement (expressed in #22853) to better-support customers using .NET Framework i.e. to avoid relying on netstandard2.0. Basically, those writing extensions should use the netstandard2.0 assemblies while application authors should use the desktop or .NET Core assemblies.

wtgodbe · 2020-08-25T17:41:31Z

Source build failures:

##[error].dotnet/sdk/5.0.100-rc.1.20379.10/Sdks/NuGet.Build.Tasks.Pack/buildCrossTargeting/NuGet.Build.Tasks.Pack.targets(198,5): error NU5019: (NETCORE_ENGINEERING_TELEMETRY=Build) File not found: '/__w/1/s/artifacts/bin/Microsoft.AspNetCore.Mvc.Analyzers/Release/Microsoft.AspNetCore.Mvc.Analyzers.dll'.

##[error]src/Razor/Microsoft.AspNetCore.Mvc.Razor.Extensions/src/Microsoft.AspNetCore.Mvc.Razor.Extensions.csproj(0,0): error MSB4057: The target "GetTargetPath" does not exist in the project.

wtgodbe · 2020-08-25T19:31:56Z

@dotnet/aspnet-build I believe this is now ready for review

wtgodbe · 2020-08-25T19:36:24Z

eng/Versions.props

    <SystemCommandlineExperimentalPackageVersion>0.3.0-alpha.19317.1</SystemCommandlineExperimentalPackageVersion>
    <SystemComponentModelPackageVersion>4.3.0</SystemComponentModelPackageVersion>
-    <SystemNetHttpPackageVersion>4.3.2</SystemNetHttpPackageVersion>
+    <SystemNetHttpPackageVersion>4.3.4</SystemNetHttpPackageVersion>


I bumped this up to the latest

wtgodbe · 2020-08-25T19:41:14Z

From the issue:

src\DataProtection\Cryptography.KeyDerivation\src\PBKDF2\NetCorePbkdf2Provider.cs
src\Identity\Extensions.Core\src\PasswordHasher.cs
src\Shared\WebEncoders\WebEncoders.cs
src\SignalR\common\Shared\MemoryBufferWriter.cs
src\SignalR\common\Shared\PipeWriterStream.cs

@dougbu NetCorePbkdf2Provider has no #IFs, while WebEncoders, MemoryBufferWriter, and PipeWriterStream only have #IF NETCOREAPP. I did have to modify PasswordHasher (along with a couple other .cs files) to get the repo to build

dougbu

Small cleanup needed but otherwise looks good

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Microsoft.AspNetCore.Razor.Language.csproj

...rs/Manifest.MSBuildTask/src/Microsoft.Extensions.FileProviders.Embedded.Manifest.Task.csproj

wtgodbe · 2020-08-26T16:11:44Z

Test failures are all the Chrome thing - @dougbu @BrennanConroy @captainsafia do we have an issue tracking that yet?

BrennanConroy · 2020-08-26T16:29:36Z

No issue yet. Probably going to have to port the change to all branches affected. (maybe 2.1+)

src/Configuration.KeyPerFile/test/Microsoft.Extensions.Configuration.KeyPerFile.Tests.csproj

wtgodbe · 2020-08-26T22:19:07Z

Selenium failures 😢 will rebase

dougbu · 2020-08-26T22:49:35Z

will rebase

/fyi @wtgodbe doing the /azp run dance would suffice too. That creates another merge commit w/ the base branch and kicks off AzDO using that. You only really need to rebase PR branches when either (a) you wanna know exactly what you're building or (b) your updates conflict w/ something in the base branch.

wtgodbe · 2020-08-27T14:35:48Z

/azp run

azure-pipelines · 2020-08-27T14:36:05Z

Azure Pipelines successfully started running 2 pipeline(s).

wtgodbe · 2020-08-27T20:16:36Z

/azp run

azure-pipelines · 2020-08-27T20:17:00Z

Azure Pipelines successfully started running 2 pipeline(s).

Pilchie · 2020-08-27T22:39:18Z

Approved for RC1 - thanks @wtgodbe !

wtgodbe requested a review from a team August 20, 2020 20:25

dougbu reviewed Aug 20, 2020

View reviewed changes

pranavkm reviewed Aug 20, 2020

View reviewed changes

Pilchie added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Aug 24, 2020

wtgodbe marked this pull request as ready for review August 25, 2020 19:31

wtgodbe requested review from BrennanConroy, Tratcher, halter73 and jkotalik as code owners August 25, 2020 19:31

wtgodbe commented Aug 25, 2020

View reviewed changes

dougbu approved these changes Aug 25, 2020

View reviewed changes

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Microsoft.AspNetCore.Razor.Language.csproj Outdated Show resolved Hide resolved

...rs/Manifest.MSBuildTask/src/Microsoft.Extensions.FileProviders.Embedded.Manifest.Task.csproj Outdated Show resolved Hide resolved

dougbu mentioned this pull request Aug 26, 2020

[release/5.0] Update dependencies from dotnet/runtime dotnet/efcore #25247

Merged

dougbu reviewed Aug 26, 2020

View reviewed changes

src/Configuration.KeyPerFile/test/Microsoft.Extensions.Configuration.KeyPerFile.Tests.csproj Outdated Show resolved Hide resolved

wtgodbe added 9 commits August 26, 2020 15:20

Add net461 TFM to netstandard2.0 projects

9088f67

Fix a couple of errors

23a7f43

Fix some errors

282a893

Get rid of Sockets reference

f8bba0d

Respond to feedback

1578527

net461 -> property

4c9ec3a

Fixup clientSample

07a85c5

Remove net461 from analyzers/razor

c8c9dc5

Remove net461 from test projects

f9fe05e

wtgodbe added 4 commits August 26, 2020 15:20

Feedback

291db2e

Add net461 test configs

a287ace

Remove some incompatible test configs

569abaa

Fix test

3c547e2

wtgodbe force-pushed the wtgodbe/netfxAssemblies branch from f84a60d to 3c547e2 Compare August 26, 2020 22:20

Pilchie added the Servicing-approved Shiproom has approved the issue label Aug 27, 2020

wtgodbe merged commit ce058f6 into release/5.0 Aug 27, 2020

wtgodbe deleted the wtgodbe/netfxAssemblies branch August 27, 2020 23:05

wtgodbe mentioned this pull request Aug 27, 2020

Add .NET Framework assemblies to 5.0 #22853

Closed

Add net461 TFM to netstandard2.0 projects #25094

Add net461 TFM to netstandard2.0 projects #25094

Uh oh!

Conversation

wtgodbe commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtgodbe commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented Aug 20, 2020

Uh oh!

wtgodbe commented Aug 25, 2020

Uh oh!

wtgodbe commented Aug 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgodbe commented Aug 25, 2020

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wtgodbe commented Aug 26, 2020

Uh oh!

BrennanConroy commented Aug 26, 2020

Uh oh!

Uh oh!

wtgodbe commented Aug 26, 2020

Uh oh!

dougbu commented Aug 26, 2020

Uh oh!

wtgodbe commented Aug 27, 2020

Uh oh!

azure-pipelines bot commented Aug 27, 2020

Uh oh!

wtgodbe commented Aug 27, 2020

Uh oh!

azure-pipelines bot commented Aug 27, 2020

Uh oh!

Pilchie commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wtgodbe commented Aug 20, 2020 •

edited

Loading

wtgodbe commented Aug 20, 2020 •

edited

Loading