diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ad6a700..5382fd0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -147,13 +147,25 @@ jobs: # green, the package is cut, and the symptom arrives weeks later as clients that never update. # # -getProperty evaluates without building, so this costs a second and runs before the build. + # + # ◆ -t:MinVer IS NOT OPTIONAL, AND WITHOUT IT THIS STEP FAILED EVERY TAG BUILD. + # + # -getProperty on its own evaluates the project and runs no targets, and MinVer sets Version + # from inside a target — so what came back was the SDK's default 1.0.0, for any tag, on any + # commit, in a clean checkout with every tag present. The step would have failed the first + # release ever cut and blamed fetch-depth for it, which is the one explanation that is not it. + # Naming a target makes -getProperty report the value after that target has run. + # + # Never exercised, because no v* tag exists yet: this whole step is `if:` a tag ref. It is the + # shape of bug a guard is prone to — the guard is the thing nothing tests, and it only speaks + # on the one build nobody is watching, which is the reason this step exists at all. - name: the tag and the version agree if: startsWith(github.ref, 'refs/tags/v') run: | set -euo pipefail tag="${GITHUB_REF#refs/tags/v}" declared="$(dotnet msbuild src/DodoSSH.Client.App/DodoSSH.Client.App.csproj \ - -getProperty:Version -nologo | tr -d '[:space:]')" + -getProperty:Version -t:MinVer -nologo | tr -d '[:space:]')" if [ "$tag" != "$declared" ]; then echo "The tag says v$tag and MinVer computed $declared." >&2 @@ -214,8 +226,7 @@ jobs: # The one build shape nothing else here exercises: a self-contained RID-specific publish. Its # failure mode is a restore graph or a native asset that resolves for net10.0 and not for # net10.0/win-x64, which nobody would see until a person was halfway through cutting a release - # on a Windows machine. vpk can pack a Windows package from Linux; only signing needs Windows, - # and this repository signs nothing yet, so proving the publish here is worth the minutes. + # on a Windows machine. # # RestoreLockedMode=false for this command only, and it is not a loosened gate. The committed # lock files are deliberately RID-free: declaring win-x64 on the desktop head writes a @@ -224,20 +235,144 @@ jobs: # under locked mode, so the image job would fail NU1004. The gate is the locked solution # restore at the top of this job, which is unchanged. # - # It rewrites the lock files as it goes; nothing after this step reads them, and the runner's + # It rewrites the lock files as it goes; nothing after it reads them, and the runner's # checkout is thrown away. The release script does the same thing and puts them back, because # there the tree is somebody's working copy. # # After the tests rather than before them, so a red suite does not first spend a hundred # megabytes pulling a win-x64 runtime pack. main and tags only, for the same reason: a break # found by the person about to release is found early enough. - - name: the windows publish still resolves + # + # The output directory is read by the packaging step below, so the two names have to agree. + - name: publish the windows desktop client if: github.event_name != 'pull_request' run: > dotnet publish src/DodoSSH.Client.App/DodoSSH.Client.App.csproj --configuration Release --runtime win-x64 --self-contained true -p:RestoreLockedMode=false - --output "$RUNNER_TEMP/win-x64-check" + --output "$RUNNER_TEMP/win-x64" + + # ◆ THE WINDOWS INSTALLER IS BUILT HERE, ON LINUX, AND IS DELIBERATELY THROWN AWAY. + # + # Packaging rather than only publishing, for the same reason the android job packages an APK + # nobody installs: the failures a release is most exposed to are the ones only the packager + # finds. vpk opens the published binaries and verifies that the main executable really calls + # VelopackApp.Build().Run() — so a refactor that drops that call, which compiles, tests green + # and produces an application that silently never updates itself, fails here instead of + # shipping. A publish alone cannot see that, and the person who would otherwise see it first is + # the one midway through a release. + # + # ◆ NOT WITH scripts/release-windows.ps1. That script is a person's release procedure and holds + # things a runner must not have and must not skip — it refuses a dirty tree, insists HEAD is + # tagged, downloads the previous release to build deltas against, and in its second phase asks + # for the forge token. Calling it from here would mean either weakening it with CI switches or + # having CI meet conditions that only make sense at a desk. So the packaging arguments below + # are written out again in shell, and the duplication is accepted and stated: the pack id, the + # title, the authors, the channel and the icon are a contract with VelopackUpdateChannel and + # with every installed client. Change one, change the other. + # + # ◆ AND CROSS-COMPILING NEEDS THE OS DIRECTIVE, WHICH IS EASY TO MISS. + # + # vpk picks its target from the host: on Linux, plain `vpk pack` builds a Linux release and + # refuses a Windows RID outright — "the target rid must be Linux (actually was Windows)". The + # bracketed [win] before the verb is what turns it into a cross-compile, and it must be quoted + # or the shell reads it as a glob matching any one of w, i and n. With it, the runner writes + # DodoSSH.Desktop-win-Setup.exe, the portable zip, the full .nupkg and releases.win.json, + # exactly as a Windows machine does. Only signing needs Windows tooling, and nothing here is + # signed — see ADR 0013 rule 8 for the trigger that changes. + # + # ◆ NOTHING IS UPLOADED, AND NOTHING IS EVEN OFFERED FOR DOWNLOAD. + # + # The output goes to RUNNER_TEMP and dies with the job. That is the whole point rather than an + # omission: ADR 0013 rule 3 puts the capability to ship somebody a build on a machine which is + # not a runner, because Velopack clients apply what their feed serves without verifying a + # signature. Publishing a CI-built package as a workflow artefact would not cross that line by + # itself, and it would put an installer nobody has run in a place that invites passing it on — + # so it is not done either. What reaches users is built, installed and walked through Phase 16 + # of docs/manual-checks.md by a person first. + - name: package the windows desktop client + if: github.event_name != 'pull_request' + run: | + set -euo pipefail + + # vpk comes from the tool manifest, at the version committed there, which is the same one + # the release script uses. Restored here rather than in a step of its own because this is + # the only job that needs a tool at all. + dotnet tool restore + + # The same value the release script packs with, read the same way. A version vpk disagrees + # with the assemblies about is a client that compares wrongly, so it is read from MSBuild + # rather than reconstructed from the ref — a main build has no tag to reconstruct from + # anyway, and MinVer's answer there carries the prerelease height that tells two of them + # apart. + # + # -t:MinVer for the reason spelled out on the tag check above: without a target named, + # -getProperty answers 1.0.0 and every package would carry the same version forever, which + # Velopack reads as "no update available". + version="$(dotnet msbuild src/DodoSSH.Client.App/DodoSSH.Client.App.csproj \ + -getProperty:Version -t:MinVer -nologo | tr -d '[:space:]')" + + if [ -z "$version" ]; then + echo "Could not read the version from MSBuild." >&2 + exit 1 + fi + + # ◆ AND A FLOOR UNDER IT, BECAUSE MINVER'S PRE-FIRST-TAG ANSWER IS ONE vpk REFUSES. + # + # Until a v* tag exists MinVer answers 0.0.0-alpha.0.N — correct, documented, and rejected + # outright by vpk: "Invalid package version '0.0.0-alpha.0.143', it must be >= 0.0.1". So + # today, and on every main build until the first release, packing the true version cannot + # work at all. + # + # The patch digit is lifted and the prerelease part kept, so the stand-in still sorts below + # anything real and still differs between commits. Sound only because this package is + # thrown away: what is being proved here is that the desktop client packages, and no client + # will ever see this number. The release script is not given a floor and must not be — its + # version is the one users compare against, and if MinVer ever answered 0.0.0 there the + # right outcome is the refusal. + packVersion="$version" + case "$packVersion" in + 0.0.0*) + packVersion="0.0.1${packVersion#0.0.0}" + echo "MinVer says $version, which vpk will not pack; packaging as $packVersion." + echo "This disappears the moment a v* tag exists, and never reaches a release." + ;; + esac + + releases="$RUNNER_TEMP/win-releases" + + # --skip-updates because vpk otherwise asks nuget.org whether a newer vpk exists on every + # run, which is a network call whose answer this build must not act on: the version that + # packs is the one in the tool manifest. + # + # No `vpk download`, where the release script has one. That fetches the previous release so + # deltas can be built against it, which needs the feed and produces an artefact nobody + # applies. A full package proves the packaging. + dotnet vpk '[win]' pack \ + --skip-updates \ + --packId DodoSSH.Desktop \ + --packVersion "$packVersion" \ + --packDir "$RUNNER_TEMP/win-x64" \ + --packTitle DodoSSH \ + --packAuthors DodoTech \ + --mainExe DodoSSH.exe \ + --icon src/DodoSSH.Client.App/Assets/dodossh.ico \ + --runtime win-x64 \ + --channel win \ + --outputDir "$releases" + + # Asked for rather than assumed. vpk fails loudly and this costs a line, but the artefact + # whose existence is the entire question — a Windows setup stub built on a machine that is + # not Windows — is worth naming rather than inferring from an exit code. + setup="$releases/DodoSSH.Desktop-win-Setup.exe" + if [ ! -s "$setup" ]; then + echo "vpk reported success and there is no setup executable at $setup." >&2 + ls -la "$releases" >&2 || true + exit 1 + fi + + ls -la "$releases" + echo "Packaged DodoSSH $packVersion for win-x64, from a build MinVer calls $version." # This includes the end-to-end suite, which starts PostgreSQL, Keycloak and an OpenSSH # server through Testcontainers and runs the API as a child process — so it needs a @@ -892,18 +1027,27 @@ jobs: if: always() && github.event_name != 'pull_request' run: docker logout registry-docker.dodotech.cloud -# There is no job here that publishes the desktop client, and there is not going to be one. Two -# independent reasons, and both need saying because someone will fix one and think they are done. +# There is no job here that publishes the desktop client, and there is not going to be one. It is built +# and packaged here — the two steps at the end of the build job — and what is withheld is only the +# upload. # -# The smaller one is mechanical: vpk stamps and embeds the Setup.exe and Update.exe stubs with Windows -# tooling, and every job in this file is runs-on: [linux]. A Windows runner would answer that. +# ◆ ONE REASON, WHERE THIS ONCE CLAIMED TWO, AND THE SECOND WAS NOT TRUE. # -# The larger one is that a Windows runner would not answer the other. Velopack clients fetch from the -# release feed and do not verify a package signature when they apply it, so whoever can write a release -# on this repository can publish an update that every installed client downloads and runs. That is the -# same capability as the signing key, reached through a different door — and docs/adr/0011 rule 1 puts -# that capability on a machine which is not a runner, because a workflow secret is held by everyone who -# can change a workflow file. See docs/adr/0013-desktop-distribution-and-updates.md. +# It used to say that vpk stamps and embeds the Setup.exe and Update.exe stubs with Windows tooling and +# so could not run on a Linux runner, and offered that as the smaller of two reasons. It was wrong, and +# worth recording as wrong because it is the kind of claim that discourages anybody from trying: vpk +# cross-compiles when told to, `vpk [win] pack` on this runner builds the setup stub, the portable zip +# and the .nupkg, and only signing needs Windows — which this repository does not do yet. That is what +# the packaging step now does on every main and tag build. +# +# The reason that stands is the one that was never mechanical. Velopack clients fetch from the release +# feed and do not verify a package signature when they apply it, so whoever can write a release on this +# repository can publish an update that every installed client downloads and runs. That is the same +# capability as the signing key, reached through a different door — and docs/adr/0011 rule 1 puts that +# capability on a machine which is not a runner, because a workflow secret is held by everyone who can +# change a workflow file. A Windows runner would never have answered it, which is why fixing the +# mechanical half changes nothing about the division of labour. +# See docs/adr/0013-desktop-distribution-and-updates.md. # # What cuts a release is scripts/release-windows.ps1, run by a person. What this file does is prove the # thing still builds and packages, which is the same division of labour the android job above already diff --git a/docs/adr/0013-desktop-distribution-and-updates.md b/docs/adr/0013-desktop-distribution-and-updates.md index 2f90ddc..1b6e5eb 100644 --- a/docs/adr/0013-desktop-distribution-and-updates.md +++ b/docs/adr/0013-desktop-distribution-and-updates.md @@ -85,13 +85,38 @@ packs and stops. Phase two, a separate invocation, asks for the token and upload what reaches users has been installed and started by a human first, and so that the credential is in memory only for the minutes that need it. -There is a second, smaller reason the release could not be a CI job here anyway: `vpk` stamps and embeds -the `Setup.exe` and `Update.exe` stubs with Windows tooling, and every job in `ci.yml` is -`runs-on: [linux]`. Both reasons are recorded because somebody will fix one and believe they are done. +A second, smaller reason was recorded here — that `vpk` stamps and embeds the `Setup.exe` and +`Update.exe` stubs with Windows tooling, and every job in `ci.yml` is `runs-on: [linux]` — on the +reasoning that somebody will fix one reason and believe they are done. -What CI does gain is the same thing the `android` job already does — it proves the artefact still builds. -A `win-x64` publish runs on main and on tags, so a restore graph that resolves for `net10.0` and not for -`net10.0/win-x64` fails there rather than under a person midway through a release. +◆ **That second reason was not true, and the correction is worth more than the claim was.** `vpk` +cross-compiles when it is told to: the bracketed OS directive, `vpk [win] pack`, builds the setup stub, +the portable bundle and the `.nupkg` on Linux, and reports `Directive enabled for cross-compiling from +Linux (current os) to Windows` while doing it. Only signing needs Windows tooling, and rule 8 above is +why nothing is signed yet. Plain `vpk pack` on Linux targets Linux and rejects a Windows RID, which is +the shape of the mistake: the tool does not fail because it cannot, it fails because it was not asked. + +The correction does not move this rule at all, and that is the point of separating the two. The reason +that stands was never mechanical, so a Linux runner that *can* pack changes nothing: what may not be on +a runner is the token, not the build. + +What CI gains is the same thing the `android` job already does — it proves the artefact still builds and +still packages. On main and on tags the build job publishes `win-x64` and then packs it, so a restore +graph that resolves for `net10.0` and not for `net10.0/win-x64` fails there rather than under a person +midway through a release. Packaging is the more valuable half and adds a few seconds to a publish that +has already happened — measured at two to seven, against the minutes the publish itself takes: `vpk` verifies +that the main executable actually calls `VelopackApp.Build().Run()`, so the change that quietly produces +an application which never updates itself — compiling, tested, and invisible until a release goes +unreceived — fails in CI. The packages are written to `RUNNER_TEMP` and discarded, and are deliberately +not offered as workflow artefacts: an installer nobody has run should not sit somewhere that invites +passing it on. + +CI does not call `scripts/release-windows.ps1` to do any of this, and should not. That script is the +person's procedure — it refuses a dirty tree, requires a tagged `HEAD`, fetches the previous release for +deltas, and prompts for the token — so running it on a runner would mean either weakening it with CI +switches or having CI satisfy conditions that only make sense at a desk. The workflow writes the same +`vpk` arguments out in shell, and the constants they share (pack id, title, authors, channel, icon) are +noted in both places as a contract with `VelopackUpdateChannel` and with every installed client. ### 4. The update check points at the project's forge, and the address is a constant diff --git a/docs/platform-flags.md b/docs/platform-flags.md index da9d92e..ca5bfa3 100644 --- a/docs/platform-flags.md +++ b/docs/platform-flags.md @@ -568,12 +568,37 @@ Packaging the desktop client had broken the server's image build, and nothing bu have caught it. Found by running `docker build` locally rather than by reading the lock files. So the RID stays out of the committed state, and the two commands that need one — the release script's -publish and the `windows publish still resolves` step in `ci.yml` — pass `-p:RestoreLockedMode=false` for -themselves alone. That restore rewrites the lock files as a side effect, which does not matter on a runner +publish and the `publish the windows desktop client` step in `ci.yml` — pass `-p:RestoreLockedMode=false` +for themselves alone. That restore rewrites the lock files as a side effect, which does not matter on a runner whose checkout is discarded and does matter on a developer's machine, so the release script runs `git checkout -- '*packages.lock.json'` afterwards. `-p:RestorePackagesWithLockFile=false` is not an alternative: it fails NU1005 whenever a lock file already exists. +**`vpk` picks its target from the host, and cross-compiling is a bracketed directive rather than a flag.** +The CI job packages the Windows desktop client on a Linux runner, and plain `vpk pack --runtime win-x64` +there refuses outright: + +``` +To build packages for Linux, the target rid must be Linux (actually was Windows). If your real intention +was to cross-compile a release for Windows then you should provide an OS directive: eg. 'vpk [win] pack ...' +``` + +The directive goes before the verb — `dotnet vpk '[win]' pack …` — and must be quoted in a POSIX shell, +where `[win]` is a glob matching any one of `w`, `i` and `n`. With it, a Linux runner logs +`Directive enabled for cross-compiling from Linux (current os) to Windows` and writes +`DodoSSH.Desktop-win-Setup.exe`, the portable zip, the `.nupkg` and `releases.win.json` — the same set a +Windows machine produces. `--runtime win-x64` is still required: the directive says which OS is being +targeted, not which RID. Only signing needs Windows tooling, which is why `ci.yml` can package and +`scripts/release-windows.ps1` will still be the thing that signs when there is a certificate to sign with. + +**`dotnet msbuild -getProperty:Version` answers `1.0.0`, and MinVer is not to blame.** `-getProperty` +without a target *evaluates* the project and runs nothing, while MinVer computes the version inside a +target — so the read comes back as the SDK default on a full checkout with every tag present, which looks +exactly like a version that was never configured. `-t:MinVer` makes `-getProperty` report the value after +that target has run, and both readers of it — the tag check in `ci.yml` and `scripts/release-windows.ps1` +— pass it. Neither had, and neither had ever run: the CI check is `if:` a tag ref and there are no tags +yet, so the first release would have been refused by its own guard, which would have blamed `fetch-depth`. + **The Android head's lock file is outside the solution, so nothing checks it until the android job runs — and the android job was broken for an unrelated reason for the whole of the release that went stale.** `DodoSSH.Client.Android` is deliberately not in `DodoSSH.slnx` (it needs a workload the other two jobs diff --git a/scripts/release-windows.ps1 b/scripts/release-windows.ps1 index 38f3073..fcc5577 100644 --- a/scripts/release-windows.ps1 +++ b/scripts/release-windows.ps1 @@ -100,7 +100,12 @@ Push-Location $RepoRoot try { # ---- What is being released ----------------------------------------------------------------------- - $version = (& dotnet msbuild $Project -getProperty:Version -nologo) -replace '\s', '' + # -t:MinVer, and it is load-bearing. -getProperty on its own evaluates the project and runs no + # targets, while MinVer sets Version from inside one — so this read answered the SDK's default + # 1.0.0 regardless of the tag, and the check below would then have refused to build anything not + # tagged v1.0.0. CI's tag check had the same line and the same fault; both are fixed, and both + # say so, because this is the version that ends up in the package a client compares against. + $version = (& dotnet msbuild $Project -getProperty:Version -t:MinVer -nologo) -replace '\s', '' if ([string]::IsNullOrWhiteSpace($version)) { Stop-With 'Could not read the version from MSBuild.' }