Fix and simplify the native library build - #351
Open
RagingKore wants to merge 3 commits into
Open
Conversation
A clean clone needed two builds. The first downloaded the libraries and copied none of them; a second build then picked them up. Three separate defects produced that. The download guard was the existence of obj/runtimes/<rid>/native. It recorded no version, so raising the DuckDB release left every machine that had built once on the old engine, and a checkout targeting v1.5.5 kept running v1.2.0 and failed 127 tests with EntryPointNotFoundException. MakeDir also ran before DownloadFile, so a download that failed left an empty directory that satisfied the guard and every later build skipped in silence. The libraries were staged under obj, which the SDK lists in DefaultItemExcludes. Every item glob over them matched nothing and reported nothing, so the files reached no output directory and a cold dotnet pack shipped DuckDB.NET.Bindings.Full with no native library in it. The download now runs during restore, which is the only phase that precedes evaluation, where item globs expand. The hook is a NuGet target rather than the public Restore target, because Restore only runs on the project the command names and a solution build would skip every project inside it. It carries no compatibility guarantee, so if a later SDK renames it the symptom is a missing library on a clean clone. Staging moves to artifacts/natives/<version>/<rid>/native. That is outside obj, so globs see it, and the version in the path makes the directory the cache key: a bump lands in a fresh folder and cannot inherit the previous engine. Incrementality follows the SDK cache file convention, where a value rather than a source file drives the decision, as CreateGeneratedAssemblyInfoInputsCacheFile does in Microsoft.NET.GenerateAssemblyInfo.targets. The download cache file is written last, so a failure leaves it older than the inputs and the next build retries. The fetch also completes before the installed payload is cleared, so a download that fails over a dead network leaves the working engine in place. Bindings declares the libraries as Content in the project body. Content flows across a ProjectReference, so the test, benchmark and sample projects each get the engine from the reference they already had and their own globs are gone. Copying is unconditional because every project that runs needs the engine beside its assembly; only packing stays gated on BuildType. DuckDbVersion moves to Directory.Build.props and drives the artifact URL, the staging path and both packages' release notes, which each carried the version as hand typed prose. The release and nightly download targets differed in one step and are now one target. DuckDbArtifactRoot still wins when set, which the nightly CI job does through the environment.
A NuGet package writes a runtimeTargets entry into deps.json and the host then
probes runtimes/{rid}/native on its own. A project reference produces no such
entry, so every project in this repository loaded the library itself: the test
project through a module initializer, the benchmarks through a second copy of
the same code, and the samples by compiling the test project's file through a
Compile Include that reached across projects.
DuckDBNativeLibrary registers a DllImportResolver for the Bindings assembly,
which is where DuckDbLibrary and its imports live. One copy of the runtime
identifier logic now serves every consumer, in this repository and downstream.
The resolver returns zero for a name it does not own and for a file it cannot
find, so default probing still applies and a library already present on the
system still loads. It passes a relative path with
DllImportSearchPath.AssemblyDirectory, which keeps it working under single file
publish where Assembly.Location is empty, and the four argument overload applies
the platform naming convention so one name matches libduckdb.dylib,
libduckdb.so and duckdb.dll.
Raising the version used to touch three files and nothing checked that they agreed. Commit fd45dbb updated the artifact URL and left both packages' release notes on the previous version. It is one property now, so say where it is.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #351 +/- ##
===========================================
- Coverage 86.56% 86.32% -0.25%
===========================================
Files 78 79 +1
Lines 3492 3510 +18
Branches 548 557 +9
===========================================
+ Hits 3023 3030 +7
- Misses 331 337 +6
- Partials 138 143 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Coverage Report for CI Build 33379933153Coverage decreased (-0.1%) to 88.181%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
🟡 Changes recommended
Windows ARM64 resolution is incorrect, and mutable nightly artifacts can remain cached indefinitely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes native DuckDB download, caching, propagation, and runtime resolution in the bindings project.
Changes:
- Adds version-aware native staging and packaging.
- Introduces an assembly-level native resolver.
- Removes duplicated consumer loaders and build targets.
File summaries
| File | Description |
|---|---|
Directory.Build.props |
Centralizes version and staging properties. |
README.md |
Documents version updates. |
DuckDB.NET.Bindings/Bindings.csproj |
Integrates restore-time downloads and native content. |
DuckDB.NET.Bindings/DownloadNativeLibs.targets |
Implements download caching and extraction. |
DuckDB.NET.Bindings/DuckDBNativeLibrary.cs |
Adds centralized runtime resolution. |
DuckDB.NET.Bindings/NativeMethods/NativeMethods.All.cs |
Exposes the native library name internally. |
DuckDB.NET.Data/Data.csproj |
Uses the centralized version in release notes. |
DuckDB.NET.Test/Test.csproj |
Removes duplicated native staging. |
DuckDB.NET.Test/ModuleInit.cs |
Removes test-local loading. |
DuckDB.NET.Test/Helpers/NativeLibraryHelper.cs |
Deletes the obsolete test loader. |
DuckDB.NET.Samples/Samples.csproj |
Removes linked loader source. |
DuckDB.NET.Samples/Program.cs |
Relies on centralized resolution. |
DuckDB.NET.Benchmarks/Benchmarks.csproj |
Removes duplicated native staging. |
DuckDB.NET.Benchmarks/NativeLibraryLoader.cs |
Deletes the obsolete benchmark loader. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+57
to
+60
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| return Environment.Is64BitProcess ? "win-x64" : "win-x86"; | ||
| } |
Comment on lines
+29
to
+32
| <WriteLinesToFile Lines="$(LibUrl)" | ||
| File="$(NativeInputsCacheFile)" | ||
| Overwrite="true" | ||
| WriteOnlyWhenDifferent="true" /> |
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
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.
TL;DR
git clonethendotnet testnow works. Before, it took two builds, and the second one only worked by accident.Using DuckDB from a project in this repository used to mean copying a large MSBuild target and glob block into it, plus a loader class to find the library at runtime. Three projects each carried their own copy, and one of them compiled the test project's source file to get it. The DuckDB version was hard coded in three separate places.
Now a project adds a
ProjectReferencetoDuckDB.NET.Bindingsand nothing else. The version lives in one property inDirectory.Build.props, and the library finds itself through aDllImportResolverin the assembly that declares the imports, which is the mechanism the platform provides for this.What this fixes
EntryPointNotFoundException. CI clones fresh, so CI never saw it.MakeDirran beforeDownloadFile. One failure left an empty directory that satisfied the guard, and every later build skipped in silence.obj, which the SDK lists inDefaultItemExcludes. Every glob over them matched nothing and said nothing about it.dotnet packshipped a broken package. On a clean clone,DuckDB.NET.Bindings.Fullpacked with zero native libraries after downloading all five. CI escaped it only because it builds before it packs.DuckDB.NET.Samplesnever worked on its own. It had no glob at all and depended on another project having been built first.Compile Includein the samples that reached across into the test project.What a consumer needed before
BeforeTargets="GenerateAdditionalSources"names a target that does not exist during restore or in the outer build. MSBuild reporteddoes not exist in the project, and will be ignoredand dropped the hook.How it works now
The download runs during restore, the only phase that precedes evaluation. Item globs expand during evaluation, so anything downloaded later is invisible to them.
The hook is
_GenerateRestoreGraphProjectEntry, not the publicRestoretarget.Restoreruns only on the project the command names, so a solution build skips every project inside it. The NuGet target runs once per project either way. It carries no compatibility guarantee: if a later SDK renames it, the symptom is a missing library on a clean clone.Staging moves to
artifacts/natives/<version>/<rid>/native, outsideobjso globs see it. The version in the path makes the directory the cache key, so a bump lands in a fresh folder and cannot inherit the previous engine. Incrementality follows the SDK cache file convention, where a value rather than a source file drives the decision, asCreateGeneratedAssemblyInfoInputsCacheFiledoes inMicrosoft.NET.GenerateAssemblyInfo.targets. The cache file is written last, so a failure leaves it stale and the next build retries. The fetch also completes before the installed payload is cleared, so a download that fails over a dead network leaves the working engine in place.Bindings.csprojdeclares the libraries asContentin the project body.Contentflows across aProjectReference, so consumers inherit the engine from the reference they already had.DuckDBNativeLibraryregisters aDllImportResolverfor the bindings assembly. A NuGet package writes aruntimeTargetsentry intodeps.jsonand the host probesruntimes/{rid}/nativeon its own; a project reference produces no such entry, which is why each project had its own loader. The resolver returns zero for a name it does not own and for a file it cannot find, so default probing still applies and a library already on the system still loads. Package consumers see no change: with and without the resolver, aPackageReferenceapp resolves throughdeps.jsonas before.Verification
dotnet testdotnet packwithBuildType=Fulldotnet packwithoutBuildTypeNativeMethodsP/Invoke sample and a 100000 row appender loaddotnet restore, thendotnet build --no-restoreDuckDbVersionto 1.4.1, then buildNo CI workflow changes. Every command in
ci.yml,Sonar.ymlandcodeql-analysis.ymlwas run against this branch.