Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR removes .NET Framework 4.8 support and related NETFRAMEWORK conditionals, consolidates targets to .NET Standard 2.0/2.1 and .NET 8.0, drops x86 Windows native packaging, simplifies platform/PInvoke logic, updates CI/workflows and packaging, and removes or adjusts framework-specific tests and serialization code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenCvSharp/Fundamentals/OpenCVException.cs (1)
6-8:⚠️ Potential issue | 🟠 Major
[Serializable]now advertises a contract that was removed.At line 6, the class is marked
[Serializable], but lacks the required serialization constructor(SerializationInfo, StreamingContext)andGetObjectData()method. The custom properties (Status,FuncName,ErrMsg,FileName,Line) will not be serialized with binary serialization, causing data loss for consumers still usingBinaryFormatteror.NET Frameworkwithnetstandard2.0. Either remove the[Serializable]attribute or restore serialization members conditionally where compatibility is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenCvSharp/Fundamentals/OpenCVException.cs` around lines 6 - 8, The OpenCVException class is marked [Serializable] but lacks the required serialization constructor and GetObjectData, so its custom properties (Status, FuncName, ErrMsg, FileName, Line) won't be preserved; either remove the [Serializable] attribute from class OpenCVException, or restore full binary-serialization support by adding the protected constructor OpenCVException(SerializationInfo info, StreamingContext context) that reads those properties and override GetObjectData(SerializationInfo info, StreamingContext context) to write them, and if you need conditional compilation for modern runtimes wrap the serialization members with the appropriate target-framework `#if` (e.g., NETSTANDARD2_0 || NETFRAMEWORK) guards so the attribute and methods are present only where binary serialization compatibility is required.
🧹 Nitpick comments (1)
src/OpenCvSharp/Modules/core/Struct/Vec/Vec6w.cs (1)
7-9: Pre-existing documentation typo: "4-Tuple" should be "6-Tuple".The XML doc comment on line 8 says "4-Tuple of ushort" but this struct is
Vec6w, a 6-tuple. This is not introduced by this PR, but worth fixing while you're here.📝 Suggested fix
/// <summary> -/// 4-Tuple of ushort (System.UInt16) +/// 6-Tuple of ushort (System.UInt16) /// </summary>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenCvSharp/Modules/core/Struct/Vec/Vec6w.cs` around lines 7 - 9, Update the XML summary for the Vec6w struct: change the incorrect "4-Tuple of ushort (System.UInt16)" to "6-Tuple of ushort (System.UInt16)" in the Vec6w.cs file so the documentation matches the struct name Vec6w and its semantics; locate the summary tag above the Vec6w struct declaration and correct the tuple size text accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/windows.yml:
- Around line 182-188: CI currently only runs tests for net8.0-windows leaving
the declared net48 target unvalidated; add a Windows workflow step that builds
and runs the net48 target for the OpenCvSharp.WpfExtensions/tests. Modify the
"Test Windows-only functions" section (or add a new step near it) to run dotnet
restore/build/test for the net48 target (use -f net48 when invoking dotnet test
or dotnet build/test for the OpenCvSharp.Tests.Windows project) so both
net8.0-windows and net48 TFMs are exercised; ensure the step runs on the same
Windows runner and the workspace path (test/OpenCvSharp.Tests.Windows) is used
just like the existing test step.
In `@test/OpenCvSharp.Tests/FileDownloader.cs`:
- Around line 16-20: The response handling in FileDownloader should validate
status and dispose the response content stream: after obtaining httpResponse
from httpClient.Send (and before reading content) call
httpResponse.EnsureSuccessStatusCode() to throw on non-2xx, then obtain the
content stream via httpResponse.Content.ReadAsStream() inside a using block and
CopyTo the MemoryStream; keep using var memoryStream = new MemoryStream() and
return memoryStream.ToArray() but ensure the content stream is disposed by the
using around ReadAsStream() and that EnsureSuccessStatusCode() is invoked on
httpResponse.
---
Outside diff comments:
In `@src/OpenCvSharp/Fundamentals/OpenCVException.cs`:
- Around line 6-8: The OpenCVException class is marked [Serializable] but lacks
the required serialization constructor and GetObjectData, so its custom
properties (Status, FuncName, ErrMsg, FileName, Line) won't be preserved; either
remove the [Serializable] attribute from class OpenCVException, or restore full
binary-serialization support by adding the protected constructor
OpenCVException(SerializationInfo info, StreamingContext context) that reads
those properties and override GetObjectData(SerializationInfo info,
StreamingContext context) to write them, and if you need conditional compilation
for modern runtimes wrap the serialization members with the appropriate
target-framework `#if` (e.g., NETSTANDARD2_0 || NETFRAMEWORK) guards so the
attribute and methods are present only where binary serialization compatibility
is required.
---
Nitpick comments:
In `@src/OpenCvSharp/Modules/core/Struct/Vec/Vec6w.cs`:
- Around line 7-9: Update the XML summary for the Vec6w struct: change the
incorrect "4-Tuple of ushort (System.UInt16)" to "6-Tuple of ushort
(System.UInt16)" in the Vec6w.cs file so the documentation matches the struct
name Vec6w and its semantics; locate the summary tag above the Vec6w struct
declaration and correct the tuple size text accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
.github/copilot-instructions.md.github/workflows/windows.yml.vscode/settings.jsonREADME.mdnuget/OpenCvSharp4.runtime.win.csprojnuget/OpenCvSharp4.runtime.win.slim.csprojsrc/OpenCvSharp.Extensions/OpenCvSharp.Extensions.csprojsrc/OpenCvSharp/Fundamentals/OpenCVException.cssrc/OpenCvSharp/Fundamentals/OpenCvSharpException.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/NativeMethods.cssrc/OpenCvSharp/Internal/PInvoke/Win32API.cssrc/OpenCvSharp/Internal/PInvoke/WindowsLibraryLoader.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec2b.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec2d.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec2f.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec2i.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec2s.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec2w.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec3b.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec3d.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec3f.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec3i.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec3s.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec3w.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec4b.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec4d.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec4f.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec4i.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec4s.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec4w.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec6b.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec6d.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec6f.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec6i.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec6s.cssrc/OpenCvSharp/Modules/core/Struct/Vec/Vec6w.cssrc/OpenCvSharp/OpenCvSharp.csprojtest/OpenCvSharp.Tests.Windows/OpenCvSharp.Tests.Windows.csprojtest/OpenCvSharp.Tests/FileDownloader.cstest/OpenCvSharp.Tests/OpenCvSharp.Tests.csprojtest/OpenCvSharp.Tests/imgcodecs/ImgCodecsTest.cstest/OpenCvSharp.Tests/system/AppDomainTest.cstest/OpenCvSharp.Tests/system/WindowsLibraryLoaderTest.cstest/OpenCvSharp.Tests/videoio/VideoCaptureTest.cstest/OpenCvSharp.Tests/videoio/VideoWriterTest.cstest/OpenCvSharp.Tests/xphoto/XPhotoTest.cstool/OpenCvSharp.ReleaseMaker/Packer.cs
💤 Files with no reviewable changes (5)
- test/OpenCvSharp.Tests/videoio/VideoWriterTest.cs
- tool/OpenCvSharp.ReleaseMaker/Packer.cs
- test/OpenCvSharp.Tests/videoio/VideoCaptureTest.cs
- src/OpenCvSharp/Internal/PInvoke/Win32API.cs
- test/OpenCvSharp.Tests/system/AppDomainTest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drop
.NET Framework 4.8from the explicitTargetFrameworkslist inOpenCvSharp,OpenCvSharp.Extensions, and the test/nuget projects. The library continues to be usable from .NET Framework 4.6.1+ via thenetstandard2.0target, which NuGet resolves automatically.OpenCvSharp.WpfExtensionsretainsnet48for now because WPF depends on framework assemblies (PresentationCoreetc.) that cannot be expressed through anetstandardtarget.New target frameworks
OpenCvSharpnetstandard2.0; netstandard2.1; net48; net8.0netstandard2.0; netstandard2.1; net8.0OpenCvSharp.Extensionsnet48; netstandard2.0; netstandard2.1; net8.0netstandard2.0; netstandard2.1; net8.0OpenCvSharp.WpfExtensionsnet48; net8.0-windowsnet48net48removedCode changes
Removed
#if DOTNET_FRAMEWORK/#if NET48/#if NETFRAMEWORKbranchesNativeMethods.cs– removedSuppressUnmanagedCodeSecurity,SecurityPermissionattribute, andEnvironment.OSVersion.Platform-based OS detectionWindowsLibraryLoader.cs– removedAppDomain.CurrentDomain.BaseDirectoryfallback,System.Web.HttpContext.CurrentASP.NET hack, andAssembly.GetExecutingAssembly()branch;IsDotNetCore()now always returns the runtime-detection pathWin32API.cs–CharSetconstant simplified toCharSet.UnicodeunconditionallyOpenCVException.cs/OpenCvSharpException.cs– removedBinaryFormatterserialization constructor andGetObjectDataoverride; removed now-unusedusing System.Runtime.SerializationSimplified
#if DOTNET_FRAMEWORK || NETSTANDARD2_0→#if NETSTANDARD2_0Vec*.csstruct files (GetHashCodefallback for missingHashCode.Combine)NativeMethods.cs–StringUnmanagedTypeNotWindowsconstant (UnmanagedType.LPStrfallback, still needed fornetstandard2.0)Test cleanup
AppDomainTest.cs– deleted (entire file was#if NET48;AppDomain.CreateDomainis not supported on .NET Core)FileDownloader.cs– collapsed to theHttpClientbranchImgCodecsTest.cs– useAsSpan()[100..^100]andFile.Move(..., true)unconditionallyXPhotoTest.cs– removedWinForms-basedOpenFileDialogsample testVideoCaptureTest.cs,VideoWriterTest.cs– removed#if NETFRAMEWORKclass guards (tests now always compile and run)WindowsLibraryLoaderTest.cs– removedNET48branch; assert alwaystrueCI / tooling
windows.yml– changed test steps from-f net48to-f net8.0/-f net8.0-windowsPacker.cs(ReleaseMaker) – removednet48entry fromdllFilesdictionary and XML file listnuget/OpenCvSharp4.runtime.win*.csproj– removedbuild/net48package path entryDocumentation
README.md– updated supported framework description; .NET Framework users are directed to use thenetstandard2.0pathImpact on existing consumers
netstandard2.0automaticallyOpenCvSharp4.WpfExtensionsWpfExtensionsstill targetsnet48directlyOpenCvSharp4.Windowsmeta-packagenet48as a TFM so WpfExtensions is pulled in transitivelyBreaking changes
None for end users. The
netstandard2.0binary is functionally equivalent to what thenet48binary provided (minus legacy serialization constructors that were only meaningful withBinaryFormatter, which is itself removed in .NET 9+).Summary by CodeRabbit
Documentation
Refactor
Tests