Skip to content

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

Merged
jonorossi merged 17 commits into
castleproject:masterfrom
generik0:feature/564-Update-Windsor-TargetFrameworks
Dec 10, 2020
Merged

jonorossi merged 17 commits into
castleproject:masterfrom
generik0:feature/564-Update-Windsor-TargetFrameworks

Conversation

@generik0

Copy link
Copy Markdown
Contributor

@jonorossi

Copy link
Copy Markdown
Member

The build fails.

Comment thread buildscripts/BuildScripts.csproj Outdated
@generik0

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/Castle.Windsor/Castle.Windsor.csproj Outdated
@jonorossi

Copy link
Copy Markdown
Member

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

generik0 commented Dec 3, 2020 •

Copy link
Copy Markdown
Contributor Author

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...

@jonorossi jonorossi mentioned this pull request Dec 4, 2020
@generik0

generik0 commented Dec 7, 2020 •

Copy link
Copy Markdown
Contributor Author

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

generik0 commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

@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

lg2de commented Dec 7, 2020

Copy link
Copy Markdown

@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

stakx commented Dec 7, 2020

Copy link
Copy Markdown
Member

@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

lg2de commented Dec 7, 2020

Copy link
Copy Markdown

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

generik0 commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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 commented Dec 8, 2020

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

@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 generik0 changed the title Change frameworks to net45;netstandard2.0;net5.0 Change frameworks to net45;netstandard2.0 and tests net45;net5.0 Dec 8, 2020
@generik0

generik0 commented Dec 8, 2020 •

Copy link
Copy Markdown
Contributor Author

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

lg2de commented Dec 8, 2020

Copy link
Copy Markdown

@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

generik0 commented Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

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

@lg2de

lg2de commented Dec 8, 2020

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

@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

lg2de commented Dec 8, 2020

Copy link
Copy Markdown

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

@generik0 generik0 changed the title Change frameworks to net45;netstandard2.0 and tests net45;net5.0 Change frameworks to net45;netstandard2.0 and tests net45;net5.0 where applicable Dec 8, 2020
@generik0

generik0 commented Dec 8, 2020

Copy link
Copy Markdown
Contributor Author

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

@jonorossi jonorossi changed the title Change frameworks to net45;netstandard2.0 and tests net45;net5.0 where applicable Change target framework from .NET Standard 1.6 to 2.0, and move .NET Core tests to .NET 5 Dec 9, 2020

@jonorossi jonorossi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a couple of minor comments.

Comment thread src/Castle.Windsor/Castle.Windsor.csproj Outdated
Comment thread src/Castle.Windsor/Castle.Windsor.csproj Outdated
Comment thread src/Castle.Windsor/Castle.Windsor.csproj
@jonorossi jonorossi added this to the vNext milestone Dec 10, 2020
@jonorossi
jonorossi merged commit 3575883 into castleproject:master Dec 10, 2020
@jonorossi

Copy link
Copy Markdown
Member

Thanks, merged.

@jonorossi jonorossi linked an issue Dec 10, 2020 that may be closed by this pull request
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.

Castle.Core target frameworks vs Castle.Windsor

4 participants