diff --git a/.github/workflows/publish-one.yml b/.github/workflows/publish-one.yml index 82a026a0..1ef6b4b3 100644 --- a/.github/workflows/publish-one.yml +++ b/.github/workflows/publish-one.yml @@ -56,18 +56,12 @@ jobs: - name: Build the browser bundle run: deno task build:web + # One attempt. The builder constructs this package's internal closure from + # this checkout, so nothing here waits on npm indexing a sibling the + # previous job published — the wait the retry loop used to bound, and + # never reliably did (#843). - name: Build with dnt - # Retry to absorb npm propagation of a just-published sibling dependency. - run: | - set -eu - for attempt in 1 2 3 4; do - if deno run -A scripts/build-npm.ts "${{ inputs.package }}" "${{ inputs.version }}"; then - exit 0 - fi - [ "$attempt" -lt 4 ] && sleep 15 || true - done - echo "build failed after 4 attempts" >&2 - exit 1 + run: deno run -A scripts/build-npm.ts "${{ inputs.package }}" "${{ inputs.version }}" # Idempotent: skip a version npm already has, so tag re-runs succeed. - name: Publish to npm diff --git a/scripts/build-npm.ts b/scripts/build-npm.ts index df634e3e..7f3b2484 100644 --- a/scripts/build-npm.ts +++ b/scripts/build-npm.ts @@ -11,14 +11,30 @@ * Everything published is derived from the member's own deno.json (name, * exports) and package.json (dependencies, description, bin) — those manifests * are the single source of truth. Internal @executablemd siblings are declared - * as external npm dependencies (resolved to the sibling's own version), never - * inlined, so each published package resolves them from npm. + * as external npm dependencies, never inlined. * - * `DNT_LOCAL_SIBLINGS=1` builds each internal sibling first and depends on those - * artifacts by path instead of by published version, so a branch can build and - * type-check against its own workspace sources. The resulting package.json names - * local directories and is therefore unpublishable; release workflows never set - * the variable. + * A build happens in two phases, and the split is what keeps a release off the + * registry's clock. + * + * **Phase 1 builds the local closure.** The requested package's internal + * dependencies are built first, depth-first, each at most once, and handed to + * dnt as absolute `file:` ranges pointing at the artifacts this same invocation + * produced. Nothing asks npm for a package from the same release. + * + * **Phase 2 finalizes every manifest together**, once the last dnt call has + * returned, replacing each internal `file:` range with the sibling's + * `^`. It has to be every manifest at once: rewriting B when its own + * dnt call returns leaves it describing a dependency only the registry could + * supply, and whether A survives that then rests on how npm installs a local + * directory and on what B's own build left in its `node_modules` — no cost + * under the default symlink layout, a failed install under + * `install-links=true`. Finalizing the closure at the end depends on neither. + * No install or typecheck runs after finalization begins, and a surviving local + * reference fails the build rather than reaching `npm publish`. + * + * `DNT_SKIP_INSTALL=1` still skips the install and typecheck for a leaf package + * that declares no `workspace:*` dependency, for exercising the tooling. It + * refuses anything else, and release workflows never set it. */ import { ensure, exit, main, scoped, until } from "effection"; @@ -126,36 +142,69 @@ interface WorkspaceMember { version: string; } +/** + * What one invocation reports as it runs. Diagnostic observation only: an + * observer chooses no build, no dependency resolution and no finalization + * policy, which is what keeps the regression watching the real path instead of + * a second one. + */ +export type BuildEvent = + | { + readonly type: "package-build-started"; + readonly package: string; + /** Exactly what dnt was handed, so a test can see the local ranges. */ + readonly dependencies: Readonly>; + } + | { readonly type: "package-build-completed"; readonly package: string } + | { readonly type: "closure-finalization-started" } + | { readonly type: "package-manifest-finalized"; readonly package: string }; + +export interface BuildNpmOptions { + /** The workspace root the closure is built from. */ + repoRoot: URL; + /** The requested member's directory, e.g. `packages/cli`. */ + package: string; + /** The npm version for the requested artifact; siblings use their own. */ + version: string; + observe?: (event: BuildEvent) => Operation; +} + +/** One package this invocation built, awaiting closure-wide finalization. */ +interface BuiltArtifact { + name: string; + dir: string; + /** Internal dependency name -> the workspace version to finalize it to. */ + internal: Record; +} + interface BuildContext { repoRoot: URL; rootDeno: z.infer; members: Record; - /** Depend on locally built sibling artifacts instead of published versions. */ - localSiblings: boolean; skipInstall: boolean; - /** Package names already built in this process, so a diamond builds once. */ - built: Set; + /** Artifacts already built in this process, so a diamond builds once. */ + built: Map; + observe: (event: BuildEvent) => Operation; } -await main(function* (args) { - const pkgArg = args[0]; - const version = args[1] ?? "0.0.0-dev"; - - if (!pkgArg) { - console.error("usage: build-npm.ts [version]"); - yield* exit(1); - return; - } +/** + * npm resolves a relative `file:` against the dependent's own location, which + * differs for a sibling installed under another package's node_modules, so the + * range this invocation hands dnt is absolute. + */ +function localRange(repoRoot: URL, dir: string): string { + return `file:${fromFileUrl(new URL(`${dir}/npm`, repoRoot))}`; +} - // Before anything is emitted. Copying the documentation assets is not the - // same as validating them: a package built from a set that has drifted from - // the components it documents would install cleanly and refuse the first time - // somebody asked it for documentation. The same assembly the run profile uses - // runs here, so a missing, unknown or duplicated section fails the build for - // exactly the reason it would fail a run. - yield* validateDocumentation(); +function* observeNothing(): Operation {} - const repoRoot = new URL("../", import.meta.url); +/** + * Build `options.package` and its internal closure, and leave every generated + * manifest publishable. This is the whole builder; the command below is an + * adapter over it, so the regression exercises the release path itself. + */ +export function* buildNpmPackage(options: BuildNpmOptions): Operation { + const { repoRoot } = options; const rootDeno = RootDenoSchema.parse( JSON.parse(yield* readTextFile(new URL("deno.json", repoRoot))), @@ -175,15 +224,18 @@ await main(function* (args) { } } - yield* buildPackage(pkgArg, version, { + const ctx: BuildContext = { repoRoot, rootDeno, members, - localSiblings: Deno.env.get("DNT_LOCAL_SIBLINGS") === "1", skipInstall: Deno.env.get("DNT_SKIP_INSTALL") === "1", - built: new Set(), - }); -}); + built: new Map(), + observe: options.observe ?? observeNothing, + }; + + yield* buildPackage(options.package, options.version, ctx); + yield* finalizeClosure(ctx); +} function* buildPackage(pkgArg: string, version: string, ctx: BuildContext): Operation { const { repoRoot, rootDeno, skipInstall } = ctx; @@ -200,9 +252,10 @@ function* buildPackage(pkgArg: string, version: string, ctx: BuildContext): Oper ); // Dependencies come from package.json verbatim, except internal siblings - // (workspace:* protocol) which resolve to the sibling's own version range — - // or, with local siblings, to the artifact this process just built for it. + // (workspace:* protocol), which are built first and named by the artifact + // this invocation just produced. Phase 2 turns those into version ranges. const dependencies: Record = {}; + const internal: Record = {}; for (const [name, range] of Object.entries(packageJson.dependencies ?? {})) { if (!name.startsWith(INTERNAL_SCOPE)) { dependencies[name] = range; @@ -210,19 +263,17 @@ function* buildPackage(pkgArg: string, version: string, ctx: BuildContext): Oper } const member = ctx.members[name]; if (!member) { - throw new Error(`no workspace version found for internal dependency "${name}"`); - } - if (!ctx.localSiblings) { - dependencies[name] = `^${member.version}`; - continue; + // No registry fallback: reaching npm for an unmapped internal name is how + // a misspelled or removed member would silently restore the release race. + throw new Error( + `"${denoJson.name}" depends on internal package "${name}", which is not a workspace member`, + ); } if (!ctx.built.has(name)) { yield* buildPackage(member.dir, member.version, ctx); } - // An absolute path: npm resolves a relative `file:` against the dependent's - // own location, which differs for a sibling installed under another - // package's node_modules. - dependencies[name] = `file:${fromFileUrl(new URL(`${member.dir}/npm`, repoRoot))}`; + internal[name] = member.version; + dependencies[name] = localRange(repoRoot, member.dir); } // Library entry points come from deno.json exports. An executable comes from @@ -311,6 +362,8 @@ function* buildPackage(pkgArg: string, version: string, ctx: BuildContext): Oper ), ); + yield* ctx.observe({ type: "package-build-started", package: denoJson.name, dependencies }); + // The build tree is removed when this scope closes, before the finished // package is completed below. yield* scoped(function* () { @@ -323,10 +376,10 @@ function* buildPackage(pkgArg: string, version: string, ctx: BuildContext): Oper importMap: join(srcCopy, "deno.json"), shims: { deno: false }, test: false, - // Internal @executablemd deps are published tier-by-tier, so a downstream - // package's siblings are already on npm when it builds in CI. For local - // builds (before siblings are published) set DNT_SKIP_INSTALL=1 to skip - // the npm install + type check that would otherwise 404 on them. + // The install resolves internal siblings from the artifacts phase 1 + // already built, so it never reaches npm for a package from this + // release. DNT_SKIP_INSTALL=1 drops the install and typecheck entirely, + // which only a leaf package may ask for (refused above). skipNpmInstall: skipInstall, typeCheck: skipInstall ? false : "single", declaration: "separate", @@ -383,8 +436,153 @@ function* buildPackage(pkgArg: string, version: string, ctx: BuildContext): Oper yield* copyFile(new URL(asset, pkgDir), target); } - ctx.built.add(denoJson.name); - const provenance = - ctx.localSiblings && workspaceDeps.length > 0 ? " (local siblings — not publishable)" : ""; - console.log(`built ${denoJson.name}@${version} -> ${pkgArg}/npm${provenance}`); + ctx.built.set(denoJson.name, { name: denoJson.name, dir: pkgArg, internal }); + yield* ctx.observe({ type: "package-build-completed", package: denoJson.name }); + console.log(`built ${denoJson.name}@${version} -> ${pkgArg}/npm`); +} + +/** The dependency maps npm reads, so validation misses none of them. */ +const DEPENDENCY_FIELDS = [ + "dependencies", + "devDependencies", + "peerDependencies", + "optionalDependencies", +]; + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** Every string anywhere in `value`, so a workspace path cannot hide in a field nobody checks. */ +function* strings(value: unknown): Generator { + if (typeof value === "string") { + yield value; + return; + } + if (Array.isArray(value)) { + for (const item of value) { + yield* strings(item); + } + return; + } + if (isRecord(value)) { + for (const item of Object.values(value)) { + yield* strings(item); + } + } +} + +/** + * Why `manifest` cannot be published, or `undefined` when it can. + * + * A release gate rather than cleanup. A known internal local range is rewritten + * by the caller before this runs; anything local still standing is an + * unexpected dependency, and normalizing it away would be the one edit that + * makes an unpublishable artifact look fine. + */ +function unpublishable(manifest: unknown, repoRoot: URL): string | undefined { + if (!isRecord(manifest)) { + return "it is not an object"; + } + + for (const field of DEPENDENCY_FIELDS) { + const map = manifest[field]; + if (!isRecord(map)) { + continue; + } + for (const [name, range] of Object.entries(map)) { + if ( + typeof range === "string" && + (range.startsWith("workspace:") || range.startsWith("file:")) + ) { + return `${field}["${name}"] is the local range ${range}`; + } + } + } + + const nativeRoot = fromFileUrl(repoRoot).replace(new RegExp(`${sep}$`), ""); + const urlRoot = repoRoot.href.replace(/\/$/, ""); + for (const value of strings(manifest)) { + if (value.includes(nativeRoot)) { + return `it names the workspace path ${nativeRoot}`; + } + if (value.includes(urlRoot)) { + return `it names the workspace URL ${urlRoot}`; + } + } + + return undefined; +} + +/** + * Phase 2. Every artifact this invocation produced becomes publishable at once, + * after the last dnt call returned — see the two-phase note at the top of this + * file for why it cannot happen package by package. + */ +function* finalizeClosure(ctx: BuildContext): Operation { + yield* ctx.observe({ type: "closure-finalization-started" }); + + const candidates: Array<{ name: string; url: URL; manifest: Record }> = []; + + for (const artifact of ctx.built.values()) { + const url = new URL(`${artifact.dir}/npm/package.json`, ctx.repoRoot); + const manifest: unknown = JSON.parse(yield* readTextFile(url)); + if (!isRecord(manifest)) { + throw new Error(`${artifact.name}'s generated package.json is not an object`); + } + for (const field of DEPENDENCY_FIELDS) { + const map = manifest[field]; + if (!isRecord(map)) { + continue; + } + for (const [name, version] of Object.entries(artifact.internal)) { + const member = ctx.members[name]; + if (member && map[name] === localRange(ctx.repoRoot, member.dir)) { + map[name] = `^${version}`; + } + } + } + candidates.push({ name: artifact.name, url, manifest }); + } + + // Every manifest is judged before any is written, so a closure that cannot be + // published in full is never half-published. + for (const candidate of candidates) { + const refusal = unpublishable(candidate.manifest, ctx.repoRoot); + if (refusal !== undefined) { + throw new Error(`${candidate.name} cannot be published: ${refusal}`); + } + } + + for (const candidate of candidates) { + yield* writeTextFile(candidate.url, `${JSON.stringify(candidate.manifest, null, 2)}\n`); + yield* ctx.observe({ type: "package-manifest-finalized", package: candidate.name }); + } +} + +if (import.meta.main) { + await main(function* (args) { + const pkgArg = args[0]; + const version = args[1] ?? "0.0.0-dev"; + + if (!pkgArg) { + console.error("usage: build-npm.ts [version]"); + yield* exit(1); + return; + } + + // Before anything is emitted. Copying the documentation assets is not the + // same as validating them: a package built from a set that has drifted from + // the components it documents would install cleanly and refuse the first + // time somebody asked it for documentation. The same assembly the run + // profile uses runs here, so a missing, unknown or duplicated section fails + // the build for exactly the reason it would fail a run. + yield* validateDocumentation(); + + yield* buildNpmPackage({ + repoRoot: new URL("../", import.meta.url), + package: pkgArg, + version, + }); + }); } diff --git a/scripts/tests/adapter-npm-package.test.ts b/scripts/tests/adapter-npm-package.test.ts index bb406f65..1277d548 100644 --- a/scripts/tests/adapter-npm-package.test.ts +++ b/scripts/tests/adapter-npm-package.test.ts @@ -67,7 +67,6 @@ function useBuiltPackage(): Operation { // cache and npm configuration, and a bare pair would drop both. env: { ...inheritedEnvironment(), - DNT_LOCAL_SIBLINGS: "1", npm_config_legacy_peer_deps: "true", }, }); diff --git a/scripts/tests/build-npm.test.ts b/scripts/tests/build-npm.test.ts index 3f2aaf3b..1aec03e3 100644 --- a/scripts/tests/build-npm.test.ts +++ b/scripts/tests/build-npm.test.ts @@ -3,9 +3,13 @@ import { expect } from "@executablemd/test-support/expect"; import { ensure } from "effection"; import type { Operation } from "effection"; import { exec, Stdio } from "@effectionx/process"; -import { exists, readTextFile, rm } from "@effectionx/fs"; +import { ensureDir, exists, readTextFile, rm, writeTextFile } from "@effectionx/fs"; +import { useTempDirectory } from "@executablemd/test-support/temp"; import path from "node:path"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +import { buildNpmPackage } from "../build-npm.ts"; +import type { BuildEvent } from "../build-npm.ts"; const ROOT = fileURLToPath(new URL("../../", import.meta.url)); @@ -110,3 +114,319 @@ describe("build-npm skip-install mode", () => { } }); }); + +/** + * A version no `@executablemd/fixture-*` package has on npm, so nothing here + * resolves unless the artifact beside it was the one consumed. + */ +const FIXTURE_VERSION = "9.9.9-closure"; + +function scoped(name: string): string { + return `@executablemd/fixture-${name}`; +} + +const C_SOURCE = `export interface CValue {\n readonly c: string;\n}\nexport const c: CValue = { c: "c" };\n`; + +function* member( + root: URL, + name: string, + dependencies: Record, + source: string, +): Operation { + yield* ensureDir(new URL(`packages/${name}/`, root)); + yield* writeTextFile( + new URL(`packages/${name}/deno.json`, root), + `${JSON.stringify({ name: scoped(name), version: FIXTURE_VERSION, exports: "./mod.ts" }, null, 2)}\n`, + ); + yield* writeTextFile( + new URL(`packages/${name}/package.json`, root), + `${JSON.stringify( + { name: scoped(name), version: FIXTURE_VERSION, type: "module", dependencies }, + null, + 2, + )}\n`, + ); + yield* writeTextFile(new URL(`packages/${name}/mod.ts`, root), source); +} + +/** + * `a -> b -> c`, in a workspace of its own. Never the repository's members: the + * point is a closure whose versions npm has never seen. + */ +function* closureWorkspace(): Operation { + const base = yield* useTempDirectory("build-npm-closure-"); + const root = pathToFileURL(`${base}/`); + + yield* writeTextFile( + new URL("deno.json", root), + `${JSON.stringify({ workspace: ["packages/*"], imports: {} }, null, 2)}\n`, + ); + + // `b` takes a type from `c`, so `b`'s own typecheck resolves `c` through the + // artifact phase 1 built for it. A chain of plain values would compile even + // if `c` were never consumed as a package at all. + yield* member(root, "c", {}, C_SOURCE); + yield* member( + root, + "b", + { [scoped("c")]: "workspace:*" }, + `import type { CValue } from "${scoped("c")}";\nimport { c } from "${scoped( + "c", + )}";\nexport const b: CValue = c;\n`, + ); + yield* member( + root, + "a", + { [scoped("b")]: "workspace:*" }, + `import { b } from "${scoped("b")}";\nexport const a = b.c + "a";\n`, + ); + + return root; +} + +function* manifestOf(root: URL, name: string): Operation> { + const text = yield* readTextFile(new URL(`packages/${name}/npm/package.json`, root)); + return JSON.parse(text).dependencies ?? {}; +} + +function localRange(root: URL, name: string): string { + return `file:${fileURLToPath(new URL(`packages/${name}/npm`, root))}`; +} + +/** + * npm's supported packed-dependency layout, for the length of one case. + * + * By default npm symlinks a directory `file:` dependency, so a dependent can + * reach whatever the sibling's own build left in its `node_modules` — which + * hides what a finalized sibling would actually cost. `install-links=true` + * packs and installs it as an ordinary dependency instead, so the sibling's own + * manifest is the only thing that says where its dependencies come from. + * + * The cache and registry are invocation-private, and the registry is + * unreachable, so a range that has to be resolved fails here rather than + * depending on what npmjs.org happens to answer. The environment is restored + * however the case ends: every other build in this file is the ordinary one. + */ +function* usePackedLocalDependencies(): Operation { + const cache = yield* useTempDirectory("npm-cache-"); + const scopedEnvironment: Record = { + NPM_CONFIG_INSTALL_LINKS: "true", + NPM_CONFIG_CACHE: cache, + NPM_CONFIG_REGISTRY: "http://127.0.0.1:1/", + NPM_CONFIG_AUDIT: "false", + NPM_CONFIG_FUND: "false", + // The unreachable registry is the expected outcome here, not a flake worth + // waiting out; npm's default retries would spend minutes proving it. + NPM_CONFIG_FETCH_RETRIES: "0", + }; + + const restore = new Map( + Object.keys(scopedEnvironment).map((key) => [key, Deno.env.get(key)]), + ); + // Registered before anything is set, so a halt between the two still restores. + yield* ensure(() => { + for (const [key, value] of restore) { + if (value === undefined) { + Deno.env.delete(key); + } else { + Deno.env.set(key, value); + } + } + }); + for (const [key, value] of Object.entries(scopedEnvironment)) { + Deno.env.set(key, value); + } +} + +/** + * The builder's own two phases, observed on a closure npm has never published. + * + * The repository's own members cannot tell a local closure apart from a + * registry build, because every version they name is already on npm. These + * fixtures can: nothing here resolves unless the artifact beside it was the one + * consumed. + */ +describe("build-npm local closure", () => { + it("N1: hands dnt the artifacts it just built, for the whole closure", function* () { + const root = yield* closureWorkspace(); + const events: BuildEvent[] = []; + + yield* buildNpmPackage({ + repoRoot: root, + package: "packages/a", + version: FIXTURE_VERSION, + *observe(event) { + events.push(event); + }, + }); + + const started = events.filter((event) => event.type === "package-build-started"); + expect(started.map((event) => event.package)).toEqual([scoped("c"), scoped("b"), scoped("a")]); + expect(started[1].dependencies).toEqual({ [scoped("c")]: localRange(root, "c") }); + expect(started[2].dependencies).toEqual({ [scoped("b")]: localRange(root, "b") }); + }); + + it("N2: finalizes nothing until every build in the closure has returned", function* () { + const root = yield* closureWorkspace(); + const events: BuildEvent[] = []; + const atStartOfA: Record[] = []; + + yield* buildNpmPackage({ + repoRoot: root, + package: "packages/a", + version: FIXTURE_VERSION, + *observe(event) { + events.push(event); + if (event.type === "package-build-started" && event.package === scoped("a")) { + // b is built by now, and must still name c locally. Once b + // describes c by a registry range, whether a's install survives + // depends on npm's layout and on b's own build residue — which is + // exactly what this contract refuses to rest on. + atStartOfA.push(yield* manifestOf(root, "b")); + } + }, + }); + + expect(atStartOfA).toEqual([{ [scoped("c")]: localRange(root, "c") }]); + + const order = events.map((event) => event.type); + const finalization = order.indexOf("closure-finalization-started"); + expect(finalization).toBeGreaterThan(-1); + expect(order.lastIndexOf("package-build-completed")).toBeLessThan(finalization); + expect(order.lastIndexOf("package-build-started")).toBeLessThan(finalization); + expect(order.filter((type) => type === "package-manifest-finalized")).toHaveLength(3); + }); + + it("N3: leaves every manifest in the closure publishable", function* () { + const root = yield* closureWorkspace(); + + yield* buildNpmPackage({ repoRoot: root, package: "packages/a", version: FIXTURE_VERSION }); + + expect(yield* manifestOf(root, "a")).toEqual({ [scoped("b")]: `^${FIXTURE_VERSION}` }); + expect(yield* manifestOf(root, "b")).toEqual({ [scoped("c")]: `^${FIXTURE_VERSION}` }); + expect(yield* manifestOf(root, "c")).toEqual({}); + + const workspacePath = fileURLToPath(root).replace(/\/$/, ""); + for (const name of ["a", "b", "c"]) { + const text = yield* readTextFile(new URL(`packages/${name}/npm/package.json`, root)); + expect({ name, leaked: text.includes(workspacePath) || text.includes("workspace:") }).toEqual( + { name, leaked: false }, + ); + } + }); + + /** + * N4. What an early-finalized `b` costs, once `b` is installed the way a + * published `b` would be. + * + * The positive control comes first: packed local artifacts have to work + * before a failure afterwards means anything. + */ + it("N4: a child finalized before its dependent builds fails that build", function* () { + yield* usePackedLocalDependencies(); + + const sound = yield* closureWorkspace(); + yield* buildNpmPackage({ repoRoot: sound, package: "packages/a", version: FIXTURE_VERSION }); + expect(yield* manifestOf(sound, "b")).toEqual({ [scoped("c")]: `^${FIXTURE_VERSION}` }); + + const broken = yield* closureWorkspace(); + const events: BuildEvent[] = []; + let caught: unknown; + + try { + yield* buildNpmPackage({ + repoRoot: broken, + package: "packages/a", + version: FIXTURE_VERSION, + *observe(event) { + events.push(event); + if (event.type === "package-build-completed" && event.package === scoped("b")) { + const manifest = new URL("packages/b/npm/package.json", broken); + const parsed = JSON.parse(yield* readTextFile(manifest)); + parsed.dependencies[scoped("c")] = `^${FIXTURE_VERSION}`; + yield* writeTextFile(manifest, `${JSON.stringify(parsed, null, 2)}\n`); + } + }, + }); + } catch (error) { + caught = error; + } + + // `a` reached dnt, which is what makes the failure below evidence about the + // mutation rather than about the observer that applied it: an observer that + // threw would leave this list one short. + expect( + events + .filter((event) => event.type === "package-build-started") + .map((event) => event.package), + ).toEqual([scoped("c"), scoped("b"), scoped("a")]); + + // And it failed where a registry range has to be resolved. + expect(caught).toMatchObject({ message: "npm install failed with exit code 1" }); + + // Before `a` completed and before anything was finalized: the closure never + // reaches a state where a half-rewritten set could be mistaken for output. + expect(events.map((event) => event.type)).not.toContain("closure-finalization-started"); + expect( + events + .filter((event) => event.type === "package-build-completed") + .map((event) => event.package), + ).toEqual([scoped("c"), scoped("b")]); + }); + + it("N5: refuses an internal dependency no workspace member declares", function* () { + const root = yield* closureWorkspace(); + yield* member( + root, + "b", + { [scoped("c")]: "workspace:*", [scoped("absent")]: "workspace:*" }, + `import { c } from "${scoped("c")}";\nexport const b = c;\n`, + ); + let caught: unknown; + + try { + yield* buildNpmPackage({ repoRoot: root, package: "packages/a", version: FIXTURE_VERSION }); + } catch (error) { + caught = error; + } + + expect(caught).toMatchObject({ + message: `"${scoped("b")}" depends on internal package "${scoped( + "absent", + )}", which is not a workspace member`, + }); + // Refused before the dependent was built, so no artifact claims otherwise. + expect(yield* exists(new URL("packages/b/npm/package.json", root))).toBe(false); + }); + + it("N5: refuses a publishable manifest that kept an unrelated local dependency", function* () { + const root = yield* closureWorkspace(); + yield* ensureDir(new URL("vendor/local-dep/", root)); + yield* writeTextFile( + new URL("vendor/local-dep/package.json", root), + `${JSON.stringify({ name: "fixture-local-dep", version: "1.0.0", type: "module" }, null, 2)}\n`, + ); + yield* writeTextFile(new URL("vendor/local-dep/index.js", root), "export default {};\n"); + yield* member( + root, + "c", + { "fixture-local-dep": `file:${fileURLToPath(new URL("vendor/local-dep", root))}` }, + C_SOURCE, + ); + let caught: unknown; + + try { + yield* buildNpmPackage({ repoRoot: root, package: "packages/a", version: FIXTURE_VERSION }); + } catch (error) { + caught = error; + } + + // Consumable during dnt, refused at the gate: normalizing it away is what + // would turn an unpublishable artifact into one npm would accept. + expect(caught).toMatchObject({ + message: `${scoped("c")} cannot be published: dependencies["fixture-local-dep"] is the local range file:${fileURLToPath( + new URL("vendor/local-dep", root), + )}`, + }); + }); +}); diff --git a/scripts/tests/cli-npm-bin.test.ts b/scripts/tests/cli-npm-bin.test.ts index a659f872..abe5fbb8 100644 --- a/scripts/tests/cli-npm-bin.test.ts +++ b/scripts/tests/cli-npm-bin.test.ts @@ -5,12 +5,11 @@ * `@executablemd/cli@0.5.0` off npm compiles fine under Deno and fails only * here. * - * The build runs with `DNT_LOCAL_SIBLINGS=1`, so packages/cli and every - * @executablemd sibling it depends on are built from this branch's sources. A - * release build resolves those siblings from npm instead, which type-checks the - * branch against the *previous* release — green until a branch changes a shared - * API, then red for a reason the branch cannot fix. This is also the only - * coverage of the local-sibling build mode. + * The build is the ordinary one a release runs, so packages/cli and every + * @executablemd sibling it depends on are built from this branch's sources and + * consumed as artifacts. Nothing here resolves a sibling from npm, which is + * what used to type-check a branch against the *previous* release — green until + * a branch changed a shared API, then red for a reason the branch could not fix. */ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; @@ -159,9 +158,6 @@ function* buildCliPackage(version: string): Operation { // match against the pinned 4.x prerelease — the same allowance // publish-one.yml makes. NPM_CONFIG_LEGACY_PEER_DEPS: "true", - // Build the siblings from this branch rather than resolving the last - // published versions of them. - DNT_LOCAL_SIBLINGS: "1", }, }).join(); } @@ -450,6 +446,48 @@ describe("npm CLI package", { sanitizeOps: false, sanitizeResources: false }, () * Run from a directory that is not the package, because the lookup must be * beside the module and never beside the caller. */ + /** + * N6. The bin above ran against the closure this build produced, and the same + * closure has to be publishable afterwards — not a verification artifact that + * works locally and names directories npm would reject. + */ + it("leaves every manifest in the closure publishable", function* () { + yield* ensure(removeNpmOutput); + const { version } = yield* readManifest(PKG_DIR, "deno.json"); + const built = yield* buildCliPackage(version ?? "0.0.0-dev"); + if (built.code !== 0) { + throw new Error(`build-npm.ts exited ${built.code}\n${built.stderr}`); + } + + const source = yield* readManifest(PKG_DIR, "package.json"); + const internal = Object.keys(source.dependencies ?? {}).filter((name) => + name.startsWith("@executablemd/"), + ); + // Non-vacuous: the CLI is the deepest closure in the workspace. + expect(internal.length).toBeGreaterThan(1); + + const emitted = yield* readEmittedManifest(); + for (const name of internal) { + expect({ name, range: emitted.dependencies?.[name] }).toEqual({ + name, + range: `^${version}`, + }); + } + + // Every sibling the closure built, not only the root that was requested. + for (const name of internal) { + const dir = path.join("packages", name.slice("@executablemd/".length)); + const sibling = yield* readManifest(dir, "npm", "package.json"); + for (const [dependency, range] of Object.entries(sibling.dependencies ?? {})) { + expect({ sibling: name, dependency, local: range.startsWith("file:") }).toEqual({ + sibling: name, + dependency, + local: false, + }); + } + } + }); + it("reports the same Component identity the source tree ships", function* () { yield* ensure(removeNpmOutput); const { version } = yield* readManifest(PKG_DIR, "deno.json"); diff --git a/scripts/tests/publish-workflow-membership.test.ts b/scripts/tests/publish-workflow-membership.test.ts index 1008a3b4..236fe01c 100644 --- a/scripts/tests/publish-workflow-membership.test.ts +++ b/scripts/tests/publish-workflow-membership.test.ts @@ -238,6 +238,34 @@ describe("publish-packages.yml membership", () => { }); }); +/** + * The reusable publish job builds once. Four attempts with sleeps between them + * were how this repository tolerated npm indexing a sibling the previous job + * had just published; they bounded the race rather than removing it, and three + * consecutive releases lost to it anyway (#843). The builder now constructs the + * closure from the checkout, so a retry here would only be timing-based + * recovery for a wait that no longer exists. + */ +describe("publish-one.yml build step", () => { + it("invokes the builder once, with no propagation retry", function* () { + const commands = (yield* readTextFile(PUBLISH_ONE_WORKFLOW)) + .split("\n") + .filter((line) => !line.trim().startsWith("#")) + .join("\n"); + + const invocations = commands.split("scripts/build-npm.ts").length - 1; + expect(invocations).toBe(1); + + for (const timing of ["sleep", "for attempt", "attempts"]) { + expect({ timing, present: commands.includes(timing) }).toEqual({ timing, present: false }); + } + + // The guard that makes a tag rerun idempotent is a different mechanism and + // stays: removing the retry must not remove it. + expect(commands).toContain("is already on npm — skipping"); + }); +}); + /** * Both workflows refuse a tag the manifests do not declare, and they have to * refuse it for the same set of manifests. v0.13.0 is what it costs when they diff --git a/specs/release-process-spec.md b/specs/release-process-spec.md index 2eaa7c3d..189f38fe 100644 --- a/specs/release-process-spec.md +++ b/specs/release-process-spec.md @@ -165,9 +165,17 @@ documents at the revision it checks. if the binary build fails. It then fans out one `publish-one.yml` call per package, ordered with `needs:` so dependencies publish before dependents (leaves run in parallel), plus one `jsr` job for the whole workspace. + + That ordering is a publication guarantee, not a build one. Each job builds its + own closure from the tag's checkout, so no job waits for another's artifact to + reach npm. What the edges still buy is that a failed upstream publish + withholds every dependent: whatever npm ends up holding is dependency-closed, + and never a dependent whose dependency never published. - **`publish-one.yml`** (`workflow_call`, inputs `package`/`version`): builds one package with dnt (`scripts/build-npm.ts`) and publishes it to npm. Runs in - the `npm-publish` environment. npm publishing is idempotent: it skips an + the `npm-publish` environment. The build is one attempt: it constructs its own + closure from the checkout, so there is no registry propagation left for a + retry to wait out. npm publishing is idempotent: it skips an already-published version. Library entry points come from the member's `deno.json` `exports`; an executable comes from its `package.json` `bin`, so the npm CLI ships `packages/cli/src/node.ts` while JSR gets the Deno @@ -180,28 +188,50 @@ something a package cannot ship, because npm strips `.npmrc` from published tarballs. Verify it on the emitted manifest: after a `scripts/build-npm.ts` run, `packages//npm/package.json` declares no `@jsr/*` dependency. -### Local builds - -A normal build installs the package's dependencies from npm and resolves its -siblings to their published versions. That is the path `publish-one.yml` runs. - -`DNT_SKIP_INSTALL=1` skips that install and the type check, for exercising the -tooling before a version reaches npm. It covers only packages that declare no -`workspace:*` dependencies. dnt emits through TypeScript, which resolves from -the output directory, so the install is what supplies a sibling's declarations; -without it a sibling resolves to its workspace source and lands in the package. -The builder therefore refuses a package that declares one, naming the -dependencies and leaving the output directory empty. Release workflows never set -the variable. - -`DNT_LOCAL_SIBLINGS=1` builds each internal sibling first — depth-first over the -`workspace:*` dependencies, once per package — and depends on those artifacts by -absolute path (`file:/npm`) instead of by published version. The -build therefore type-checks against the sources in the working tree, which is -what a branch changing a shared API needs and what the `packages/cli` npm suite -runs. The emitted package.json names local directories, so an artifact built this -way is a verification artifact, never a publishable one. Release workflows never -set the variable. +### Building a package + +One build serves both purposes, and it runs in two phases. + +**Phase 1 builds the local closure.** The requested package's internal +dependencies are built first — depth-first over the `workspace:*` dependencies, +each at most once, so a diamond shares one artifact — and are handed to dnt as +absolute `file:/npm` ranges naming the artifacts this same +invocation produced. The install and the type check therefore resolve every +sibling from the working tree, which is what a branch changing a shared API +needs, and no build asks npm for a package from its own release. + +**Phase 2 finalizes every manifest together**, once the last dnt call has +returned. Each internal `file:` range becomes the sibling's `^`, taken +from the workspace manifests rather than from the version on the command line. +It is the whole closure at once and not each package as its own build +finishes. A sibling rewritten early describes a dependency only the registry +could supply, and whether the rest of the closure survives that depends on how +npm installs a local directory and on what that sibling's own build left in its +`node_modules`: nothing goes wrong under the default symlink layout, while under +`install-links=true` the dependent's install fails. Finalizing at the end is +what makes the build independent of both. No install or type check runs after +finalization begins. + +**The result is publishable, or the build fails.** Before reporting success the +builder inspects every generated manifest and refuses a dependency range +beginning with `workspace:` or `file:`, and any string naming the checkout's +path or `file:` URL. A known internal range is rewritten; anything else local is +an unexpected dependency, and normalizing it away is exactly the edit that would +make an unpublishable artifact look fine. An internal dependency that no +workspace member declares is refused by name before its dependent is built — +there is no registry fallback, because falling back is how a misspelled member +would quietly restore the wait this design removes. + +There is no separate verification mode: the artifact a developer builds is the +artifact a release publishes. + +`DNT_SKIP_INSTALL=1` skips the install and the type check, for exercising the +tooling. It covers only packages that declare no `workspace:*` dependencies. dnt +emits through TypeScript, which resolves from the output directory, so the +install is what supplies a sibling's declarations; without it a sibling resolves +to its workspace source and lands in the package. The builder therefore refuses +a package that declares one, naming the dependencies and leaving the output +directory empty. Release workflows never set the variable. ### JSR publishing @@ -284,14 +314,17 @@ publish was never established, so nothing here relies on it. GitHub Actions as the package's trusted publisher with the values in §4's table. It never publishes `latest` — the first tagged release does that. -The reservation is empty because the real artifact cannot be the record that -makes publishing possible. A package declaring `workspace:*` dependencies -resolves its siblings from the registry at build time, so its first artifact -cannot be built until those siblings are published — and they cannot be -published to a package that does not exist. An artifact with no dependencies at -all has no such cycle, which is what lets a package declaring siblings — -`@executablemd/acp` and `@executablemd/test-agent` among them — be bootstrapped -at all. +The reservation is empty because nothing about it needs to be otherwise. It +exists to make the name resolvable so `npm trust` can be configured against it, +and an empty artifact carries no dependency, no entry point and no claim about +the package's contents for `latest` to inherit by accident. + +It is not a workaround for a build that cannot run. A package declaring +`workspace:*` dependencies builds its siblings from the same checkout (§3), so +its first artifact can be built before any of them is published. That was not +true when this procedure was written, and the belief that it was is what made +the reservation look forced rather than chosen; #152 records the version of this +step that assumed it. `0.0.0-bootstrap.0` is never a release version, so `publish-one.yml`'s already-published guard never matches it: the first tagged release publishes its