diff --git a/.github/workflows/release-artifacts.yml b/.github/workflows/release-artifacts.yml index c1ed0cc88..c5938dc3f 100644 --- a/.github/workflows/release-artifacts.yml +++ b/.github/workflows/release-artifacts.yml @@ -206,55 +206,10 @@ jobs: # Platform payloads first under the `platform` dist-tag, then the wrapper, # with the `latest` dist-tag checked before and after (platform versions # are semver PRERELEASES of the wrapper version and must never become - # `latest`). Versions already on the registry are skipped, so "Re-run - # failed jobs" is safe after a partial publish. + # `latest`). npm applies publishes asynchronously, so the script waits + # until npm shows every platform version before the wrapper goes out, + # and until `latest` moves after it. Versions already on the registry + # are skipped, so "Re-run failed jobs" is safe after a partial publish. - name: Publish npm packages shell: bash run: scripts/npm/publish-packages.sh dist/npm - - # One final release under the pre-rename name @gitlawb/zero. Zero 0.9.0's - # `zero upgrade` runs `npm install -g @gitlawb/zero@latest`, and people - # reinstall the old name from shell history and old tutorials, so this - # release is published once more under it: the same verified artifacts, - # assembled as @gitlawb/zero, which the binary and wrapper recognise and - # answer with the switch steps (docs/NPM_PACKAGING.md). Runs after the - # @twigpine/zero publish, so the package it points at already exists. - # - # The gate keeps this to a single release: it is owed only while the old - # name's latest is still the last pre-rename version, or is this version - # (a re-run after it went out). After this release, `npm deprecate` the - # old package and delete these two steps. - - name: Check whether the final @gitlawb/zero release is owed - id: legacy-name - shell: bash - env: - LAST_PRE_RENAME_VERSION: 0.9.0 - run: | - version="$(node -p "require('./package.json').version")" - latest="" - for _ in 1 2 3; do - latest="$(npm view @gitlawb/zero dist-tags.latest 2>/dev/null || true)" - if [ -n "$latest" ]; then - break - fi - sleep 10 - done - if [ -z "$latest" ]; then - echo "::error::could not read the latest dist-tag of @gitlawb/zero from the registry" >&2 - exit 1 - fi - if [ "$latest" = "$LAST_PRE_RENAME_VERSION" ] || [ "$latest" = "$version" ]; then - owed=true - else - owed=false - fi - echo "@gitlawb/zero latest is ${latest}; final release under the old name owed: ${owed}" - echo "owed=${owed}" >> "$GITHUB_OUTPUT" - - - name: Publish the final @gitlawb/zero release - if: ${{ steps.legacy-name.outputs.owed == 'true' }} - shell: bash - run: | - node scripts/npm/build-platform-packages.mjs \ - --artifacts-dir dist/artifacts --out-dir dist/npm-legacy --package-name @gitlawb/zero - scripts/npm/publish-packages.sh dist/npm-legacy diff --git a/docs/NPM_PACKAGING.md b/docs/NPM_PACKAGING.md index 50ddffa2d..bb5846b87 100644 --- a/docs/NPM_PACKAGING.md +++ b/docs/NPM_PACKAGING.md @@ -130,9 +130,14 @@ registry. bootstrap publish) — a missing value must not abort the job between the platform and wrapper publishes, which would strand a half-published release. -2. **Platform versions publish before the wrapper.** The wrapper's - `optionalDependencies` pin exact suffixed versions; publishing the wrapper - first would create a window where installs resolve aliases that 404. +2. **Platform versions are on the registry before the wrapper.** The + wrapper's `optionalDependencies` pin exact suffixed versions; publishing + the wrapper first would create a window where installs resolve aliases + that 404. Ordering the uploads is not enough: npm applies a publish + asynchronously, and in 0.10.0 a platform version appeared ten minutes + after its upload returned, after the wrapper. The publish script waits + until npm shows every platform version before it publishes the wrapper, + and waits the same way for `latest` afterwards (up to 30 minutes in all). 3. **Exact-version pinning.** The wrapper at `X.Y.Z` references platform versions `X.Y.Z--` exactly — never ranges — so a wrapper and its binaries can never skew. The publish script refuses to start when @@ -161,54 +166,31 @@ bin, so npm refuses to install one over the other (`EEXIST`), and forcing it lets a later uninstall of the old package delete the shared bin. The user has to uninstall the old package and install the new one, in that order. -Deprecating the old package alone does not reach existing users. 0.9.0's -`zero upgrade` runs `npm install -g @gitlawb/zero@latest`; with nothing newer -under that name, npm reinstalls nothing, prints no deprecation warning (it -only warns when it installs something), and Zero reports the update as done. -So 0.10.0 is published once more under the old name, as a bridge: - -- The publish job assembles the same verified artifacts a second time with - `build-platform-packages.mjs --package-name @gitlawb/zero` and publishes - them with the same script, after the `@twigpine/zero` publish. A gate runs - it only while the old name's `latest` is still 0.9.0 (or is this version, - on a re-run), so it cannot happen twice. +Deprecating the old package alone would not have reached existing users. +0.9.0's `zero upgrade` runs `npm install -g @gitlawb/zero@latest`; with +nothing newer under that name, npm reinstalls nothing, prints no deprecation +warning (it only warns when it installs something), and Zero reports the +update as done. So 0.10.0 was published once more under the old name, as a +bridge: + +- The release job assembled the same verified artifacts a second time with + `build-platform-packages.mjs --package-name @gitlawb/zero` and published + them with the same script, after the `@twigpine/zero` publish. The steps + that did this ran once and have been removed from `release-artifacts.yml`. - The bridge build knows its name from the `package.json` beside it. The Go binary reports it as `InstallMethodNpmLegacy`: `zero upgrade` refuses with the switch steps instead of reinstalling it, and `zero update --check` adds them. `bin/zero.js` prints the same steps after `zero` exits, on a - terminal only. -- Updating from 0.9.0 now installs a real version, so npm also shows the + terminal only. This handling stays in the code, since only a build + installed under the old name ever triggers it. +- Updating from 0.9.0 installs that bridge build, so npm also shows the deprecation message. -### Runbook - -Before the first release under the new name: - -1. Create the package, since npm only attaches a trusted publisher to a - package that exists: publish an empty `@twigpine/zero@0.0.0` by hand with - `npm publish --access public --tag bootstrap`, then - `npm deprecate @twigpine/zero@0.0.0 "Placeholder; install @twigpine/zero@latest"`. -2. Trust this workflow for it: - `npm trust github @twigpine/zero --repo Twigpine/zero --file release-artifacts.yml --env npm-publish --allow-publish`. -3. Point the old package's trusted publisher at this repository too, for the - bridge publish: `npm trust list @gitlawb/zero`, revoke the existing entry - with `npm trust revoke @gitlawb/zero --id `, then run the same - `npm trust github` command for `@gitlawb/zero`. -4. Confirm Settings → Environments → `npm-publish` has a required reviewer. - The environment exists either way; without a reviewer the job does not - wait for approval. - -After the release, once both packages show 0.10.0 as `latest`: - -1. `npm deprecate @gitlawb/zero "Zero moved to @twigpine/zero. Switch with: npm uninstall -g @gitlawb/zero && npm install -g @twigpine/zero"` - (no version range, so every version, including the platform payloads, - carries it). Never unpublish: existing installs resolve their platform - payload through these versions. -2. Revoke the old package's trusted publisher (`npm trust revoke`), so - nothing can publish under that name again. -3. Delete the two `@gitlawb/zero` steps from `release-artifacts.yml`. The Go - and wrapper handling of the old name can stay; it only activates in a - build installed under that name. +`@gitlawb/zero` is deprecated on every version and no longer has a trusted +publisher, so this release workflow cannot publish it again; another version +would take a maintainer publishing by hand. Its versions stay on the +registry: existing installs resolve their platform payload through them, so +they must never be unpublished. ## Runbook: bumping the vendored helpers diff --git a/internal/npmwrapper/publish_test.go b/internal/npmwrapper/publish_test.go index af0e2fc39..ced669fb5 100644 --- a/internal/npmwrapper/publish_test.go +++ b/internal/npmwrapper/publish_test.go @@ -23,6 +23,10 @@ import ( // FAKE_NPM_CLOBBER_LATEST=1 makes every publish take `latest`, the failure the // script must stop on. FAKE_NPM_VERSION_LOOKUP_ERRORS= fails the first n // version lookups with a network error instead of answering them. +// FAKE_NPM_APPLY_AFTER= applies each publish only after n further lookups, +// the way npm accepts an upload and shows the version minutes later; until +// then the version and its dist-tag are invisible. When a wrapper is published, +// the aliased versions not yet visible are recorded in missingAtWrapperPublish. const fakeNpm = `#!/usr/bin/env node const fs = require('fs'); const stateFile = process.env.FAKE_NPM_STATE; @@ -31,7 +35,17 @@ const args = process.argv.slice(2); fs.appendFileSync(stateFile + '.log', JSON.stringify({ args, cwd: process.cwd() }) + '\n'); const save = () => fs.writeFileSync(stateFile, JSON.stringify(state)); const notFound = (detail) => { process.stderr.write('npm error code E404\nnpm error 404 ' + detail + '\n'); process.exit(1); }; +state.pending = state.pending || []; +const apply = (publish) => { + state.versions.push(publish.version); + state.distTags[publish.tag] = publish.version; + if (process.env.FAKE_NPM_CLOBBER_LATEST === '1') state.distTags.latest = publish.version; +}; if (args[0] === 'view') { + for (const publish of state.pending) publish.wait -= 1; + state.pending.filter((publish) => publish.wait <= 0).forEach(apply); + state.pending = state.pending.filter((publish) => publish.wait > 0); + save(); const [spec, field] = args.slice(1); const at = spec.lastIndexOf('@'); const name = at > 0 ? spec.slice(0, at) : spec; @@ -62,14 +76,19 @@ if (args[0] === 'publish') { process.stderr.write('fake npm: refusing to publish ' + pkg.name + '\n'); process.exit(1); } - if (state.versions.includes(pkg.version)) { + if (state.versions.includes(pkg.version) || state.pending.some((publish) => publish.version === pkg.version)) { process.stderr.write('npm error code EPUBLISHCONFLICT\n'); process.exit(1); } + if (pkg.optionalDependencies) { + state.missingAtWrapperPublish = Object.values(pkg.optionalDependencies) + .map((spec) => spec.slice(spec.lastIndexOf('@') + 1)) + .filter((version) => !state.versions.includes(version)); + } const tagIndex = args.indexOf('--tag'); - state.versions.push(pkg.version); - state.distTags[tagIndex === -1 ? 'latest' : args[tagIndex + 1]] = pkg.version; - if (process.env.FAKE_NPM_CLOBBER_LATEST === '1') state.distTags.latest = pkg.version; + const publish = { version: pkg.version, tag: tagIndex === -1 ? 'latest' : args[tagIndex + 1], wait: Number(process.env.FAKE_NPM_APPLY_AFTER || 0) }; + if (publish.wait > 0) state.pending.push(publish); + else apply(publish); save(); process.exit(0); } @@ -88,6 +107,9 @@ type fakeRegistryState struct { DistTags map[string]string `json:"distTags"` // VersionLookups counts `npm view @ version` calls. VersionLookups int `json:"versionLookups,omitempty"` + // MissingAtWrapperPublish lists the platform versions the wrapper aliased + // that npm did not show yet when the wrapper was published. + MissingAtWrapperPublish []string `json:"missingAtWrapperPublish,omitempty"` } type npmCall struct { @@ -325,3 +347,87 @@ func TestPublishPackagesFailsWhenTheRegistryCannotBeReached(t *testing.T) { t.Fatalf("stderr does not explain the failure or show npm's error: %s", run.stderr) } } + +// npm accepts an upload and shows the version minutes later (ten minutes for a +// 0.10.0 platform payload). Publishing the wrapper as soon as the platform +// uploads return would leave installs resolving aliases that 404, and checking +// `latest` right after the wrapper upload reports a failure that is not one. +func TestPublishPackagesWaitsForNpmToApplyEachPublish(t *testing.T) { + platforms := []string{"darwin-arm64", "linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, "FAKE_NPM_APPLY_AFTER=8") + if run.exitCode != 0 { + t.Fatalf("exit = %d while npm applied the publishes late; stdout=%s stderr=%s", run.exitCode, run.stdout, run.stderr) + } + if len(run.state.MissingAtWrapperPublish) != 0 { + t.Fatalf("the wrapper went out while npm did not show %v yet; installs would resolve aliases that 404", run.state.MissingAtWrapperPublish) + } + if latest := run.state.DistTags["latest"]; latest != publishFixtureVersion { + t.Fatalf("latest = %q, want %q", latest, publishFixtureVersion) + } +} + +// A publish npm never applies must fail the job at the deadline, before the +// wrapper goes out, instead of waiting forever. +func TestPublishPackagesGivesUpWhenNpmNeverAppliesAPublish(t *testing.T) { + platforms := []string{"linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, + "FAKE_NPM_APPLY_AFTER=1000000", "ZERO_NPM_PUBLISH_WAIT_TIMEOUT=1") + if run.exitCode == 0 { + t.Fatalf("exit = 0 although npm never showed the platform version; stdout=%s", run.stdout) + } + if got := strings.Join(run.publishedDirs(), ","); got != "zero-linux-x64" { + t.Fatalf("published %s, want only the platform payload (the wrapper must wait for it)", got) + } + if !strings.Contains(run.stderr, "still does not show "+publishFixtureName+"@"+publishFixtureVersion+"-linux-x64") { + t.Fatalf("stderr does not say what npm never showed: %s", run.stderr) + } +} + +// Bash arithmetic reads a leading zero as octal and rejects 08 outright, so +// the timeout has to be converted explicitly rather than used as typed. The +// rejection does not stop a script: it prints an error, leaves the deadline +// unset and carries on, so the failure only shows once the script has to wait, +// after the platform payloads are out. npm applying late makes it wait here. +func TestPublishPackagesReadsTheWaitTimeoutAsDecimal(t *testing.T) { + platforms := []string{"linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, + "ZERO_NPM_PUBLISH_WAIT_TIMEOUT=08", "FAKE_NPM_APPLY_AFTER=3") + if run.exitCode != 0 { + t.Fatalf("exit = %d with ZERO_NPM_PUBLISH_WAIT_TIMEOUT=08; stdout=%s stderr=%s", run.exitCode, run.stdout, run.stderr) + } + if got := strings.Join(run.publishedDirs(), ","); got != "zero-linux-x64,wrapper" { + t.Fatalf("published %s, want zero-linux-x64,wrapper", got) + } +} + +// A setting the script cannot use must stop it before anything is published: +// a negative timeout would otherwise end the wait before it starts, and a bad +// delay would only fail at the first sleep, after the platform payloads. +func TestPublishPackagesRejectsAnInvalidWaitSetting(t *testing.T) { + for _, tc := range []struct{ variable, value string }{ + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "-5"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "abc"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "1e3"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "86401"}, + {"ZERO_NPM_PUBLISH_WAIT_TIMEOUT", "9999999999999999999999"}, + {"ZERO_NPM_PUBLISH_RETRY_DELAY", "soon"}, + } { + t.Run(tc.variable+"="+tc.value, func(t *testing.T) { + platforms := []string{"linux-x64"} + npmDir := writePublishFixture(t, platforms, platforms) + run := runPublishPackages(t, npmDir, fakeRegistryState{Name: publishFixtureName}, tc.variable+"="+tc.value) + if run.exitCode == 0 { + t.Fatalf("exit = 0 with %s=%s; stdout=%s", tc.variable, tc.value, run.stdout) + } + if len(run.publishes) != 0 { + t.Fatalf("published %v before rejecting %s=%s, want nothing", run.publishedDirs(), tc.variable, tc.value) + } + if !strings.Contains(run.stderr, tc.variable) { + t.Fatalf("stderr does not name %s: %s", tc.variable, run.stderr) + } + }) + } +} diff --git a/internal/update/installmethod.go b/internal/update/installmethod.go index d1e738c6b..1c6c0575c 100644 --- a/internal/update/installmethod.go +++ b/internal/update/installmethod.go @@ -17,11 +17,11 @@ import ( const npmPackageName = "@twigpine/zero" // legacyNpmPackageName is the name zero was published under on npm up to -// 0.9.0. The 0.10.0 release is published once more under it (the legacy step -// in .github/workflows/release-artifacts.yml), so that 0.9.0's `zero upgrade` -// and anyone still installing the old name land on a build that says where -// the package went. Nothing is published under it after that, which is why an -// install of it is told to switch instead of offered an update. +// 0.9.0. The 0.10.0 release was published once more under it, so that 0.9.0's +// `zero upgrade` and anyone still installing the old name land on a build that +// says where the package went (docs/NPM_PACKAGING.md). Nothing is published +// under it after that, which is why an install of it is told to switch instead +// of offered an update. const legacyNpmPackageName = "@gitlawb/zero" // InstallMethod identifies how the running zero binary was installed. diff --git a/scripts/npm/build-platform-packages.mjs b/scripts/npm/build-platform-packages.mjs index d2cb34068..60d7e93be 100644 --- a/scripts/npm/build-platform-packages.mjs +++ b/scripts/npm/build-platform-packages.mjs @@ -25,10 +25,11 @@ // CI must always assemble the full matrix, and the wrapper's // optionalDependencies always reference the full matrix regardless). // -// --package-name assembles the same artifacts under another name. It exists -// for the one final release under the pre-rename name @gitlawb/zero (see -// docs/NPM_PACKAGING.md); everything in the payloads that names the package -// follows it, so the binary and wrapper know which name they were installed as. +// --package-name assembles the same artifacts under another name. It was +// added for the one release (0.10.0) also published under the pre-rename name +// @gitlawb/zero (see docs/NPM_PACKAGING.md); everything in the payloads that +// names the package follows it, so the binary and wrapper know which name they +// were installed as. // // Usage: // node scripts/npm/build-platform-packages.mjs \ diff --git a/scripts/npm/publish-packages.sh b/scripts/npm/publish-packages.sh index 84c88e4db..338b153b5 100755 --- a/scripts/npm/publish-packages.sh +++ b/scripts/npm/publish-packages.sh @@ -4,8 +4,15 @@ # with the dist-tags docs/NPM_PACKAGING.md requires: # # 1. every platform payload, under the `platform` dist-tag, never `latest`; -# 2. check that `latest` was not moved onto a platform version; -# 3. the wrapper, which takes `latest`, then confirm `latest` is its version. +# 2. wait until npm shows every platform version, then check that `latest` +# was not moved onto one; +# 3. the wrapper, which takes `latest`, then wait until `latest` is its +# version. +# +# The waits exist because npm applies a publish asynchronously: `npm publish` +# returns once the upload is accepted, and the version can appear minutes +# later (ten minutes for a platform payload of 0.10.0). Ordering the uploads +# alone would still let the wrapper appear first, with aliases that 404. # # The package name comes from the assembled wrapper's package.json, so the # same script publishes whatever name the payloads were built under. @@ -19,7 +26,9 @@ # Usage: scripts/npm/publish-packages.sh # # Auth comes from the environment (npm OIDC trusted publishing in CI). -# ZERO_NPM_PUBLISH_RETRY_DELAY overrides the seconds between dist-tag lookups. +# ZERO_NPM_PUBLISH_RETRY_DELAY overrides the seconds between registry lookups, +# and ZERO_NPM_PUBLISH_WAIT_TIMEOUT the seconds to wait for npm to apply the +# publishes (default 1800). set -euo pipefail fail() { @@ -39,7 +48,24 @@ manifest_field() { name="$(manifest_field "$wrapper_dir" name)" version="$(manifest_field "$wrapper_dir" version)" [ -n "$name" ] && [ -n "$version" ] || fail "${wrapper_dir}/package.json has no name or version" -delay="${ZERO_NPM_PUBLISH_RETRY_DELAY:-5}" +# whole_seconds : the value as a decimal number of +# seconds. Checked here, before anything is published: Bash arithmetic reads a +# leading zero as octal (0600 would wait 384 seconds, and 08 is an error), a +# negative timeout would end the wait before it starts, and a bad delay would +# only fail at the first sleep, possibly after the platform payloads went out. +whole_seconds() { + case "$2" in + '' | *[!0-9]*) fail "$1 must be a whole number of seconds, got '$2'" ;; + esac + # Bounded before the conversion so it cannot overflow. + [ "${#2}" -le 18 ] || fail "$1 must be at most $3 seconds, got '$2'" + local seconds=$((10#$2)) + [ "$seconds" -le "$3" ] || fail "$1 must be at most $3 seconds, got '$2'" + echo "$seconds" +} + +delay="$(whole_seconds ZERO_NPM_PUBLISH_RETRY_DELAY "${ZERO_NPM_PUBLISH_RETRY_DELAY:-5}" 600)" +wait_timeout="$(whole_seconds ZERO_NPM_PUBLISH_WAIT_TIMEOUT "${ZERO_NPM_PUBLISH_WAIT_TIMEOUT:-1800}" 86400)" # The wrapper pins every platform version exactly, so a wrapper whose aliases # do not match the payloads beside it would install aliases that 404. Check @@ -105,16 +131,49 @@ publish() { (cd "$dir" && npm publish --access public --ignore-scripts "$@") } +# Unlike already_published, these lookups may fail without stopping the job: +# while npm has not applied a publish, an error or an E404 both mean "not +# yet", and the deadline bounds the wait. +version_visible() { + [ "$(npm view "${name}@$1" version 2>/dev/null || true)" = "$1" ] +} + +latest_is() { + [ "$(npm view "$name" dist-tags.latest 2>/dev/null || true)" = "$1" ] +} + +# One deadline covers every wait in this run, so the job cannot wait +# wait_timeout once per version. +deadline=$((SECONDS + wait_timeout)) + +# wait_until +wait_until() { + local what="$1" + shift + until "$@"; do + if [ "$SECONDS" -ge "$deadline" ]; then + fail "npm accepted the upload but still does not show ${what} after ${wait_timeout}s. Re-run once it does: versions already on the registry are skipped." + fi + sleep "$delay" + done +} + for dir in "${platform_dirs[@]}"; do dir="${dir%/}" publish "$dir" "$(manifest_field "$dir" version)" --tag platform done +for dir in "${platform_dirs[@]}"; do + platform_version="$(manifest_field "${dir%/}" version)" + wait_until "${name}@${platform_version}" version_visible "$platform_version" + echo "${name}@${platform_version} is on the registry." +done + # Platform versions are semver prereleases of the wrapper version, so a # platform publish that took `latest` would make a plain install deliver a # payload with no bin. Stop before the wrapper goes out. A failed lookup is -# tolerated (registry lag, or no `latest` yet on a package's first release): -# failing here would strand the platform payloads without their wrapper. +# tolerated (no `latest` yet on a package's first release): failing here would +# strand the platform payloads without their wrapper. latest="$(npm view "$name" dist-tags.latest || true)" case "$latest" in "${version}-"*) @@ -125,13 +184,5 @@ echo "dist-tag latest is ${latest:-unset} (unchanged by the platform publishes). publish "$wrapper_dir" "$version" -# Registry metadata can lag a publish by a few seconds. -for _ in 1 2 3 4 5; do - latest="$(npm view "$name" dist-tags.latest || true)" - if [ "$latest" = "$version" ]; then - echo "dist-tag latest is ${version}." - exit 0 - fi - sleep "$delay" -done -fail "dist-tag latest of ${name} is ${latest:-unset}, expected ${version} after the wrapper publish." +wait_until "${name}@${version} as the latest dist-tag" latest_is "$version" +echo "dist-tag latest is ${version}."