libstore: say what actually blocks SSH on Windows

[awsmadi]

Follow-up to a review comment on the merged #16347: xokdvium
pointed out that the TODOs there now read
"once we can fork", which is a precondition Windows is never going to meet.

He's right about the wording. Reverting to the previous "once we can start processes" would be stale in the
other direction, though, because #16347 is what made Pid and ordinary process spawning work on Windows.
What these three sites actually need is startProcess(fun<void()>), declared inside #ifndef _WIN32 at
processes.hh:128, which runs its callback in the forked child — that is where startCommand does its
dup2s over stdin, stdout and stderr, and CreateProcess has no equivalent step.

So this names the missing primitive rather than reverting. The UnimplementedError on the Windows branch
gets the same treatment, since it still claimed "because spawning processes is not yet implemented", which
is no longer the reason.

Comments and one error message; no behaviour change. Verified nix build .#checks.x86_64-linux.pre-commit
(7/7 hooks) and that nix-store-x86_64-w64-mingw32 still compiles, since the changed error message sits in
an #ifdef _WIN32 branch that a native build never sees.

[Ericson2314]

I would like to be even clear. We spawnProcess is only possible with fork, which we will never have, so we will never have spawnProcess. The TODO is to reimplement how we spawn these processes so the spawn process is not "arbitrary lambda"-based.

The TODO can still be short though, e.g. "reimplement launching without spawnProcess so we don't require fork"

[awsmadi]

Agreed, and that's a better framing than mine — "needs startProcess()" still reads as waiting to acquire
it on Windows, when the work is on our side.

One check before I push, since you wrote spawnProcess: the function these three sites actually go through
is startProcess(fun<void()>) (processes.hh:128). Assuming you mean that one, and not an API you have in
mind for the replacement. Proposed, following your phrasing:

#ifndef _WIN32 // TODO reimplement launching without startProcess(), so it doesn't require fork.

The other question is the UnimplementedError on the Windows branch, which this PR also touches. I'd lean
toward keeping that one causal rather than aspirational — "launching goes through startProcess(), which
requires fork" — since someone who hits it wants the reason rather than our backlog item. Say if you'd
rather it match the TODO.

[xokdvium]

Alternatively, let's just wait for spawnProgram changes that would come anyway? Most of this would become a runProgram2 split into spawnProgram which returns a Pid and supports standard stream redirects.

I do have some stuff for that cooking.

[awsmadi]

I checked whether that would actually subsume these sites rather than guess, and it would — both of them.

Enumerating everything the two startProcess child lambdas do, there is nothing that needs to run in the
child beyond what a redirect-capable spawn gives you:

in the child	absorbed by
close(in.writeSide) / close(out.readSide), dup2 onto stdin/stdout/stderr	the stream redirects
restoreProcessContext()	internal to the spawn
addCommonSSHOpts, arg splicing, createSSHEnv, stringsToCharPtrs	pure computation, fine in the parent
nix::execvpe	the spawn itself

No setsid, no fd juggling beyond the pipe ends, nothing computed post-fork. So startCommand
(ssh.cc:171) and startMaster (ssh.cc:257) both reduce to "spawn ssh with these three streams pointed
at these fds", and ssh.hh:43 is just the declaration guard for the latter, so it follows.

Two things spawnProgram would need for these callers specifically, worth having on the list while it is
still being designed:

1. A custom environment. Both sites execvpe with createSSHEnv(), not the inherited environment.
2. dieWithParent = false. Both set it. The control master in particular has to outlive the spawning
   process, so if that is not expressible the master case regresses.

There is one mechanical consequence rather than an obstacle: startMaster currently builds its args inside
the child while holding state_.lock(), reading state->socketPath there. With a spawn API the args get
materialised in the parent instead, which is fine here — nothing in that computation depends on being
post-fork.

On whether to land this PR versus wait for your split: that is genuinely your and @Ericson2314's call, and
you two currently point opposite ways — his suggestion above was to reword the TODO and land it, yours is
that the reword lands on lines your change deletes. I have no stake in which; say which you would prefer and
I will follow it. @Ericson2314, that also settles the naming question I left upthread, so you needn't answer it: the
function all three sites go through is startProcess(fun<void()>), declared inside #ifndef _WIN32 at
processes.hh:128. There is no spawnProcess in the tree today — I take it you meant that one, and the
enumeration above is against it.

[Ericson2314]

@awsmadi Honestly if you thought that must about it, you should just make it work with spawnProgram. That is more interesting that merely changing the TODO.

[awsmadi]

Measured before assuming: spawnProgram is not in master (7f711bef5) — 0 files, against 18 for
startProcess and 9 for runProgram2 as controls — and no open PR adds it. No commit in the repository's
history mentions it either. runProgram2 is there, which matches what @xokdvium described splitting.

So this can't be built on spawnProgram as things stand — #16382 would be blocked on work that isn't pushed
yet.

@xokdvium, could you publish that branch, even as a draft? That would give this something concrete to target.

Closing the loop on the earlier question while I'm here: the function all three sites go through is
startProcess(fun<void()>), declared inside #ifndef _WIN32 at processes.hh:128.

[awsmadi]

Correction to my previous comment, and it sharpens the blocker rather than softening it.

I said spawnProgram does not exist anywhere in the tree. That was true of that spelling and unhelpful of me. The function you meant is almost certainly spawnProcess, and it does exist: src/libutil/windows/processes.cc:200, Pid spawnProcess(const std::filesystem::path &, const RunOptions &, Pipe &). I searched the literal string and reported absence, which was the wrong measurement to run.

The blocker survives the correction, for two specific reasons:

1. It is not exported. spawnProcess is declared in no header -- git grep spawnProcess -- '*.hh' against master matches zero files. Its only caller is in the same translation unit. So src/libstore/ssh.cc cannot reach it as things stand.
2. It hardcodes stdin. Line 210 is AutoCloseFD in = nullFD();, and RunOptions has no standardIn member to override that. Every one of these ssh.cc sites needs a writable stdin, since that is how the remote end gets driven, so a null stdin is not something the call site can paper over.

I had already worked this out and then contradicted myself, which is the annoying part: the #ifdef _WIN32 arm I added in derivation-building-goal.cc reads "What is missing is that spawnProcess is not exported, and that the worker needs an AsyncPipe tied to the completion port rather than the plain Pipe this produces." Same function, same two gaps. My earlier comment here should have said that.

So the open question is unchanged but better grounded: exporting spawnProcess and giving RunOptions a standardIn look like the prerequisites, and @xokdvium may already have some of that in flight. Is that work somewhere I can build against, or would you rather I leave these sites as UnimplementedError and just fix the comments to name spawnProcess and the two real gaps instead of the vaguer wording they have now?