Publish siGit Code to NuGet as a .NET tool - #46
Conversation
![sigit-code[bot]](https://avatars.githubusercontent.com/in/3492899?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a sixth release channel publishing siGit Code as a .NET tool (SiGit.Code) on NuGet. Since a .NET tool is one package, nuget/sigit/ bundles all six native binaries under native/<os>-<arch>/ and a managed Program.cs shim execs the matching one, leaving stdio unredirected so TTY detection works. New release-nuget.yml builds each native binary, packs, enforces the 250 MB nuget.org limit, smoke-tests install on three OSes, and publishes. Also sets strip = "symbols" on the release profile to shrink all channels.
Reviewers should check the RID mapping consistency between the shim and workflow, the process-exec/signal handling in the shim, and whether the native/ glob actually lands binaries at the expected package path.
Automated review by siGit Code · commit 0685143
| startInfo.ArgumentList.Add(argument); | ||
| } | ||
|
|
||
| using Process process = Process.Start(startInfo) |
There was a problem hiding this comment.
warning — The shim spawns a child process and waits, but does not forward signals (e.g. Ctrl-C/SIGTERM); on Unix the terminal signal reaches the whole process group so it usually works, but abrupt termination of the launcher can orphan the child or skip cleanup. Consider handling CancelKeyPress / process exit to propagate termination.
| MSBuild from adding the binaries twice: they reach the package through the | ||
| tool's publish output, which is what `dotnet tool install` unpacks. | ||
| --> | ||
| <Content Include="native/**/*" CopyToOutputDirectory="PreserveNewest" CopyToPublishDirectory="PreserveNewest" Pack="false" Visible="false" /> |
There was a problem hiding this comment.
warning — The comment says binaries reach the package via the tool's publish output, but a .NET tool is packed from the build output of the target framework, not a RID-specific publish; verify that CopyToOutputDirectory actually places native/** into the packed tool payload — otherwise the smoke test's .store lookup will fail.
| $_.FullName -like "*native*${runtimeIdentifier}*${binaryName}" | ||
| } | Select-Object -First 1 | ||
|
|
||
| if (-not $nativeBinary) { |
There was a problem hiding this comment.
warning — Write-Error without -ErrorAction Stop (or exit 1) does not fail the step by default when a native binary is missing, so this verification can pass silently.
| shell: bash | ||
| run: | | ||
| package_index_url="https://api.nuget.org/v3-flatcontainer/sigit.code/index.json" | ||
| if curl -fsS "${package_index_url}" | grep -F "\"${RELEASE_VERSION}\"" >/dev/null; then |
There was a problem hiding this comment.
warning — grep -F for "1.5.1" against the flatcontainer index can match a substring (e.g. a prerelease/version containing that text) and wrongly skip publishing; match the exact quoted version entry more precisely.
| return; | ||
| } | ||
|
|
||
| File.SetUnixFileMode(executablePath, currentMode | requiredMode); |
There was a problem hiding this comment.
nit — The permission check only OR-adds missing bits when not all required bits are present, but the final SetUnixFileMode is only reached in that branch; a file with mode 700 (missing group/other) will get group/other read+execute added — likely fine but slightly broader than intended.
Adds a sixth release channel: `dotnet tool install --global SiGit.Code`. A .NET tool is a single package rather than one artifact per platform, so nuget/sigit/ bundles all six native binaries under native/<os>-<arch>/ and a small managed shim execs the right one. This is the pattern smbcloud-cli and onde-cli already use. The shim leaves stdin, stdout, and stderr unredirected so sigit's TTY check still picks TUI or ACP mode correctly. Two things worth knowing about it: The shim sets RollForward=Major. It targets net8.0, and without that a machine carrying only the .NET 10 runtime installs a tool that refuses to start with "You must install or update .NET to run this application". Bundling every target makes the package large, so the pack job fails if the .nupkg crosses nuget.org's 250 MB limit. At v1.5.1 it lands near 160 MB. If that check ever trips, split into RID-specific tool packages instead of dropping targets. Also sets strip = "symbols" on the release profile. That takes the macOS arm64 binary from 102 MB to 88 MB and shrinks every channel, not just this one.
0685143 to
1055253
Compare
There was a problem hiding this comment.
Adds a sixth release channel publishing siGit Code as a .NET tool (SiGit.Code) to NuGet. A GitHub Actions workflow (release-nuget.yml) builds native binaries for six targets, packs them into a single .NET tool package with a managed Program.cs shim that execs the right native binary based on the runtime, enforces a 250 MB package size limit, smoke-tests install across OSes, and publishes. Cargo.toml gains strip = "symbols" on the release profile to shrink binaries. Docs (AGENTS.md, READMEs) updated. Key things to review: the shim's process handling and the Content ItemGroup relying on tool publish output.
Automated review by siGit Code · commit 1055253
| using Process process = Process.Start(startInfo) | ||
| ?? throw new Win32Exception($"Failed to start '{CommandName}'."); | ||
|
|
||
| await process.WaitForExitAsync(); |
There was a problem hiding this comment.
warning — The launcher doesn't forward termination signals (SIGINT/SIGTERM, Ctrl+C) to the child, and WaitForExitAsync has no cancellation; on Unix a true exec (replacing the process) would be cleaner, but at minimum SIGINT handling matters for a TUI/ACP process.
| MSBuild from adding the binaries twice: they reach the package through the | ||
| tool's publish output, which is what `dotnet tool install` unpacks. | ||
| --> | ||
| <Content Include="native/**/*" CopyToOutputDirectory="PreserveNewest" CopyToPublishDirectory="PreserveNewest" Pack="false" Visible="false" /> |
There was a problem hiding this comment.
warning — Verify the native/ Content actually lands in the packed tool: dotnet pack does not run publish, so relying on CopyToPublishDirectory with Pack="false" may exclude the binaries from the .nupkg. Confirm the smoke test would catch a missing binary (it runs on real matrix OSes, but only after publish output is packed).
| $binaryName = if ($IsWindows) { "sigit.exe" } else { "sigit" } | ||
| $nativeBinary = Get-ChildItem ".tool/.store" -Recurse -File | Where-Object { | ||
| $_.FullName -like "*native*${runtimeIdentifier}*${binaryName}" | ||
| } | Select-Object -First 1 |
There was a problem hiding this comment.
nit — The smoke test only warns via Write-Error without -ErrorAction Stop or a non-zero exit, so a missing native binary may not fail the job depending on pwsh error preference; make the failure explicit with exit 1.
| if [[ "${GITHUB_REF_TYPE}" == "tag" ]]; then | ||
| release_version="${GITHUB_REF_NAME#v}" | ||
| else | ||
| release_version="${{ github.event.inputs.tag }}" |
There was a problem hiding this comment.
nit — On workflow_dispatch the checkout uses github.event.inputs.tag but GITHUB_REF_TYPE will be branch, so release_version correctly falls to the input branch here; however the checkout ref at line 55 uses the raw tag input while version strips v — ensure the tag input format (with/without v) is consistent across checkout and version derivation.
3 participants
Adds a sixth release channel:
dotnet tool install --global SiGit.Code.A .NET tool is a single package rather than one artifact per platform, so nuget/sigit/ bundles all six native binaries under native/-/ and a small managed shim execs the right one. This is the pattern smbcloud-cli and onde-cli already use. The shim leaves stdin, stdout, and stderr unredirected so sigit's TTY check still picks TUI or ACP mode correctly.
Two things worth knowing about it:
The shim sets RollForward=Major. It targets net8.0, and without that a machine carrying only the .NET 10 runtime installs a tool that refuses to start with "You must install or update .NET to run this application".
Bundling every target makes the package large, so the pack job fails if the .nupkg crosses nuget.org's 250 MB limit. At v1.5.1 it lands near 160 MB. If that check ever trips, split into RID-specific tool packages instead of dropping targets.
Also sets strip = "symbols" on the release profile. That takes the macOS arm64 binary from 102 MB to 88 MB and shrinks every channel, not just this one.