Change target framework from .NET Standard 1.6 to 2.0, and move .NET Core tests to .NET 5

[generik0]

#564

[jonorossi]

The build fails.

[generik0]

@jonorossi Ready. Change in test framework and those nasty preprocessor labels we have in windsor :-/

[jonorossi]

There is a few of these warnings in the build log, should we be doing something about them?

C:\Program Files\dotnet\sdk\5.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.EolTargetFrameworks.targets(28,5): warning NETSDK1138: The target framework 'netcoreapp2.0' is out of support and will not receive security updates in the future. Please refer to https://aka.ms/dotnet-core-support for more information about the support policy.

[generik0]

There is a few of these warnings in the build log, should we be doing something about them?

C:\Program Files\dotnet\sdk\5.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.EolTargetFrameworks.targets(28,5): warning NETSDK1138: The target framework 'netcoreapp2.0' is out of support and will not receive security updates in the future. Please refer to https://aka.ms/dotnet-core-support for more information about the support policy.

The netcoreapp2.0 issue is from the Castle.Facilities.AspNetCore tests. The Castle.Facilities.AspNetCore as you know is only for netcoreapp2.0, and the netcoreapp2.0 tests

The netcoreapp2.2 is from the new dependency injection extension tests. I have removed, but I actually thought it was nice to show that netcoreapp2.2 is supported with the dependency injection extension.

My question is. Can we not remove the Castle.Facilities.AspNetCore and Castle.Facilities.AspNetCore.Tests. Are we making new builds of this? If people want to use this facility should they not use Windsor 5.0? I guess this raises the old question if the old asp.net, wcf and now alos aspnetcore facility shouldn't be moved to an obsolete repo. But I think you should decide here :-)

As with the build issue. I can add the warnings to the build ignore.? or we can leave it. What is best?

@jonorossi I have tried with 1138, SDK1138 and NETSDK1138 in the tests csproj. I cannot stop the warning...

[generik0]

Hi @stakx and @lg2de we are looking into moving castle.core's target frameworks to Castle.Windsor.
Can you tell us why do you build Castle.Core for both ;netstandard2.0 and netstandard2.1. and not just ;netstandard2.0? Is there a technical reason? Thanks for your time

[generik0]

@jonorossi once we get all the details cleared up, i think i will create a new PR with less commits. A more clean/correct PR / Branch

[lg2de]

@generik0 The library Castle.Core may not need to build for netstandard2.1 because it does not use new features.
BUT, other projects refer to this base library. Depending on its own target it needs to refer "compatibility packages" for new function not available in netstandard2.0.

I run into problems with the limited list of target framework because we have an internal approval process for packages from nuget.org. The more packages are referenced indirectly the more packages we need to approve.
Please do not ask me at the moment for more details. I've analyzed this months ago. I guess it was related to the package "Moq".

I recommend to target netstandard2.0 AND netstandard2.1 for more flexibility to all users.

[stakx]

@generik0:

Can you tell us why do you build Castle.Core for both ;netstandard2.0 and netstandard2.1. and not just ;netstandard2.0?

This was decided in castleproject/Core#407 (comment). I'll try to summarize from what I remember: I argued that we should target netstandard2.1, since netstandard2.0 doesn't officially support System.Reflection.Emit, which DynamicProxy heavily depends on. @jonorossi agreed that this is the .NET Standard version we want upstream code to target in the longer term, but felt that we should still have netstandard2.0 (I suppose in order to have .NET Core 2.x covered) because in practice, System.Reflection.Emit will still work just fine; so the "missing official support" argument by itself is not good enough to exclude netstandard2.0.

(Btw. while we're on the topic of target frameworks, re: net5.0 – there has been an announcement that .NET Standard 2.1 will be the last version of .NET Standard. Fortunately, .NET 5 implements netstandard2.1 so as long as you don't require any new platform or language features, you shouldn't have to add net5.0 as a target framework.)

[lg2de]

so as long as you don't require any new platform or language features, you shouldn't have to add net5.0 as a target framework.

With the same arguments as above I vote for adding net5.0 in addition to other targets. Building libraries on top of Castle (Core), the user has more flexibility to reference what is required for each target.

[generik0]

@jonorossi I think you need to decide. I have no preference for any of it :-)

However i do feel we should mirror Castle.Core and make tests run to each included framework. Hence for now make Castle.Windsor use .net45, .netstandard2.0 and .netstandard2.1. and the tests .net472, and .net5.0.

In the future --> if Castle.Core add a framework or Castle.Windsor needs something specific from a framework then add this also.

[jonorossi]

Can we not remove the Castle.Facilities.AspNetCore

I have no problem removing the obsolete facilities now that we've got a proper Extensions DependencyInjection one. Those that are stuck on old versions of ASP.NET Core won't need a new Windsor anyway. Log an issue to everyone following alone will see the notification.

once we get all the details cleared up, i think i will create a new PR with less commits. A more clean/correct PR / Branch

Just use rebase to clean up your commits on the branch, GitHub will detect the changes and automatically update this pull request. Or I'll just squash everything together before merging.

Can you tell us why do you build Castle.Core for both ;netstandard2.0 and netstandard2.1. and not just ;netstandard2.0?

As @stakx explained, there is annoying problems with reflection emit not properly being exposed with specific .NET Standard versions. We can do something different here with Windsor.

With the same arguments as above I vote for adding net5.0 in addition to other targets

@lg2de if Castle Core doesn't target net5.0 yet I assume it won't help you reduce the number of dependencies?

However i do feel we should mirror Castle.Core and make tests run to each included framework. Hence for now make Castle.Windsor use .net45, .netstandard2.0 and .netstandard2.1. and the tests .net472, and .net5.0.

In the future --> if Castle.Core add a framework or Castle.Windsor needs something specific from a framework then add this also.

We could change Windsor from net45;netstandard1.6 to just netstandard2.0, that'd give us support for .NET Core 2.0+ and .NET Framework 4.6.1+, I see no need to support less than .NET Framework 4.6.1 at this point, there are always older versions of Windsor people can use. Then once Castle Core adds net5.0 we can look at adding it here too if it helps simplify dependencies.

[lg2de]

@lg2de if Castle Core doesn't target net5.0 yet I assume it won't help you reduce the number of dependencies?

The less dependencies the better!
A library which is used quite often in other libraries should be provided with minimal and exact matching target framework.

I do not understand your question in relation to net5.0.

[jonorossi]

@lg2de you said:

I run into problems with the limited list of target framework because we have an internal approval process for packages from nuget.org. The more packages are referenced indirectly the more packages we need to approve.
Please do not ask me at the moment for more details. I've analyzed this months ago. I guess it was related to the package "Moq".

and:

With the same arguments as above I vote for adding net5.0 in addition to other targets. Building libraries on top of Castle (Core), the user has more flexibility to reference what is required for each target.

You are requesting something but providing little information, I attempted to read between the lines but obviously was wrong. I thought you were saying that under net5.0 TFM it has less corefx libraries or something, we definitely know that .NET Standard 1.x TFMs have heaps of implicit dependencies, adding .NET Standard 2.x will fix that problem. Can you please explain why it would help you if we add the net5.0 TFM.

[generik0]

Hi, @jonorossi looks like the PR is good then.
Change frameworks to core net45;netstandard2.0 and tests net45;net5.0
Should we start her, And Continue the discussion for other changes in #564

Please squash, ok? I have run a little out of time :-)

[lg2de]

@jonorossi, I did not request anything here. I was asked for details on discussion months ago.
I tried to remember. Unfortunately I do NOT get back all the details.
I think it was a complex constellation with multiple libraries referencing over several steps causing unexpected high number of packages finally referenced (directly and indirectly) in MY project.

At the time of my analysis net5.0 was not available, or at least not in my focus.

Can you please explain why it would help you if we add the net5.0 TFM.

At the moment I can not.
I was focusing on Castle.Core only, not Windsor. So, please proceed with the information you have.
As soon I hit the problem again, I'll reanalyze again with the current situation.

[generik0]

Yes, I looked in the Castle.Core history, and tried to discover why there was netstandard2.1 and asked for some historical information:-)

[lg2de]

Yes, I looked in the Castle.Core history, and tried to discover why there was netstandard2.1 and asked for some historical information:-)

... and stakx gave the correct answer I did not remember :(

[jonorossi]

@lg2de I understood that part and knew exactly why Castle Core has .NET Standard 2.0 and 2.1. I was only asking about this comment of yours because that is what we are discussing, if we should add net5.0 or not:

With the same arguments as above I vote for adding net5.0 in addition to other targets.

[lg2de]

At the moment I do not have strong arguments to add net5.0.

[generik0]

I think we are good with "Change frameworks to net45;netstandard2.0 and tests net45;net5.0 where applicable"

[jonorossi]

Just a couple of minor comments.

[jonorossi]

Thanks, merged.