refactor!: unbundle lora_ffi natives from LoraDb.Client (breaking packaging change) - #48
Draft
HarryCordewener wants to merge 1 commit into
Draft
refactor!: unbundle lora_ffi natives from LoraDb.Client (breaking packaging change)#48HarryCordewener wants to merge 1 commit into
HarryCordewener wants to merge 1 commit into
Conversation
LoraDb.Client packed the same five lora_ffi binaries as LoraDb.Client.Native (since ccefc93), duplicating that package wholesale and contradicting its own description ("Install alongside LoraDb.Client to enable the embedded transport"). It was also the only reason LoraDb.Client could not be plain MIT. The binaries now ship only in LoraDb.Client.Native, which keeps the BUSL-1.1 PackageLicenseFile treatment. LoraDb.Client returns to PackageLicenseExpression=MIT and drops PACKAGE-LICENSE.md, THIRD-PARTY-NOTICES.md and PackageRequireLicenseAcceptance — none of which apply to a package that ships no BSL content. It also drops from ~15 MB to ~71 KB. Nothing about the resolver changes. LoraDbNativeLoader.cs is still source-linked into LoraDb.Client (MIT source, not a binary reference) so its module initializer registers the DllImport hook when LoraDb.Client loads; the hook looks under runtimes/{rid}/native/, which is exactly where the LoraDb.Client.Native package places its assets. Dependency direction is unchanged and acyclic: LoraDb.Client.Native -> LoraDb.Client. BREAKING CHANGE: consumers who install only LoraDb.Client and use the embedded transport must now also install LoraDb.Client.Native. HTTP mode is unaffected. Such consumers previously got a bare DllNotFoundException; the bridge now rethrows one naming the companion package, but only when the default library name is in use — a caller who supplied their own name is not missing the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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.
What
LoraDb.Client.csprojpacked the same fivelora_ffibinaries asLoraDb.Client.Native(added inccefc93). That duplicated the.Nativepackage wholesale, contradicted its own description ("Install alongsideLoraDb.Clientto enable the embedded (in-process) mode…"), and was the only reasonLoraDb.Clientcould not be plain MIT.This PR removes the native
<None … Pack="true">block fromLoraDb.Clientand restores<PackageLicenseExpression>MIT</PackageLicenseExpression>, dropping itsPackageLicenseFile,PackageRequireLicenseAcceptance, and its packing ofPACKAGE-LICENSE.md/THIRD-PARTY-NOTICES.md— none of which apply to a package that ships no BSL content.LoraDb.Client.Nativeis untouched from what #47 left it as: it still ships all five binaries, the BUSL-1.1PackageLicenseFile, andTHIRD-PARTY-NOTICES.md.Side effect:
LoraDb.Client.nupkgdrops from ~15 MB to ~71 KB.A consumer who installs only
LoraDb.Clientand uses the embedded transport will break. They must now alsodotnet add package LoraDb.Client.Native.LoraDb.Clientalone remains sufficient.DllNotFoundExceptionfromNativeLibrary.Load.PInvokeLoraDbNativeBridgenow rethrows one that names the companion package — but only when the default library name (lora_ffi) is in use, since a caller who supplied their own name/path is not missing the package. Two unit tests cover both sides of that branch.nuget-publish.yml. Note thatv0.2shipped ~an hour ago with the natives insideLoraDb.Clientand withrequireLicenseAcceptance, so this removes assets from a version already on nuget.org. That argues for a clearly-flagged bump with release notes (and, if you want the two packages to stay pinnable together, releasing both at the same tag as usual). Your call — I published/unlisted nothing.Why this is safe (resolver mechanics)
The custom resolver keeps working because nothing about it changes:
LoraDbNativeLoader.csis source-linked intoLoraDb.Client(<Compile Include="..\LoraDb.Client.Native\LoraDbNativeLoader.cs">), not binary-referenced. It is MIT source, so it stays. Its[ModuleInitializer]therefore fires whenLoraDb.Clientitself loads and setsPInvokeLoraDbNativeBridge.LibraryPathResolver.runtimes/{rid}/native/next to the assembly, then underAppContext.BaseDirectory— exactly where NuGet lays out theLoraDb.Client.Nativepackage's RID assets in an app's output directory. It never cared which package put them there.LoraDb.Client.Native→ProjectReference/<dependency>→LoraDb.Client.LoraDb.Clienthas no reference back, so it cannot pull the natives in transitively (confirmed in the nuspec below).LoraDb.Clientpacking the natives:Pack="true"never copied them to build output anyway. The FFI integration tests take an absolute path fromLORADB_FFI_LIBRARY_PATH, whichci.ymlpoints at the checked-inLoraDb.Client.Native/runtimes/linux-x64/native/liblora_ffi.so. No workflow change needed.Docs corrected
Statements added in #47 that said both packages bundle the binaries are no longer true — corrected in
README.md,LICENSE,PACKAGE-LICENSE.md, and theDirectory.Build.propscomment.README.mdanddocs/USAGE.mdnow document that embedded mode requires the companion package.Verification
dotnet build -c Release+ unit tests (272 passed)270 before, 272 now — the two added are the new diagnostic-message tests.
LoraDb.Client.nupkg— noruntimes/*/native/*, license expression MITnuspec (excerpt) — MIT expression, and no
LoraDb.Client.Nativedependency, so no cycle and no transitive pull-back:LoraDb.Client.Native.nupkg— all five binaries + BUSL-1.1 license file, unchangednuspec (excerpt):
End-to-end consumer smoke test: both packages → embedded transport resolves the native library
Scratch console app, no project references, restoring the two packed
.nupkgs from a local feed, withLORADB_FFI_LIBRARY_PATHexplicitly unset:That is real Rust executing through the FFI bridge, resolved solely from what
LoraDb.Client.Nativeshipped.Consumer with
LoraDb.Clientalone: HTTP works, embedded fails with an actionable errorSame setup,
LoraDb.Clientonly. HTTP runs against a localHttpListener; embedded is then attempted.FFI integration tests still pass
Locally unverified, then confirmed by CI
The HTTP integration tests (Testcontainers,
ghcr.io/lora-db/lora-server) were not run locally — no container runtime was set up in this environment. They are untouched by this change (they never involved the natives), and CI'sintegration (http + ffi)job passes on this branch, along withuniton ubuntu/windows/macos. Everything else above is real local command output.🤖 Generated with Claude Code