Prepare release-ready C# NuGet package workflow#1707
Conversation
Replace the draft nuspec package with a packable SDK-style project that emits a public managed assembly and RID-specific native assets, following the .NET guidance for packages that ship native libraries. Add release-profile native asset scripts, package smoke validation in a clean sample app, and CI jobs that build, pack, and install the preview package across the supported RID matrix: linux-arm64, linux-x64, osx-arm64, osx-x64, win-arm64, and win-x64.
Add maintainer documentation covering package versioning and the publish steps to nuget.org, and link it from the package readme. The package version tracks the payjoin-ffi crate version with a preview suffix, and publication uses the CI-built package artifact so releases always contain every supported RID native asset.
Coverage Report for CI Build 29448737091Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 86.187%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions574 previously-covered lines in 20 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
I'm going to point you to @spacebear21 for the release pipelining here since he's done the other languages and (it seems) has the most information to align this method with those. |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
concept ACK!
I think it would be worth investigating whether we could use a cross-compiler such as https://github.com/cross-rs/cross to simplify compiling for multiple targets across many github runners, instead it could all be done from a single runner for an arbitrary list of targets.
I have not tested this, nor have I reviewed the C#/windows specifics since I'm not familiary with that syntax at all. @ValeraFinebits could you sanity check those?
| [publish guide], [package authoring best practices], and | ||
| [native library packaging] documentation. |
There was a problem hiding this comment.
looks like some links are missing here
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <IsPackable>true</IsPackable> | ||
| <PackageId>Payjoin</PackageId> | ||
| <Version>0.24.0-preview.1</Version> |
There was a problem hiding this comment.
Okay, will match those here too
There was a problem hiding this comment.
I'm not entirely clear on the purpose and name of this file, and how it compares to contrib/test.sh?
| native_library_name() { | ||
| case "$1" in | ||
| linux-*) | ||
| echo "libpayjoin_ffi.so" | ||
| ;; | ||
| osx-*) | ||
| echo "libpayjoin_ffi.dylib" | ||
| ;; | ||
| win-*) | ||
| echo "payjoin_ffi.dll" | ||
| ;; | ||
| *) | ||
| echo "Unsupported RID: $1" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| } | ||
|
|
There was a problem hiding this comment.
These helpers are used in a few files now, it would be good to consolidate them and export them from a shared utilities script to prevent future divergences.
| run: dotnet test --logger "console;verbosity=minimal" | ||
| run: dotnet test Payjoin.Tests.csproj --logger "console;verbosity=minimal" | ||
|
|
||
| build-nuget-native: |
There was a problem hiding this comment.
This, pack-nuget, and maybe smoke-nuget seem tightly coupled and I don't see a clear advantage of having them be independent jobs. Could they be consolidated such that:
- The transient artifacts for each architecture aren't kept around. The only useful artifact is the final payjoin-csharp-nuget-package .nupkg file.
- The os matrix is shared between all relevant steps, to prevent inconsistent matrices between steps on future updates.
- Currently pack-nuget declares the matrix strategy but then uses a hardcoded loop
for rid in linux-arm64 linux-x64 osx-arm64 osx-x64 win-arm64 win-x64; dothat seems prone to error.
- Currently pack-nuget declares the matrix strategy but then uses a hardcoded loop
I also wonder if we could simplify the logic and not rely so much on github runners by using a cross-compiler like https://github.com/cross-rs/cross
ValeraFinebits
left a comment
ValeraFinebits
left a comment
There was a problem hiding this comment.
I reviewed the C# and Windows-specific changes. I found no blocking issues in the CI release path.
I could not complete local packaging validation on Windows, however. After running build_nuget_native.ps1, src/payjoin.cs still contained BitcoindEnv, and dotnet pack failed the production-binding guard. This suggests that the PowerShell path is not reliably preserving the intended empty PAYJOIN_FFI_FEATURES value.
This may not block the current CI release path, because pack-nuget regenerates production bindings on Ubuntu, but it does break the documented local Windows validation flow and should be fixed or documented before publishing.
A few additional non-blocking improvements to consider, either before merge or in a follow-up:
-
The native-asset scripts generate
src/payjoin.csin every RID build job, butpack-nugetgenerates the production bindings again and that is the copy ultimately packaged. Consider separating native-library builds from binding generation, or otherwise avoiding generation in the per-RID jobs when its output is not consumed there. -
NoPackageAnalysis=truedisables package analysis globally. Could we identify the specific package-analysis warnings it avoids and suppress or address only those, so future packaging regressions remain visible? -
Version
0.24.0-preview.1is duplicated inPayjoin.csproj, the workflow smoke-test invocation, and documentation. It would be safer for the workflow/scripts to derive the version fromPayjoin.csproj(or from the produced.nupkg) rather than maintaining a second executable copy manually. Documentation examples can remain descriptive.
build_nuget_native.ps1 marked production bindings by setting PAYJOIN_FFI_FEATURES to "", mirroring the bash scripts. On Windows, assigning an empty string to an environment variable removes it, so generate_bindings.ps1 read it back as unset, fell through to the default, and compiled the _test-utils feature into the bindings. That left BitcoindEnv in src/payjoin.cs and failed the production-binding guard during dotnet pack. Pass production intent through an explicit -ProductionBindings switch instead of an empty environment variable, which Windows cannot represent distinctly from unset. An explicitly-set PAYJOIN_FFI_FEATURES is still forwarded.
The per-RID packaging jobs run generate_bindings, which builds the native library and then regenerates the C# bindings with uniffi-bindgen. Those jobs upload only the native asset; pack-nuget regenerates the production bindings once on Ubuntu and that copy is what ships. The per-RID binding generation is therefore discarded work on every matrix runner. Add a native-only mode (--native-only / -NativeOnly) that builds and copies the native library but skips the uniffi-bindgen step, and use it from build_nuget_native. Callers that need bindings (the pack and test jobs) are unchanged.
The packaging examples still taught setting PAYJOIN_FFI_FEATURES to an empty string, which Windows cannot express, and the pack sequence had no Windows variant at all. Document -ProductionBindings for PowerShell, drop the now-redundant empty assignments (production is the default for build_nuget_native when the variable is unset), and note that the native-asset step no longer regenerates bindings.
NoPackageAnalysis=true silenced every package-analysis warning. Packing with analysis enabled shows exactly one: NU5131, raised because the package ships a ref/net10.0 assembly without a nuspec references group, which csproj-based pack cannot emit. That ref/runtimes split is the intentional native-asset layout and consumption is covered by the per-RID smoke tests, so suppress NU5131 alone and keep future packaging regressions visible.
The package version was maintained in three places: Payjoin.csproj, the workflow smoke invocation, and the documentation examples. The smoke script now accepts 'auto' and derives the version from the single Payjoin.*.nupkg it is pointed at, failing clearly on zero or multiple candidates. CI uses 'auto', so Payjoin.csproj becomes the only place the version is maintained and the smoke test always exercises the artifact that was actually packed.
Native assets were built on six runner types across three operating systems. Build every RID from Linux instead: cargo-zigbuild links the Linux targets against a pinned glibc 2.17 floor (the previous builds inherited the runner's glibc, restricting which distributions could load the .so) and the macOS targets against zig's redistributable libSystem stubs, while cargo-xwin links MSVC-ABI Windows DLLs for both architectures against the Microsoft CRT. build_nuget_native.sh gains a PAYJOIN_FFI_CROSS=1 mode mapping each RID to its Rust triple and tool; bindings are never generated in this mode since the pack step owns the packaged bindings. The workflow matrix keeps one job per RID (unchanged artifact names, so the pack job is untouched) but every row now runs on ubuntu-latest. The per-RID smoke jobs still verify every produced asset on real hardware. All six targets were verified locally from a Linux host: ELF aarch64 (max glibc ref 2.17), Mach-O x86_64/arm64, and PE32+ x86-64/ARM64 artifacts, with release-profile builds validated for both cargo-zigbuild and cargo-xwin.
|
Thanks both, all four points addressed, and the cross suggestion turned into a working implementation locally.
@spacebear21, implemented, with a bit of a hack: cross alone can't cover the matrix (no Apple-darwin images for licensing reasons, and Windows is gnu-ABI only), so this uses the maturin-ecosystem recipe instead: |
RUSTUP_TOOLCHAIN=1.85 selects a different rustup toolchain entry than the 1.85.0 one the toolchain action installed the cross-target std components into, so every cross build failed with E0463 (can't find crate for core). Pin the job to 1.85.0 so the build uses the toolchain that owns the added targets.
Build the cross cargo invocation as an array instead of word-splitting a two-word tool string, which shellcheck flags as SC2086 and which quoting would have broken for 'xwin build'.
cargo-xwin compiles C dependencies with clang-cl and archives them with llvm-lib, and cc-rs looks both up by their unversioned names. The runner image ships LLVM binaries with versioned names only, so the win-arm64 build failed with 'failed to find tool llvm-lib'. Symlink the unversioned names for the Windows rows, with an apt fallback when no versioned binary exists. Also stop cancelling sibling RID builds on the first failure; each target's result is independent signal.
The cross tools produce the exact binaries shipped to nuget.org, but pip installed whatever versions were latest; an upstream release or a compromised PyPI package would silently change the shipped artifacts. Pin ziglang, cargo-zigbuild, cargo-xwin, and ninja (cargo-xwin's transitive dependency) by version and wheel hash, installed with --require-hashes. pack-nuget also still used RUSTUP_TOOLCHAIN=1.85, which rustup resolves to an implicitly-installed latest 1.85.x instead of the action-pinned 1.85.0 — the same mismatch fixed for the native-asset job — and that job builds the bindings that ship in the package. Pin it to 1.85.0.
5 participants
Replaces the draft
nuspecpackaging with a packable SDK-style project that ships a compiled managedPayjoin.dllplus release-profile native libraries underruntimes/{rid}/native/for all six supported RIDs,linux-arm64,linux-x64,osx-arm64,osx-x64,win-arm64,win-x64and adds CI that builds each native asset, packs the.nupkg, and installs it into a clean console app with a smoke test on every RID.RELEASING.mddocuments versioning and the manual publish path with a release-readiness checklist. Version0.24.0-preview.1trackspayjoin-ffi 0.24.0and is an intentional package-model break from the0.0.1placeholder. musl/Alpine is the one platform left out — it needs a musl cross-toolchain rather than just a matrix entry.Verified locally on linux-x64 end-to-end (release native build → pack → clean-app install from a local feed → FFI smoke run, 27/27 tests); the remaining RIDs are exercised by this PR's CI.
Closes #1448
Disclosure: co-authored with Claude Code