-
Notifications
You must be signed in to change notification settings - Fork 289
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
base: main
Are you sure you want to change the base?
Target .NET 6 #766
Conversation
Is there a specific reason why we're doing this? Many folks are banking on this library to help them move away from The folks who are still waiting on NamedPipe support will be left out in the cold. |
@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. |
@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 |
@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: |
@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?
@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. |
What about command line building? Can you use Rosslyn 3.11 at the command line to build? |
That sounds acceptable to me. Moving on 😂 |
@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 |
@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 Examples:
|
@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? |
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. |
There was a problem hiding this 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.
@g7ed6e Thanks for catching those items. I've made all changes- build and test worked locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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 |
Description
Testing