Fix duplicate native DLLs in net4x output directory - #1886
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds conditional MSBuild .targets that remove package native assets from RuntimeCopyLocalItems for .NET Framework, packages those targets into NuGet builds for multiple TFMs, updates release/contributor documentation, and marks issues ChangesNative DLL Copy Prevention
Sequence Diagram(s)sequenceDiagram
participant ResolvePackageAssets
participant OpenCvSharp4RuntimeWin_RemoveRootNativeAssets
participant OpenCvSharp4RuntimeWinSlim_RemoveRootNativeAssets
participant RuntimeCopyLocalItems
ResolvePackageAssets->>OpenCvSharp4RuntimeWin_RemoveRootNativeAssets: AfterTargets invoke (net4*/v4*)
ResolvePackageAssets->>OpenCvSharp4RuntimeWinSlim_RemoveRootNativeAssets: AfterTargets invoke (net4*/v4*)
OpenCvSharp4RuntimeWin_RemoveRootNativeAssets->>RuntimeCopyLocalItems: remove OpenCvSharp4.runtime.win entries
OpenCvSharp4RuntimeWinSlim_RemoveRootNativeAssets->>RuntimeCopyLocalItems: remove OpenCvSharp4.runtime.win.slim entries
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/issue-backlog.md`:
- Line 22: Update the resolved checklist entry that currently references "`#1704`"
to the correct issue number "`#1765`" (the duplicate native DLLs fix) in the
docs/issue-backlog.md checklist item that reads "### [x] `#1704` — Assembly
version reported as `0.0.0.0`"; if you intentionally meant to mark `#1704` as
resolved, replace the checklist text to explain why `#1704` is closed by this PR
instead of changing the issue number.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20a3b379-448e-4d3e-b108-4fc19d76867e
📒 Files selected for processing (5)
docs/issue-backlog.mdnuget/OpenCvSharp4.runtime.win.csprojnuget/OpenCvSharp4.runtime.win.slim.csprojnuget/OpenCvSharp4.runtime.win.slim.targetsnuget/OpenCvSharp4.runtime.win.targets
When a net4x project references OpenCvSharp4.runtime.win via PackageReference, two separate mechanisms both copy the native DLLs to the build output: 1. The .props file (at build/netstandard/) copies them to dll/x64/, which is where WindowsLibraryLoader looks at runtime. 2. NuGet''s PackageReference pipeline also copies runtimes/win-x64/native/ assets to the output root, which are never loaded by OpenCvSharp. Add a .targets file for each runtime package (full and slim) that removes the package''s entries from RuntimeCopyLocalItems after ResolvePackageAssets runs, suppressing the redundant root copy for net4x targets. The dll/x64/ placement from the .props file is unaffected, so runtime behavior is unchanged. This fix applies only to SDK-style projects using PackageReference. Old-style packages.config projects do not process runtimes/ assets automatically, so there is no duplicate in that case and the target has no effect. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
docs/release-process.md: document the release workflow and assembly versioning policy (when to bump AssemblyVersion, how the NuGet version flows through CI, steps for upgrading OpenCV). CLAUDE.md: AI-facing instructions covering repository structure, the versioning convention, WindowsLibraryLoader behavior, and NuGet runtime package layout. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Merge the repository overview, versioning policy, WindowsLibraryLoader notes, and issue backlog pointer from CLAUDE.md into the existing .github/copilot-instructions.md to avoid maintaining two parallel files. Remove CLAUDE.md. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Claude Code reads CLAUDE.md automatically at session start. A one-line pointer avoids duplicating content while keeping .github/copilot-instructions.md as the single source of truth. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
a5685e7 to
dbed22b
Compare
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/release-process.md (1)
1-52:⚠️ Potential issue | 🟠 MajorFix UTF-8 with BOM encoding for docs/release-process.md
docs/release-process.mdis missing the UTF-8 BOM (file starts with23 20 52=# R), so it violates the UTF-8 BOM requirement for.mdfiles.- Re-save it as UTF-8 with BOM:
$enc = New-Object System.Text.UTF8Encoding $true $content = [System.IO.File]::ReadAllText("docs\release-process.md", [System.Text.Encoding]::UTF8) [System.IO.File]::WriteAllText("docs\release-process.md", $content, $enc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/release-process.md` around lines 1 - 52, docs/release-process.md is missing the UTF‑8 BOM; re-save that file with UTF‑8 BOM encoding so it meets the .md BOM requirement (e.g. use the provided PowerShell snippet that creates a UTF8Encoding(true) and writes the file back, or re-save via your editor/IDE with “UTF-8 with BOM”); ensure the file start bytes include the BOM (EF BB BF) before the existing content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 1: CLAUDE.md currently lacks the UTF‑8 BOM (it starts with the bytes for
"See"), so re-save the file using UTF‑8 with BOM encoding; open CLAUDE.md and
change the file encoding to "UTF-8 with BOM" (or run a script that reads the
file as UTF‑8 and writes it back using an UTF8Encoding instance with
emitBOM=true) so the file begins with EF BB BF while preserving the existing
content.
---
Outside diff comments:
In `@docs/release-process.md`:
- Around line 1-52: docs/release-process.md is missing the UTF‑8 BOM; re-save
that file with UTF‑8 BOM encoding so it meets the .md BOM requirement (e.g. use
the provided PowerShell snippet that creates a UTF8Encoding(true) and writes the
file back, or re-save via your editor/IDE with “UTF-8 with BOM”); ensure the
file start bytes include the BOM (EF BB BF) before the existing content.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad6b4999-0d29-41b3-9d2f-784d87b2ff0e
📒 Files selected for processing (8)
.github/copilot-instructions.mdCLAUDE.mddocs/issue-backlog.mddocs/release-process.mdnuget/OpenCvSharp4.runtime.win.csprojnuget/OpenCvSharp4.runtime.win.slim.csprojnuget/OpenCvSharp4.runtime.win.slim.targetsnuget/OpenCvSharp4.runtime.win.targets
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/issue-backlog.md
- nuget/OpenCvSharp4.runtime.win.slim.csproj
- nuget/OpenCvSharp4.runtime.win.slim.targets
- nuget/OpenCvSharp4.runtime.win.csproj
- nuget/OpenCvSharp4.runtime.win.targets
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Fixes #1765
Problem
When a .NET Framework (net4x) project references
OpenCvSharp4.runtime.winorOpenCvSharp4.runtime.win.slimviaPackageReference, two separate mechanisms both copy the native DLLs into the build output:.propsfile (imported viabuild/netstandard/) copies them todll/x64/, whereWindowsLibraryLoaderlooks at runtime.PackageReferencepipeline also copiesruntimes/win-x64/native/assets to the output root as part of its native asset resolution.The root copies are never loaded by OpenCvSharp and serve no purpose, but they inflate the output directory and can interfere with deployment tooling that packages everything in the output folder.
Fix
Add a
.targetsfile for each runtime package (OpenCvSharp4.runtime.winandOpenCvSharp4.runtime.win.slim) that runs afterResolvePackageAssetsand removes the package's entries fromRuntimeCopyLocalItemswhen targeting net4x. This suppresses the redundant root copy. Thedll/x64/placement from the.propsfile is unaffected, so runtime behavior is unchanged.This fix only applies to SDK-style projects using
PackageReference. Old-stylepackages.configprojects do not processruntimes/assets automatically and have no duplicate in the first place, so the target is a no-op for them.Summary by CodeRabbit
Bug Fixes
Chores
Documentation