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

.NET Target Frameworks for Castle Core v5.0 #597

Closed
jonorossi opened this issue Sep 3, 2021 · 40 comments
Closed

.NET Target Frameworks for Castle Core v5.0 #597

jonorossi opened this issue Sep 3, 2021 · 40 comments
Milestone

Comments

@jonorossi
Copy link
Member

jonorossi commented Sep 3, 2021

We've made changes to our target frameworks for our next major version but none of this has been released yet. With so much time passed and the .NET Core landscape changing I think we need to re-evaluate our target frameworks before releasing.

.NET Core notes:

.NET Framework notes:

Our set up:

<!-- v4.4.1 -->
<TargetFrameworks>net35;net40;net45;netstandard1.3;netstandard1.5</TargetFrameworks>

<!-- master -->
<TargetFrameworks>net45;netstandard2.0;netstandard2.1</TargetFrameworks>

What do we support for v5.0? I don't think we should be supporting any .NET Core versions that are now end of life, there is no reason for people to expect we support new versions of Castle Core on end of life runtimes. Do we continue with any .NET Standard TFMs? Even though DynamicProxy might not work on some .NET Standard-based runtimes, Windsor depends on Castle Core and so we shouldn't restrict Windsor unnecessarily.

/cc @stakx

@jonorossi jonorossi added this to the v5.0.0 milestone Sep 3, 2021
@stakx

This comment has been minimized.

@stakx
Copy link
Member

stakx commented Sep 4, 2021

Sorry, please ignore my above post. I misread some relevant parts of the top post.

TL;DR: I think the targets we have now are still fine. I understand the desire to only target supported versions, but OTOH when it's easy for us to support older versions (no code changes needed), why shouldn't we? We'll otherwise force downstream libraries to update their targets, and I'm not sure we need to do so. What are the actual downsides to targeting unsupported older versions?

This isn't the same situation now as when we dropped support for .NET 3.5 and .NET Standard 1.x; dropping those allowed us to simplify our code and conditional compilation. This time, by dropping netstandard2.x we likely wouldn't gain anything significant.

Re: exchanging .NET Standard, citing from here:

When to target net5.0 vs. netstandard

For existing code that targets netstandard, there's no need to change the TFM to net5.0. .NET 5.0 implements .NET Standard 2.1 and earlier. The only reason to retarget from .NET Standard to .NET 5.0 would be to gain access to more runtime features, language features, or APIs.

I'm not sure we need any new APIs just now. The major one we've been waiting for is probably AssemblyBuilder.Save, AFAIK that still hasn't arrived.

@talenor
Copy link

talenor commented Dec 7, 2021

Do we know any dates when Castle Core v5.0 will be released? We are waiting for .netstandard 2.0 support as a step forward for migration to .net 6 from .net 4.8.

Seems it is the latest item for this release

@jonorossi
Copy link
Member Author

@talenor Does netstandard1.5 not work on .NET 6, or does the big list of BCL packages cause other problems?

@talenor
Copy link

talenor commented Dec 7, 2021

@jonorossi Well... we haven't dived deep into netstandard1.5 because netstandard 2.0 exists quite a long time and we checked only 2.0 so I can't provide a reasonable answer.
However, in my opinion, 2.0 brings more stable and solid support. Especially if we are talking of an application with ~500k lines of .net code

@jonorossi
Copy link
Member Author

@talenor Are you just using Castle.Core (maybe via a mocking library), or other Castle libraries too?

@talenor
Copy link

talenor commented Dec 7, 2021

@jonorossi only Castle.Core (directly, via some ninject extensions, and via Moq)

@Havunen
Copy link

Havunen commented Dec 15, 2021

We are also waiting for this because we want to get rid off these old assemblies that come from old version of netstandard libraries #602

When there is newer target framework defined then those assemblies dont get installed. This is why .net6 target framework would be good

@Havunen
Copy link

Havunen commented Dec 16, 2021

Also since .net5 that is the recommended way forward from microsoft. https://docs.microsoft.com/en-us/dotnet/standard/frameworks

.net 5 and .net 6 TFMs should be included in next major IMO

@Havunen
Copy link

Havunen commented Dec 28, 2021

Is there any reason why castle core cannot just add netstandard 2+ / net5 / net6 target frameworks and reduce the big number of unnecessary dependencies for those target frameworks https://www.nuget.org/packages/Castle.Core/ This way modern applications can get rid off possible security vulnerabilities #602 and reduce installation size?

@jonorossi jonorossi mentioned this issue Apr 4, 2022
6 tasks
@jonorossi
Copy link
Member Author

What are the actual downsides to targeting unsupported older versions?

I know that with .NET Framework the target framework affects things at runtime (e.g. default SSL/TLS protocols enabled is a big one until you upgrade to 4.6.2), but that could only apply to the executable and not a library.

@jonorossi
Copy link
Member Author

@stakx's quote from Microsoft Docs is still there regarding whether we need net5.0 or net6.0 now under the "When to target net5.0 or net6.0 vs. netstandard" heading.

Any one with a concrete reason for wanting the target frameworks to change for v5.0? Otherwise I'll close this by the end of the week.

@Jevonius
Copy link
Contributor

Jevonius commented Apr 6, 2022

Hi @jonorossi, what would you define as a concrete reason? We bounced a couple of messages in #602 relating to transitive references, and that's probably one reason I'd push for including net6.0 as a target.

Reviewing the Microsoft Docs page from above, there is this in the same section under Reusable libraries:

Most widely used libraries will multi-target for both .NET Standard 2.0 and .NET 5+. Supporting .NET Standard 2.0 gives you the most reach, while supporting .NET 5+ ensures you can leverage the latest platform features for customers that are already on .NET 5+.

It doesn't seem entirely unreasonable to assume that having a net6.0 (and net5.0 for that matter, despite it being out of support) target would allow modern consuming projects that don't otherwise reference anything netstandard to streamline their dependency trees (and build quicker, with less transient older dependencies).

There's also less chance of subtle bugs, such as breaking changes causing issues, where older versions are used because of Castle pulling them down. I had an issue recently with Kestrel, where my project was pulling in an older version because of a dependency, but other code was using newer shared DLLs that built fine, but then broke at runtime because of changed functionality.

@stakx
Copy link
Member

stakx commented Apr 6, 2022

while supporting .NET 5+ ensures you can leverage the latest platform features for customers that are already on .NET 5+

A net50 application referencing a netstandard2.x library will be able to use .NET 5 features. Only the library itself will be limited to the .NET Standard 2.x feature set... right? So if that feature set actually suffices for the library, what would it gain by adding a net50 TFM? (I fail to see the benefit.)

Or does the lack of a net50 TFM in the library somehow "downgrade" the referencing net50 application? (I don't think so, but I may not be up to date with NuGet's sometimes weird versioning algorithm.)

@jonorossi
Copy link
Member Author

I had a quick look at some of the top 100 by downloads on nuget.org (https://www.nuget.org/stats/packages) and found the most downloaded (e.g. Json.NET, Serilog, nlog, log4net) that run across heaps of frameworks didn't have a .NET 5/6 target. As I went down the list I found a bunch that do (e.g. StackExchange.Redis, FluentValidation, Google.Protobuf, Npgsql, RestSharp, Autofac), but each of them use at least one new API or they replaced older netcoreapp with net5/6.

Happy to add a new target if it solves a problem, but similar to @stakx I too am not seeing what we gain with another target to build, run unit tests against and ship around in the nuget package.

@pwdst
Copy link

pwdst commented Apr 6, 2022

I think that point about targetting .NET 5 or .NET 6 is that netstandard requires package references that are not required if targetting the actual framework because those are part of the runtime. Thus a smaller dependency tree, which is much easier to manage for those who have requirements to run package license and security scans. It most likely means that some of the dependencies are more up to date, as netstandard likely references the lowest supported version of a dependency. I know that we have had to reference up to date versions of System.Net.Http and System.Text.RegularExpressions in projects which contain a reference that targets .NETStandard 1.0 as the implicit versions referenced were from 2017 and had known high severity security vulnerabilities. As these packages were not needed by the actual project, which had access to those classes and namespaces as part of the framework, it was necessary to install additional packages simply to update implicitly added references and satisfy a security scanning tool. I believe this is what @Jevonius is referring to.

@jonorossi
Copy link
Member Author

streamline their dependency trees (and build quicker, with less transient older dependencies).

@Jevonius is this true, I'm no expert on this stuff. If so, that's been enough.

@jonorossi
Copy link
Member Author

@Jevonius @pwdst If we added net6.0 would we then just keep bumping it to the next lowest supported .NET each major DP version since things move at a pretty rapid pace now.

Is there any recommendation or feeling on how long we'd want to even keep a .NET Standard target for anymore? If we kept a net4.x target for .NET Framework and added net5/6 for .NET, at some point all the old .NET Core versions are well and truly obsolete and unnecessary to support. We know that DynamicProxy doesn't work on all .NET Standard targets and that we just let the runtime throw a platform not supported exception.

I assume with the .NET linker project in development, things probably work much better without .NET Standard too which is sort of obsolete now.

I think having a single .net5/6 wouldn't be too much of a problem, just don't see the need to have both. .NET 5 is end of life May 8 so no one should be doing new development using it, I'd have no problem just doing net6.0 if it solves the dependency messy problems with .NET Standard.

@jonorossi
Copy link
Member Author

Correction for my previous comment. One reason to kept a .NET Standard target is for Windsor, if it wants to target .NET Standard Castle Core also needs to target .NET Standard.

@pwdst
Copy link

pwdst commented Apr 6, 2022

It is worth noting that Newtonsoft.JSON has not had a release for over a year, is heavily constrained by previous design decisions that the author has publically said would not be made today, and has no public roadmap for future releases (at least when I last checked a couple of weeks ago).

Microsoft has largely replaced it with System.Text.Json by default as of .NET Core 3.0 - authors are required to opt back in to Newtonsoft.JSON after this point. The latest stable version release two months ago targets .NET Framework 4.6.1, .NET Core 3.1, .NET 6 and .NETStandard 2.0 - the most recent preview version drops explicit .NET Core 3.1 support, bumps .NET Framework to 4.6.2 and adds .NET 7.

This is clearly how Microsoft believe that libraries should handle target frameworks.

That said, with a Long Term Support version of .NET Core and now .NET offered periodically, I would probably offer the following-

  • .NET Framework 4.6.2
  • .NETStandard 2.0
  • .NET 6 (and then the latest LTS version when available)

I would probably ignore the non LTS releases unless they added a new API that was useful to the library.

@stakx
Copy link
Member

stakx commented Apr 6, 2022

Agreed that if adding net50 / net60 results in much fewer assemblies getting added to an application's output dir, then fine.

Let's just not confuse today's situation with how it was back then with netstandard1.x, which resulted in a ton of additional framework assembly dependencies. With netstandard2.x, that situation was much improved, all of those were collected into a single (additional) assembly that redirects framework types to the proper location.

I can't judge whether that one netstandard assembly (whatever it's called, I'd have to look that up) is going to spell trouble for people doing security auditing etc. Happy to go along with a recommendation.

@pwdst
Copy link

pwdst commented Apr 6, 2022

@jonorossi I don't believe that anyone is asking that .NETStandard support is dropped or taken away as a target framework, simply that a recent .NET framework is added as additional target. This would allow the library to be used both in projects that target .NET Standard themselves, and also have potential benefits for projects that are targetting the full framework. I would imagine a supported LTS version would be more than sufficient for any authors. For a planned release of Castle.Core that would probably be .NET 6, as .NET Core 3.1 is ending LTS support this year and authors should be upgrading anyway.

@Jevonius
Copy link
Contributor

Jevonius commented Apr 6, 2022

@Jevonius is this true, I'm no expert on this stuff. If so, that's been enough.

I'm not an expert either! @pwdst's answer (#597 (comment)) aligns with what we've observed but it's all anecdotal.

Your follow on question around when to drop support for framework/standard/older cores is worth some thought - the jump to 5.0.0 would seem like a reasonable time to drop support for older versions, and @pwdst's suggestion around supporting net462, netstandard2.0 and net6.0 sounds reasonable. Would the build pipeline allow for a 4.X branch to be maintained/released if any critical issues did arise requiring a release for older no-longer-supported targets?

As an aside, as I understand it the guy that wrote Newtonsoft.JSON now works for Microsoft and is part of the System.Text.Json team, which probably explains why the original library is in decline.

@jonorossi
Copy link
Member Author

Let's just not confuse today's situation with how it was back then with netstandard1.x, which resulted in a ton of additional framework assembly dependencies. With netstandard2.x, that situation was much improved, all of those were collected into a single (additional) assembly that redirects framework types to the proper location.

I can't judge whether that one netstandard assembly (whatever it's called, I'd have to look that up) is going to spell trouble for people doing security auditing etc. Happy to go along with a recommendation.

NETStandard.Library is that package, but when using .NET Standard 2.0 it looks like it only has Microsoft.NETCore.Platforms as a dependency which has no more dependencies.

I don't believe that anyone is asking that .NETStandard support is dropped or taken away as a target framework

Sorry, I didn't mean for it to come across like that. Just trying to understand the landscape better. I've really lost track of the "best practice" guidance for target frameworks in the last couple of years.

I would imagine a supported LTS version [...] that would probably be .NET 6, as .NET Core 3.1 is ending LTS support this year and authors should be upgrading anyway.

Sounds good.

In master right now we've got net45;netstandard2.0;netstandard2.1, so:

  • At this point I see no need to support net45 as even the Windows Server 2008 R2 servers I've recently heard of are running .NET Framework 4.8. 4.6.2 sounds good to me. Our unit tests are targeting 4.6.1 because they need to, so we bump that to 4.6.2 too.
  • Leave .NET Standard 2.0
  • You are proposing .NET Standard 2.0, if we drop .NET Standard 2.1 in favour of net6.0 then we don't actually get an extra one anyway. I'm not seeing any difference in common.prop between the .NET Standard 2.0 and 2.1 builds. We'll need to clean up the unit tests that are currently targeting netcoreapp2.1/3.1.

@Jevonius @pwdst Do either of you have the time to contribute part or all of this work?

@Jevonius
Copy link
Contributor

Jevonius commented Apr 6, 2022

To be clear, you mean retargeting the projects and putting a PR together? I should have some time for that, it's a fairly minor tweak and hopefully won't break any tests!

@jonorossi
Copy link
Member Author

if we drop .NET Standard 2.1 in favour of net6.0

Clarification for those reading at home: dropping it from master. As those here know, we've never had a release with a .NET Standard 2.x target.

To be clear, you mean retargeting the projects and putting a PR together? I should have some time for that, it's a fairly minor tweak and hopefully won't break any tests!

Yes, Probably best to do them as 2 separate commits (i.e. netfx and net6), or even 2 separate pull requests. Our build has Mono building and testing too. Our set up is a little unique, make sure to check out the files in the buildscripts directory which drives a bunch of it.

@Jevonius
Copy link
Contributor

Jevonius commented Apr 6, 2022

As suggested, I've created a PR (#614) moving the minimum version up to 4.6.2. First attempt at contributing, but I've tried to be guided by previous PRs/commit structures so hopefully all in order. If happy with this, I'll take a look at the changes for net6 now that I've got a better grasp of how things are structured.

@stakx
Copy link
Member

stakx commented Apr 7, 2022

  • Leave .NET Standard 2.0
  • You are proposing .NET Standard 2.0, if we drop .NET Standard 2.1 [...] I'm not seeing any difference in common.prop between the .NET Standard 2.0 and 2.1 builds.

Just for reference, the netstandard2.x targets were discussed in detail back in #407. IIRC, the main reason why we have netstandard2.1 is our dependency on System.Reflection.Emit (which .netstandard2.0 never officially supported, but got to work nevertheless through some magic extra FCL NuGet package). Other than that, netstandard2.1 and netstandard2.0 are pretty much exchangeable for this project.

Quoting from @jonorossi's decision from #407 (comment):

likely no one really cares that .NET Standard 2.0 doesn't really support reflection emit, and we should [...] add .NET Standard 2.0 to get rid of all the cruft of every BCL package being listed. At the same time add .NET Standard 2.1 because that is the target we want people to use, as we'll drop .NET Standard 2.0 in a couple of years.

Since it now looks like we're going to drop support for ancient framework versions, perhaps we should drop netstandard2.0 instead, and keep netstandard2.1?

@jonorossi
Copy link
Member Author

Just for reference, the netstandard2.x targets were discussed in detail back in #407. IIRC, the main reason why we have netstandard2.1 is our dependency on System.Reflection.Emit (which .netstandard2.0 never officially supported, but got to work nevertheless through some magic extra FCL NuGet package). Other than that, netstandard2.1 and netstandard2.0 are pretty much exchangeable for this project.

Ah yes, I had completely forgotten about that. Cheers.

as we'll drop .NET Standard 2.0 in a couple of years

"commented on 28 Apr 2020"... that's right now 😆

Since it now looks like we're going to drop support for ancient framework versions, perhaps we should drop netstandard2.0 instead, and keep netstandard2.1?

That'll drop support .NET Core 2.1 which we currently support through .NET Standard 1.x, but I'm good with that since it was end of life August 21, 2021. .NET Standard 2.1 supports .NET Core 3.1 which is still in support. And we support .NET Framework via the netfx target rather than .NET Standard anyway.

Sounds good to me. @Jevonius do you agree?

@Jevonius
Copy link
Contributor

Jevonius commented Apr 7, 2022

I'm a little wary of suggesting removing netstandard2.0, mainly because of projects that consume Castle and what they target; my perception is that most projects tend to target netstandard2.0 rather than netstandard2.1; the exception in the top 10 from https://www.nuget.org/stats/packages is AutoMapper (which only targets netstandard2.1).

Given the only difference I can see between Castle builds for 2.0 and 2.1 is the inclusion of the PackageReference for System.Reflection.Emit, it feels like dropping 2.0 is not really of much benefit from a maintenance perspective, but does penalise those stuck with older frameworks. Then again, given this is going to be a major version jump, perhaps now is the right time to drive things forward.

@stakx
Copy link
Member

stakx commented Apr 7, 2022

it feels like dropping 2.0 is not really of much benefit from a maintenance perspective, but does penalise those stuck with older frameworks

Despite my above post, and despite having argued against netstandard2.0 in the past, I tend to agree with this reasoning... if preserving the netstandard2.0 TFM doesn't overly complicate things in this project here.

For me, the more important thing would be to not drop netstandard2.1 in favor of netstandard2.0.

One argument against having too many TFMs was that we'd need to cover all of them in the CI test runs (thus making the CI setup more complex and slower). How about keeping netstandard2.0, but not running extra CI tests for it? A test configuration for netstandard2.1 should sufficiently cover the netstandard2.0 target, too. That would allow us to keep both of them (as is).

Then again, given this is going to be a major version jump, perhaps now is the right time to drive things forward.

Also agree with that one. I think it's fine either way, with or without netstandard2.0.

@jonorossi
Copy link
Member Author

Good reasoning @Jevonius.

I think to keep things moving we just take the path of least resistance and go with both (since we've already got them) and leave it running tests on both targets as the CI build is pretty quick (5 minutes) anyway. We might find by the time we get to v6 no one is using .NET Standard targets anymore 😉 .

So we do: <TargetFrameworks>net462;netstandard2.0;netstandard2.1;net6.0</TargetFrameworks>

@jonorossi
Copy link
Member Author

For the unit tests we've currently got:

<TargetFrameworks>net462;netcoreapp2.1;netcoreapp3.1</TargetFrameworks>

There is no point testing on end of life .NET runtimes, do we change this to:

<TargetFrameworks>net462;net6.0</TargetFrameworks>

@Jevonius
Copy link
Contributor

Jevonius commented Apr 7, 2022

3.1 is in support until December 2022, so I'd be tempted to say:

<TargetFrameworks>net462;netcoreapp3.1;net6.0</TargetFrameworks>

This then covers the netstandard2.X targeting too, which would otherwise be missed/excluded. As noted, the CI process time is negligible, so doesn't seem like there's any real advantage to excluding it.

@jonorossi
Copy link
Member Author

3.1 is in support until December 2022, so I'd be tempted to say:

Ah yer, 3.1 is an LTS release too. Yep go with your proposed list.

@Jevonius
Copy link
Contributor

Jevonius commented Apr 9, 2022

Have just opened a PR (#616) for adding net6.0; had a bit of a fight with the build pipelines and NUnit but have something that builds and passes tests now.

@Jevonius
Copy link
Contributor

Jevonius commented May 1, 2022

Coming back to dropping netcoreapp2.1 from the test side, if we do that the netstandard2.0 build won’t get tested. Arguably that’s already the case since 2.1 tests aren’t executed. Is that ok? Alternatively, @stakx if we were to drop netstandard2.1 as a target, the 3.1 tests would then consume the 2.0 packages, and everything would still be in support.

@stakx
Copy link
Member

stakx commented May 1, 2022

Alternatively, @stakx if we were to drop netstandard2.1 as a target

That would, IMHO, be the wrong move, as I commented elsewhere recently. If we decided to drop any of the netstandard2.x TFMs at all, it should be netstandard2.0 instead.

If it's important to you guys to set the test coverage issue aright, then by all means go ahead and remove netstandard2.0 (or alternatively set up a netcoreapp2.x test run). I personally certainly won't insist on it; these days, I care much more about finally seeing v5 released than I care about the perfect, squeaky clean project setup. :-)

@Havunen
Copy link

Havunen commented May 2, 2022

That would, IMHO, be the wrong move, as I commented elsewhere recently. If we decided to drop any of the netstandard2.x TFMs at all, it should be netstandard2.0 instead.

netstandard2.1 is not compatible with older xamarin and .NET framework, but as people are including .net45 for example I don't this think this really matters. https://dotnet.microsoft.com/en-us/platform/dotnet-standard#versions

Adding new target frameworks is not breaking change... so if anybody ever comes with an issue about those they can be added in 5.0.1 and so on... Just hurry up with the release of v5 please

Not having a modern target framework is bigger issue than having some old target framework as they can always be added later without breaking changes...

@jonorossi
Copy link
Member Author

Thanks to @Jevonius we've now got a .NET 6 target in master (#616).

Here are the final TFMs for Castle Core v5.0.0:

<!-- v4.4.1 -->
<TargetFrameworks>net35;net40;net45;netstandard1.3;netstandard1.5</TargetFrameworks>

<!-- master / v5.0.0 -->
<TargetFrameworks>net462;netstandard2.0;netstandard2.1;net6.0</TargetFrameworks>

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

No branches or pull requests

6 participants