Skip to content

npins: init at 0.5.1 - #212

Merged
jonringer merged 5 commits into
masterfrom
ic4-y/npins
Sep 22, 2026
Merged

jonringer merged 5 commits into
masterfrom
ic4-y/npins

Conversation

@ic4-y

@ic4-y ic4-y commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Why

ekala-org pins nixpkgs for exactly two packages corepkgs does not carry — npins and lefthook — and describes it as a gap-filler (ekala-org/README.md:7, ekala-org/nix/nixpkgs.nix:3). Lefthook landed in #209. This closes the second gap, so the nixpkgs input can be dropped entirely and the dependency graph collapses to a single npins-pinned corepkgs.

npins is the pinning tool the project already runs (nix/npins/sources.json, nix run nixpkgs#npins -- -d nix/npins update), so carrying it here also removes the bootstrapping oddity of fetching the pinning tool from the thing it pins.

What it does

Ports pkgs/by-name/np/npins/package.nix from nixpkgs master into pkgs/npins/default.nix. Registered automatically from the directory name — no top-level.nix entry.

The package wraps npins with the external tools it shells out to, because npins is a process orchestrator: every pin type it supports is delegated to a separate binary. Miss one and that pin type fails at runtime with command not found, not at build time.

Divergence from nixpkgs

Five deliberate departures. A reviewer reading the diff against upstream should see each as a decision, not drift:

# Change Why
1 Wrap nix npins spawns nix-prefetch-url (libnpins/src/nix.rs:20) and nix-instantiate (:264) directly, so nix is needed on every invocation. It also supplies the nix-hash/nix-store that nix-prefetch-git's own wrapper omits when --hash is passed without --builder (#211). nixpkgs dropped this in b8a35d1d, reasoning that other packages assume nix is in PATH; that assumption does not hold in a bare dev shell
2 Wrap skopeo Called directly for container pins (libnpins/src/nix.rs:204). nixpkgs omits it, leaving npins add container latently broken
3 Omit openssh See below
4 Hoist/simplify the completions guard nixpkgs builds npins-completions unconditionally while guarding only its use, so on a cross build the build-time helper still lands in $out/bin and has to be removed by a second, unguarded rm. This gates the build instead, which collapses generation and removal into one guarded postInstall fragment
5 Drop the orphaned # (Almost) all tests require internet comment It justified nixpkgs' doCheck = false, which this repo's rust builder already defaults (pkgs/rust/build-rust-package/default.nix:172)

On openssh

npins sets GIT_SSH_COMMAND="ssh -o StrictHostKeyChecking=yes" (libnpins/src/nix.rs:76, pins/git.rs:695). That is a shell-resolved command, so git finds ssh via PATH: get_ssh_command() returns the env value before consulting GIT_SSH or core.sshcommand.

So ssh:// pins work wherever the caller's environment provides openssh — any standard system — and fail loudly with ssh: command not found where it does not. Measured cost to add it: +77.5 MiB on a 635.9 MiB closure (~12%).

Omitted because no ssh:// pin exists in this project, the failure names its own fix, and nixpkgs omits it too. The rationale is recorded at the runtimePath binding so a future reader sees a decision rather than an oversight.

Note for anyone revisiting: git.override { withSsh = true; } would not help. pkgs/git/ssh-path.patch rewrites the else branch of connect.c — reached only when GIT_SSH_COMMAND is unset, which npins never does. Add openssh to runtimePath instead.

Departure 1 is verified functionally, not argued: with nix off PATH nix-prefetch-git --hash sha256 … dies with nix-hash: command not found at line 522; with nix on PATH the identical invocation returns the full JSON result. Relates to #211, which this does not block on — npins needs nix for its own direct calls regardless of how #211 is resolved.

Verification

nix-build -A npins
$out/bin/npins --version        → npins 0.5.1
ls $out/bin                     → npins   (helper removed; departure 4)
source …/npins.bash && complete -p npins → complete -F _npins npins
nix-build -A npins.passthru.tests.version  → passes
./ci/eval.sh                    → All 2063 packages evaluate

The wrapper PATH resolves nix, nix-prefetch-git, nix-prefetch-docker, skopeo, and git, and contains no openssh. Beyond building, npins was exercised end-to-end in a scratch dir: npins init fetched a real nixpkgs revision, and npins add git … --at 0.5.1 resolved sha256-PRdGQlxpv8qXdQ6KwlP2Ky2HBHDY83lGTSiD6yljUxE= — independently reproducing the src hash pinned above.

Try it locally

nix build github:ekala-project/corepkgs/ic4-y/npins#legacyPackages.x86_64-linux.npins

Intent source

  • Kind: prompt/reference
  • Source: an npins: init at 0.5.1 implementation handoff that specified the target file, the five departures, and the PR-body requirements. The upstream reference is pkgs/by-name/np/npins/package.nix on nixpkgs master.
  • Verification: /verify pr should confirm the delivered package realizes the handoff — a working npins with all six runtime tools wrapped, the documented divergences present, and the omissions annotated in-file.

Both argument lines were unexplained in the local delta against nixpkgs. Note each one's direct call site, and that only half of nix's justification disappears once corepkgs#211 is fixed.
npins-completions was built unconditionally while only its use was guarded, so on a cross build it still landed in $out/bin and forced an unguarded rm to remove it. Gate the build itself and collapse generation and removal into one guarded postInstall fragment, restoring the phase ordering the upstream expression relied on. nixpkgs keeps the rm inside the guard, which ships the helper on cross builds.
The openssh rationale explains why runtimePath omits a dependency, but sat above postFixup where that binding is merely consumed. Move it to the binding it describes and align the formal argument order with runtimePath so the two lists read identically.
@ic4-y

ic4-y commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Hickey/Lowy Analysis

# Lens Finding Disposition
1 Hickey completion generation and helper removal coupled only by string order Fixed in this PR
2 Hickey npins-completions built unconditionally though only usable when the host can execute Fixed in this PR
3 Hickey runtime-dep set spelled twice (formals and runtimePath) ⚠️ No-op
4 Hickey openssh omission asserted in prose only ⚠️ No-op
5 Hickey lib.optionalString … + ''…'' does not complect completions with PATH setup ⚠️ No-op
6 Hickey hoisting rm out of the guard is functionally required ⚠️ No-op
7 Hickey passthru.updateScript appears to contradict the porting guidance ⚠️ No-op
8 Hickey $src dereferenced in postFixup under structured attrs ⚠️ No-op
9 Lowy runtimePath is a closed, faithful map of npins' outbound tool surface ⚠️ No-op
10 Lowy nix has two justifications but the file recorded only one Fixed in this PR
11 Lowy openssh omission leaves ssh support caller-supplied, resolved oppositely to the nix leak Accepted residual risk
12 Lowy upstream churn isolates cleanly via the existing sync tooling ⚠️ No-op
13 Lowy added runtime deps carried no divergence marker Fixed in this PR
14 Lowy splitting into a base + overlay file would be the wrong boundary ⚠️ No-op
15 Lowy updateScript cannot couple to the local divergence ⚠️ No-op
16 Lowy cargoHash not regenerated by the update shim ⚠️ No-op
17 Lowy no shared abstraction added, no concept multiplied ⚠️ No-op

Three findings were fixed; one is an accepted residual risk. A cross-validation round then ran once per lens, and both returned nothing new — the applied fixes strictly reduce interleaving relative to the initial commit.

Hickey rationale

Two real structural defects, both rooted in the same mistake: guarding use while building unconditionally.

F1 (temporal coupling). Generation and removal lived in two concatenated fragments whose only ordering guarantee was left-to-right +. Writing them in the other order would silently delete the helper before use. The builder offers real phase ordering (installPhase → postInstall → fixupPhase → postFixup), so the pair now occupies one guarded postInstall fragment.

F2 (root cause). cargoBuildFlags always passed -p npins-completions; the canExecute guard suppressed only its invocation. The helper therefore still landed in $out/bin — cargoInstallHook copies every executable — which is exactly what forced the second, unguarded rm. Gating the build instead lets the run-and-remove pair collapse into one guarded unit, which subsumes F1 entirely.

postInstall is the repo's documented home for installShellCompletion (used by cue, git-lfs, ast-grep); wrapProgram remains in postFixup, so the two concerns stay in their natural phases. Verified: $out/bin holds only npins, and all three completions still install.

The remaining seven were dismissed with evidence rather than dropped — notably #3, which is unavoidable because callPackage requires individually named formals (every makeBinPath user in the tree duplicates likewise), and #8, where $src is exported to build phases.

Lowy rationale

F10 (the finding worth fixing). Adding nix to runtimePath has two independent justifications, and the file recorded only one. Reason A: npins itself spawns nix-prefetch-url and nix-instantiate, so nix is required for every invocation. Reason B: nix-prefetch-git reaches nix-hash/nix-store, which its own wrapper omits (#211). Only Reason B disappears when #211 closes — and with #211 referenced nowhere in-tree, the natural reading on that day would be "this became redundant", silently breaking url and channel pins. Both reasons are now annotated at the formal, so the surviving half is identifiable.

F13 is the same failure mode in miniature: the two added dependencies were the only unexplained lines in the local delta, because the sync normaliser strips bare testers,/nix-update-script, argument lines but not these.

F11 (accepted residual risk). The openssh omission and the nix inclusion are structurally similar transitive leaks resolved oppositely. Lowy's symmetry check flags that asymmetry; it is accepted deliberately — nix has no ambient fallback and is needed on every invocation, whereas openssh is supplied by any standard environment, costs 12% of closure, and fails loudly with the fix named. Recorded here so it is not re-litigated.

F12 found the boundary question already answered by the repo: the sync tooling pairs the whole pkgs/npins/ directory with one upstream by-name path and normalises the incidental divergence, so the genuine local delta is already isolated. No new encoding needed.

Comment thread pkgs/npins/default.nix Outdated
Comment on lines +42 to +44
# npins-completions is a build-time helper, not a shipped binary: it is only
# useful on a host that can execute the npins it just built, so it is neither
# built nor invoked when cross-compiling.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of this can be inferred, do you mind having the comments shrunk down to a line or less?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem.

Review feedback on #212: most of the prose can be inferred from the code.

Collapse five multi-line comment blocks to one line each:

- the runtime-dep preamble and per-dep notes move onto the formals
  themselves, keeping both reasons `nix` is required (npins' own direct
  calls, and nix-prefetch-git's nix-hash path from #211) so the surviving
  half stays identifiable if #211 closes
- the openssh note keeps the mechanism (GIT_SSH_COMMAND is shell-resolved,
  so ssh comes from the caller's PATH) and drops the closure arithmetic
- the completions guard and its build/removal pairing each fit one line

No change to the derivation: same formals, runtimePath, build flags,
postInstall and postFixup. Rebuilt and re-checked: `npins --version`
returns 0.5.1, `$out/bin` holds only `npins`, all three completions
install, and the wrapper PATH still carries git.
@ic4-y
ic4-y marked this pull request as ready for review September 22, 2026 16:15
@jonringer
jonringer merged commit 97a85b2 into master Sep 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants