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

Update target frameworks for .NET 6, .NET Framework 4.6.2, .NET Standard 2.0 #710

Merged
merged 10 commits into from Jan 25, 2023

Conversation

Romfos
Copy link
Contributor

@Romfos Romfos commented Jan 16, 2023

What's changed?

  • Target frameworks now: .NET 6, .NET Framework 4.6.2, .NET Standard 2.0
  • Update some nuget packages

Why target frameworks were changed?

Microsoft recommend to target .NET 6, .NET Standard and minimal supported .NET Framework
https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/older-framework-versions-dropped
Castle.Core now have similar list of supported platforms
https://www.nuget.org/packages/castle.core/#dependencies-body-tab

Notes:
.NET Framework 4.6.1 is out of support, .NET Framework 4.6.2 is the older version that is still supported
https://devblogs.microsoft.com/dotnet/net-framework-4-5-2-4-6-4-6-1-will-reach-end-of-support-on-april-26-2022/

.NET 5 is out of support
https://devblogs.microsoft.com/dotnet/dotnet-5-end-of-support-update/

.NET Core 3.1 is out of support
https://devblogs.microsoft.com/dotnet/net-core-3-1-will-reach-end-of-support-on-december-13-2022/

@Romfos
Copy link
Contributor Author

Romfos commented Jan 16, 2023

Maybe someone can assis with build.fsx ?

@Romfos
Copy link
Contributor Author

Romfos commented Jan 17, 2023

looks like fake-cli do not work as expected with dotnet 7+

Copy link
Member

@alexandrnikitin alexandrnikitin left a comment

Choose a reason for hiding this comment

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

@Romfos Thank you for the PR.

Re versions: Keeping them as they were is the safe option. -* includes beta versions and looks too permissive for me, docs. IMO MAJOR.* is a better choice but I'm fine with either.

Re: build issue. I also get that error in my local environment. Not sure what's wrong with it.
There is an issue on FAKE repo fsprojects/FAKE#2722
I tried workarounds from there but no luck.
The last successful build on appveyor used FAKE 5.23.1 and .NET 6.0.400. Tried these versions in my local env but it fails too.

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3' or '$(TargetIsNetFx)' == 'true'">
<PackageReference Include="Castle.Core" Version="4.4.1-*" />
<ItemGroup>
<PackageReference Include="Castle.Core" Version="5.1.1" />
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the PackageReference version ranges as they were. It was 5.0.0-* for Castle.Core.

Copy link
Contributor Author

@Romfos Romfos Jan 18, 2023

Choose a reason for hiding this comment

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

I restored 5.0.0-*, but what is wrong with it? why do not use patched versions?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! My point is to use floating/ range versions vs direct versions (5.1.1).
As for 5.0.0-* vs 5.1.1-*, dependencies are "flattened" and can be managed and upgraded by Application/NuGet directly. AFAIK NSubstitute works well with 5.0.0. I'm not aware of a strong reason to limit the lower boundary. Maybe I'm missing something.

In general, the floating to direct version change and mismatch of direct versions can potentially break an application.

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0' or '$(TargetIsNet5OrNewer)' == 'true'">
<PackageReference Include="Castle.Core" Version="5.0.0-*" />
<ItemGroup Condition="'$(TargetIsNet5OrNewer)' != 'true'">
<PackageReference Include="System.Threading.Tasks.Extensions" Version="4.3.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Same here, it was 4.3.0-*

@alexandrnikitin
Copy link
Member

@Romfos The build has been fixed via #711

@Romfos
Copy link
Contributor Author

Romfos commented Jan 24, 2023

pr was updated

@alexandrnikitin
Copy link
Member

LGTM. It makes sense to align it with Castle.Core and remove out of support versions. Removing netstandard1.3 and net45 looks like a breaking change for me though.

@dtchepak What do you think about the PR? Do we want to include anything else in the release? Any pending breaking changes? We probably want to sync it with nsubstitute/NSubstitute.Analyzers#198 cc @tpodolak

@dtchepak
Copy link
Member

@dtchepak What do you think about the PR? Do we want to include anything else in the release? Any pending breaking changes? We probably want to sync it with nsubstitute/NSubstitute.Analyzers#198 cc @tpodolak

I'm happy to release as-is. If you're happy could you please merge and I'll release?

I still need to get to #700 (I want to do some more testing of that and update docs before releasing), but I can rebase that and put that in a later release.

@@ -1,3 +1,6 @@
4.5.0 Release
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
4.5.0 Release
5.0.0 Release

CHANGELOG.md Outdated
@@ -1,3 +1,6 @@
### 4.5.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### 4.5.0
### 5.0.0 (Pending)

Copy link
Member

Choose a reason for hiding this comment

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

Let's just bump the Major version. @Romfos

@Romfos
Copy link
Contributor Author

Romfos commented Jan 25, 2023

All comments are fixed

@alexandrnikitin alexandrnikitin merged commit 1affd2d into nsubstitute:main Jan 25, 2023
@alexandrnikitin
Copy link
Member

@Romfos Merged. Thank you for the PR! We appreciate it.

@tpodolak
Copy link
Member

We probably want to sync it with nsubstitute/NSubstitute.Analyzers#198 cc @tpodolak

I still need some time to do a proper testing of TargetFramework bump in analyzers, but IMO you dont need to wait for me as NSubstitute changes wont affect analyzers

@dtchepak
Copy link
Member

dtchepak commented Feb 5, 2023

@alexandrnikitin Thanks for merging this. Happy for me to release this now?

@alexandrnikitin
Copy link
Member

@dtchepak Yes, when you are ready. Thank you!

@dtchepak
Copy link
Member

Sorry for the delay. I've pushed 5.0.0 to nuget. Please give it a try and let me know ASAP if there are any problems. 🙇

Thanks very much for the contribution!

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.

None yet

4 participants