Added Net7.0 / Net 6.0 / Net 5.0 and NetStandard2.1#1227
Added Net7.0 / Net 6.0 / Net 5.0 and NetStandard2.1#1227adamhathcock merged 5 commits intomasterfrom
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (15 files)
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for additional .NET target frameworks to the SharpCompress library: .NET 5.0, .NET 6.0, .NET 7.0, and .NET Standard 2.1. The changes enable the library to be consumed by projects targeting these intermediate framework versions, improving compatibility and allowing consumers to benefit from framework-specific optimizations.
Changes:
- Added net5.0, net6.0, net7.0, and netstandard2.1 to target frameworks
- Updated conditional compilation directives to enable framework-specific APIs for new targets
- Added polyfill attributes (MemberNotNullAttribute, IsExternalInit) for older frameworks
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/SharpCompress/SharpCompress.csproj | Added net5.0, net6.0, net7.0, netstandard2.1 to TargetFrameworks; updated LEGACY_DOTNET constant to include netstandard2.1; updated package description (missing NET 7.0) |
| src/SharpCompress/packages.lock.json | Added package dependencies for new target frameworks; updated ILLink.Tasks versions for net8.0 and net10.0 |
| tests/SharpCompress.Test/packages.lock.json | Added platform-specific dependencies for .NET Framework 4.8 and .NET 10.0 on win-x86 |
| src/SharpCompress/Polyfills/StreamExtensions.cs | Changed NET8_0_OR_GREATER to NET6_0_OR_GREATER for CopyToAsync with CancellationToken |
| src/SharpCompress/Compressors/ZStandard/*.cs | Updated conditional compilation to support Span overloads in netstandard2.1 using (!LEGACY_DOTNET || NETSTANDARD2_1) pattern |
| src/SharpCompress/Compressors/PPMd/PpmdStream.cs | Updated DisposeAsync conditional to exclude netstandard2.1 from legacy fallback |
| src/SharpCompress/Common/Tar/Headers/*.cs | Changed NET8_0_OR_GREATER to NET6_0_OR_GREATER for Encoding.GetByteCount(ReadOnlySpan) |
| src/SharpCompress/Common/Rar/Headers/MarkHeader.Async.cs | Updated DisposeAsync conditional to exclude netstandard2.1 from legacy pattern |
| src/SharpCompress/Common/Rar/CryptKey5.cs | Added NET5_0 to legacy condition for HMACSHA256 API (HashData added in .NET 6) |
| src/SharpCompress/Common/MemberNotNullAttribute.cs | Added polyfill attribute for netstandard2.1 and legacy frameworks |
| src/SharpCompress/Common/IsExternalInit.cs | Added netstandard2.1 to conditional compilation for init-only property support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Oops Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/SharpCompress/Common/Rar/CryptKey5.cs:43
- The conditional compilation directive includes NET5_0 in the legacy path, but HMACSHA256.HashData is only available starting from .NET 6.0. .NET 5.0 does not have this static method and should use the HMAC instance-based approach like LEGACY_DOTNET. The condition should be changed to exclude only .NET 6.0 and later from the legacy path.
#if LEGACY_DOTNET || NET5_0
using var hmac = new HMACSHA256(passwordBytes);
var block = hmac.ComputeHash(salt);
#else
var block = HMACSHA256.HashData(passwordBytes, salt);
#endif
src/SharpCompress/Common/Rar/CryptKey5.cs:57
- The conditional compilation directive includes NET5_0 in the legacy path, but HMACSHA256.HashData is only available starting from .NET 6.0. .NET 5.0 does not have this static method and should use the HMAC instance-based approach like LEGACY_DOTNET. The condition should be changed to exclude only .NET 6.0 and later from the legacy path.
#if LEGACY_DOTNET || NET5_0
block = hmac.ComputeHash(block);
#else
block = HMACSHA256.HashData(passwordBytes, block);
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net8.0' Or '$(TargetFramework)' == 'net9.0' Or '$(TargetFramework)' == 'net10.0' "> | ||
| <IsTrimmable>true</IsTrimmable> | ||
| <IsAotCompatible>true</IsAotCompatible> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
The IsTrimmable and IsAotCompatible properties are only set for net8.0 and net10.0, but net9.0 is also added as a target framework. Consider adding net9.0 to this condition if trimming and AOT compatibility should also apply to net9.0 builds.
| <AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net48' Or '$(TargetFramework)' == 'netstandard2.0' "> | ||
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net48' Or '$(TargetFramework)' == 'netstandard2.0' Or '$(TargetFramework)' == 'netstandard2.1' "> |
There was a problem hiding this comment.
.NET Standard 2.1 is being marked as LEGACY_DOTNET, which may cause confusion since .NET Standard 2.1 has Span and other modern APIs. The code already handles this with conditional checks like !LEGACY_DOTNET || NETSTANDARD2_1 throughout the codebase, but this creates complexity. Consider whether netstandard2.1 should be excluded from LEGACY_DOTNET definition, or alternatively, verify that all LEGACY_DOTNET usages correctly account for netstandard2.1's capabilities with explicit NETSTANDARD2_1 checks where needed.
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net48' Or '$(TargetFramework)' == 'netstandard2.0' Or '$(TargetFramework)' == 'netstandard2.1' "> | |
| <PropertyGroup Condition=" '$(TargetFramework)' == 'net48' Or '$(TargetFramework)' == 'netstandard2.0' "> |
Thanks! Gotta quiz why these frameworks should be supported. The dotnet ones are out of support and netstandard 2.1 seems redundant now. I get there's little to do but still work to support. I'd not do 4.8 but had requests and netstandard 2.0 seemed easy to do. |
|
People still run older (out of support) operating systems too. I always think it's worth supporting them if it's easy, especially with such a key 100% managed package. I understand where you're coming from though. It's fine if you don't want to merge it too :-). I just noticed the code was already 99% compliant. |
|
I'll support it for now. I just don't want things to get in my way more than they already are! Though, I suppose supporting net4.8 or net standard 2.0 is the worst |
|
Having NetStandardX.X covers most legacy requirements to be fair. I will admit to getting carried away :). |
Minimal change, thought I might as well. Great progress recently. Nice one.