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

Target .NET 6 #766

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Target .NET 6 #766

wants to merge 3 commits into from

Conversation

jonlouie
Copy link
Contributor

@jonlouie jonlouie commented Jul 22, 2022

Description

  • Change target framework of all projects to .NET 6
  • Remove defunct preprocessor directives related to target frameworks
  • Upgrade all nuget packages
  • Remove Roslyn 3.11 build tools
  • Fix compile time errors and broken tests from .NET 6 upgrade

Testing

  • Ran all unit tests

@MeikTranel
Copy link

Is there a specific reason why we're doing this?

Many folks are banking on this library to help them move away from .NET Framework by allowing them to migrate functionaility step by step i.e. Legacy -> ASPNETCore Hosting on .NET Framework -> ASPNETCore Hosting on .NET Core -> Upgrade to NET 6.

The folks who are still waiting on NamedPipe support will be left out in the cold.

@mconnew
Copy link
Member

mconnew commented Jul 25, 2022

@MeikTranel, the move to .NET 6 is needed for multiple reasons, one of which is to support TCP port sharing. The 1.x version has the same support lifecycle as asp.net core 2.1 (which runs on .NET Framework) so isn't going to go out of support any time soon. A best effort approach will be done to port anything new to the 1.x version where reasonable. I'm hoping NamedPipe support fits this criteria as it doesn't depend on anything in .NET 6. WCF Core client NamedPipe support is needed to support TCP port sharing, so all the foundational NamedPipe support work will be completed before I work on the port sharing component so I don't see anything blocking bringing it to 1.x simultaneously. I'm being a little non-committal in the language as there is a possibility that I might run into something which blocks it on asp.net core 2.1, but there's nothing I'm currently aware of.

@g7ed6e
Copy link
Collaborator

g7ed6e commented Jul 28, 2022

@g7ed6e
Copy link
Collaborator

g7ed6e commented Jul 28, 2022

@mconnew i see in the PR description that you aim to drop the Roslyn 3.11 Build.Tools, please do notice that this is not required to take the dependency on .NET 6. The Roslyn 3.11 version of the build tools do exist only to ensure compatibility of the source generator feature for VS2019 users.

edit: @jonlouie to drop the 3.11 Build.Tools and thus the VS2019 support it should be sufficient to drop these 3 files:

@jonlouie
Copy link
Contributor Author

jonlouie commented Jul 28, 2022

@jonlouie Why not taking a dependency to ASP.NET core using FrameworkReference ? please see https://docs.microsoft.com/en-us/aspnet/core/fundamentals/target-aspnetcore?view=aspnetcore-6.0&tabs=visual-studio#use-the-aspnet-core-shared-framework

@g7ed6e I can look into this. I don't see why we couldn't make this adjustment. @mconnew Do you have any reservations about this?

@mconnew i see in the PR description that you aim to drop the Roslyn 3.11 Build.Tools, please do notice that this is not required to take the dependency on .NET 6. The Roslyn 3.11 version of the build tools do exist only to ensure compatibility of the source generator feature for VS2019 users.

@g7ed6e I removed the Roslyn 3.11 Build.Tools project because VS2019 does not support .NET 6. Unless there is still reason to keep the project within the solution, I will take your suggestions for removing the 3 files mentioned in your last comment.

@mconnew
Copy link
Member

mconnew commented Jul 28, 2022

What about command line building? Can you use Rosslyn 3.11 at the command line to build?

@MeikTranel
Copy link

@MeikTranel, the move to .NET 6 is needed for multiple reasons, one of which is to support TCP port sharing. The 1.x version has the same support lifecycle as asp.net core 2.1 (which runs on .NET Framework) so isn't going to go out of support any time soon. A best effort approach will be done to port anything new to the 1.x version where reasonable. I'm hoping NamedPipe support fits this criteria as it doesn't depend on anything in .NET 6. WCF Core client NamedPipe support is needed to support TCP port sharing, so all the foundational NamedPipe support work will be completed before I work on the port sharing component so I don't see anything blocking bringing it to 1.x simultaneously. I'm being a little non-committal in the language as there is a possibility that I might run into something which blocks it on asp.net core 2.1, but there's nothing I'm currently aware of.

That sounds acceptable to me. Moving on 😂

@g7ed6e
Copy link
Collaborator

g7ed6e commented Aug 1, 2022

@g7ed6e I removed the Roslyn 3.11 Build.Tools project because VS2019 does not support .NET 6. Unless there is still reason to keep the project within the solution, I will take your suggestions for removing the 3 files mentioned in your last comment.

@jonlouie I did not know that VS2019 does not support .net6. It makes sense indeed to drop support of BuildTools in VS2019 in this PR

@g7ed6e
Copy link
Collaborator

g7ed6e commented Aug 1, 2022

What about command line building? Can you use Rosslyn 3.11 at the command line to build?

@mconnew As CoreWCF will target .net 6 and .net 6 requires .net 6 sdk to build and Roslyn4 is installed with .net6 i think we can assume only .net5 users would need Roslyn3.11 to command line build. And .net 5 is now EOL.

@jonlouie
Copy link
Contributor Author

jonlouie commented Aug 1, 2022

What about command line building? Can you use Rosslyn 3.11 at the command line to build?

@mconnew As CoreWCF will target .net 6 and .net 6 requires .net 6 sdk to build and Roslyn4 is installed with .net6 i think we can assume only .net5 users would need Roslyn3.11 to command line build. And .net 5 is now EOL.

@g7ed6e Thanks for the additional context.

@mconnew I did attempt to build using the command line and Roslyn 3.11. The build output reported 0 errors but there were some concerning warnings, notably CS8032: Could not load file or assembly or one of its dependencies. There were no such warnings using MSBuild with Roslyn 4.x.

Examples:

CSC : warning CS8032: An instance of analyzer Microsoft.AspNetCore.Analyzers.RouteHandlers.RouteHandlerAnalyzer cannot be created from C:\Program Files\dotnet\packs\Microsoft.AspNetCore.App.Ref\6.0.5\analyzers\dotnet\cs\Microsoft.AspNetCore.App.Analyzers.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.. [D:\Workplace\CoreWCF\src\CoreWCF.WebHttp\src\CoreWCF.WebHttp.csproj]
CSC : warning CS8032: An instance of analyzer Microsoft.Extensions.Logging.Generators.LoggerMessageGenerator cannot be created from C:\Program Files\dotnet\packs\Microsoft.AspNetCore.App.Ref\6.0.5\analyzers\dotnet\roslyn4.0\cs\Microsoft.Extensions.Logging.Generators.dll: Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.. [D:\Workplace\CoreWCF\src\CoreWCF.WebHttp\src\CoreWCF.WebHttp.csproj]

@jonlouie
Copy link
Contributor Author

jonlouie commented Aug 4, 2022

@mconnew Tests pass locally, but there is a failure in the test pipeline between tests. The error occurs when writing to the test logger- has this been seen before and is it flaky behavior?

https://dev.azure.com/dotnet/CoreWCF/_build/results?buildId=76059&view=logs&j=b022b3f7-954a-5bee-8630-c83c3a9b9ce4&t=8fb4180c-4405-53d3-3e02-65aad02e9564&l=1761

@mconnew
Copy link
Member

mconnew commented Aug 4, 2022

Posting this in case someone else hits a similar issue in the future. I already explained the issue offline to Jon. Basically the ILogger passed to asp.net core which wraps the ITestOutputHelper (for logging things for the test) is being used after the test method has returned. This is likely due to the WebHost not being stopped before the test returns.

Copy link
Collaborator

@g7ed6e g7ed6e left a comment

Choose a reason for hiding this comment

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

@jonlouie it looks good to me :). There's still some minor changes required to clean the csproj files.

src/CoreWCF.WebHttp/tests/CoreWCF.WebHttp.Tests.csproj Outdated Show resolved Hide resolved
src/CoreWCF.Http/src/CoreWCF.Http.csproj Outdated Show resolved Hide resolved
src/CoreWCF.NetTcp/src/CoreWCF.NetTcp.csproj Outdated Show resolved Hide resolved
src/CoreWCF.NetTcp/src/CoreWCF.NetTcp.csproj Outdated Show resolved Hide resolved
src/CoreWCF.Primitives/src/CoreWCF.Primitives.csproj Outdated Show resolved Hide resolved
@jonlouie
Copy link
Contributor Author

jonlouie commented Aug 8, 2022

@g7ed6e Thanks for catching those items. I've made all changes- build and test worked locally.

@jonlouie jonlouie requested a review from g7ed6e August 8, 2022 20:46
Copy link
Collaborator

@g7ed6e g7ed6e left a comment

Choose a reason for hiding this comment

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

Thanks!

@Havunen
Copy link

Havunen commented Sep 5, 2024

Is there some particular reason why this PR does not target multiple target frameworks. This way the project could support netstandard2.0 and also the newer target frameworks allowing using compiler directives to change the execution code if needed. Also I believe many of the external references (https://www.nuget.org/packages/CoreWCF.Primitives) could be dropped for modern target frameworks like NET8.0

I can help implementing that if there is interest pushing it forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants