From 5cfb2a4ea8c73e10b33b6abf08352df6f1580c0c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 13:46:39 +0100 Subject: [PATCH 01/10] cli-common: add unified run utils Signed-off-by: Patrik Oldsberg --- .changeset/modern-taxes-start.md | 5 + packages/cli-common/package.json | 3 + packages/cli-common/report.api.md | 39 ++++++ packages/cli-common/src/errors.ts | 34 +++++ packages/cli-common/src/index.ts | 9 ++ packages/cli-common/src/run.ts | 213 ++++++++++++++++++++++++++++++ yarn.lock | 3 + 7 files changed, 306 insertions(+) create mode 100644 .changeset/modern-taxes-start.md create mode 100644 packages/cli-common/src/errors.ts create mode 100644 packages/cli-common/src/run.ts diff --git a/.changeset/modern-taxes-start.md b/.changeset/modern-taxes-start.md new file mode 100644 index 0000000000..9fd7645570 --- /dev/null +++ b/.changeset/modern-taxes-start.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli-common': patch +--- + +Added new `run`, `runOutput`, and `runCheck` utilities to help run child processes in a safe and portable way. diff --git a/packages/cli-common/package.json b/packages/cli-common/package.json index b532da83c8..8f8904a968 100644 --- a/packages/cli-common/package.json +++ b/packages/cli-common/package.json @@ -35,11 +35,14 @@ "test": "backstage-cli package test" }, "dependencies": { + "@backstage/errors": "workspace:^", + "cross-spawn": "^7.0.3", "global-agent": "^3.0.0", "undici": "^7.2.3" }, "devDependencies": { "@backstage/cli": "workspace:^", + "@types/cross-spawn": "^6.0.2", "@types/node": "^20.16.0" } } diff --git a/packages/cli-common/report.api.md b/packages/cli-common/report.api.md index 0e3c80afa4..a2aba06228 100644 --- a/packages/cli-common/report.api.md +++ b/packages/cli-common/report.api.md @@ -3,12 +3,23 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +import { ChildProcess } from 'child_process'; +import { CustomErrorBase } from '@backstage/errors'; +import { SpawnOptions } from 'child_process'; + // @public export const BACKSTAGE_JSON = 'backstage.json'; // @public export function bootstrapEnvProxyAgents(): void; +// @public +export class ExitCodeError extends CustomErrorBase { + constructor(code: number, command?: string); + // (undocumented) + readonly code: number; +} + // @public export function findPaths(searchDir: string): Paths; @@ -29,4 +40,32 @@ export type Paths = { // @public export type ResolveFunc = (...paths: string[]) => string; + +// @public +export function run(args: string[], options?: RunOptions): RunChildProcess; + +// @public +export function runCheck(args: string[]): Promise; + +// @public +export interface RunChildProcess extends ChildProcess { + waitForExit(): Promise; +} + +// @public +export type RunLogFunc = (data: Buffer) => void; + +// @public +export type RunOptions = Omit & { + env?: Partial; + stdoutLogFunc?: RunLogFunc; + stderrLogFunc?: RunLogFunc; + stdio?: SpawnOptions['stdio']; +}; + +// @public +export function runOutput( + args: string[], + options?: RunOptions, +): Promise; ``` diff --git a/packages/cli-common/src/errors.ts b/packages/cli-common/src/errors.ts new file mode 100644 index 0000000000..07664d2ffb --- /dev/null +++ b/packages/cli-common/src/errors.ts @@ -0,0 +1,34 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { CustomErrorBase } from '@backstage/errors'; + +/** + * Error thrown when a child process exits with a non-zero code. + * @public + */ +export class ExitCodeError extends CustomErrorBase { + readonly code: number; + + constructor(code: number, command?: string) { + super( + command + ? `Command '${command}' exited with code ${code}` + : `Child exited with code ${code}`, + ); + this.code = code; + } +} diff --git a/packages/cli-common/src/index.ts b/packages/cli-common/src/index.ts index 11e5d05f4f..632aff9eb6 100644 --- a/packages/cli-common/src/index.ts +++ b/packages/cli-common/src/index.ts @@ -24,3 +24,12 @@ export { findPaths, BACKSTAGE_JSON } from './paths'; export { isChildPath } from './isChildPath'; export type { Paths, ResolveFunc } from './paths'; export { bootstrapEnvProxyAgents } from './proxyBootstrap'; +export { + run, + runOutput, + runCheck, + type RunChildProcess, + type RunOptions, + type RunLogFunc, +} from './run'; +export { ExitCodeError } from './errors'; diff --git a/packages/cli-common/src/run.ts b/packages/cli-common/src/run.ts new file mode 100644 index 0000000000..9a0a0975c1 --- /dev/null +++ b/packages/cli-common/src/run.ts @@ -0,0 +1,213 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ChildProcess, SpawnOptions } from 'child_process'; +import spawn from 'cross-spawn'; +import { ExitCodeError } from './errors'; +import { assertError } from '@backstage/errors'; + +/** + * Callback function that can be used to receive stdout or stderr data from a child process. + * + * @public + */ +export type RunLogFunc = (data: Buffer) => void; + +/** + * Options for running a child process with {@link run} or related functions. + * + * @public + */ +export type RunOptions = Omit & { + env?: Partial; + stdoutLogFunc?: RunLogFunc; + stderrLogFunc?: RunLogFunc; + stdio?: SpawnOptions['stdio']; +}; + +/** + * Child process handle returned by {@link run}. + * + * @public + */ +export interface RunChildProcess extends ChildProcess { + /** + * Waits for the child process to exit. + * + * @remarks + * + * Resolves when the process exits successfully (exit code 0) or is terminated by a signal. + * If the process exits with a non-zero exit code, the promise is rejected with an {@link ExitCodeError}. + * + * @returns A promise that resolves when the process exits successfully or is terminated by a signal, or rejects on error. + */ + waitForExit(): Promise; +} + +/** + * Runs a command and returns a child process handle. + * + * @public + */ +export function run(args: string[], options: RunOptions = {}): RunChildProcess { + if (args.length === 0) { + throw new Error('run requires at least one argument'); + } + + const [name, ...cmdArgs] = args; + + const { + stdoutLogFunc, + stderrLogFunc, + stdio: customStdio, + ...spawnOptions + } = options; + const env: NodeJS.ProcessEnv = { + ...process.env, + FORCE_COLOR: 'true', + ...(options.env ?? {}), + }; + + const stdio = + customStdio ?? + ([ + 'inherit', + stdoutLogFunc ? 'pipe' : 'inherit', + stderrLogFunc ? 'pipe' : 'inherit', + ] as ('inherit' | 'pipe')[]); + + const child = spawn(name, cmdArgs, { + ...spawnOptions, + stdio, + env, + }) as RunChildProcess; + + if (stdoutLogFunc && child.stdout) { + child.stdout.on('data', stdoutLogFunc); + } + if (stderrLogFunc && child.stderr) { + child.stderr.on('data', stderrLogFunc); + } + + const commandName = args.join(' '); + + let signalHandlersRegistered = false; + const handleSignal = () => { + if (!child.killed && child.exitCode === null) { + child.kill(); + } + }; + + child.waitForExit = async (): Promise => { + // Register signal handlers to kill child process on SIGINT/SIGTERM + if (!signalHandlersRegistered) { + for (const signal of ['SIGINT', 'SIGTERM'] as const) { + process.on(signal, handleSignal); + } + signalHandlersRegistered = true; + } + + try { + if (typeof child.exitCode === 'number') { + if (child.exitCode) { + throw new ExitCodeError(child.exitCode, commandName); + } + return; + } + + await new Promise((resolve, reject) => { + child.once('error', reject); + child.once('exit', code => { + if (code) { + reject(new ExitCodeError(code, commandName)); + } else { + resolve(); + } + }); + }); + } finally { + // Clean up signal handlers when done waiting + if (signalHandlersRegistered) { + for (const signal of ['SIGINT', 'SIGTERM'] as const) { + process.removeListener(signal, handleSignal); + } + signalHandlersRegistered = false; + } + } + }; + + return child; +} + +/** + * Runs a command and returns the stdout. + * + * @remarks + * + * On error, both stdout and stderr are attached to the error object as properties. + * + * @public + */ +export async function runOutput( + args: string[], + options?: RunOptions, +): Promise { + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + + if (args.length === 0) { + throw new Error('runOutput requires at least one argument'); + } + + try { + await run(args, { + ...options, + stdoutLogFunc: data => { + stdoutChunks.push(data); + options?.stdoutLogFunc?.(data); + }, + stderrLogFunc: data => { + stderrChunks.push(data); + options?.stderrLogFunc?.(data); + }, + }).waitForExit(); + + return Buffer.concat(stdoutChunks).toString().trim(); + } catch (error) { + assertError(error); + + (error as Error & { stdout?: string }).stdout = + Buffer.concat(stdoutChunks).toString(); + (error as Error & { stderr?: string }).stderr = + Buffer.concat(stderrChunks).toString(); + + throw error; + } +} + +/** + * Runs a command and returns true if it exits with code 0, false otherwise. + * + * @public + */ +export async function runCheck(args: string[]): Promise { + try { + await run(args).waitForExit(); + return true; + } catch { + return false; + } +} diff --git a/yarn.lock b/yarn.lock index 3502df3388..9011dc7b35 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3140,7 +3140,10 @@ __metadata: resolution: "@backstage/cli-common@workspace:packages/cli-common" dependencies: "@backstage/cli": "workspace:^" + "@backstage/errors": "workspace:^" + "@types/cross-spawn": "npm:^6.0.2" "@types/node": "npm:^20.16.0" + cross-spawn: "npm:^7.0.3" global-agent: "npm:^3.0.0" undici: "npm:^7.2.3" languageName: unknown From ecb44ccdf169bc4dd2e3894ace48e6d066e3a2ee Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 13:47:02 +0100 Subject: [PATCH 02/10] e2e-test: update to use new run utils from cli-common Signed-off-by: Patrik Oldsberg --- packages/e2e-test/src/commands/index.ts | 4 +- .../src/commands/{run.ts => runCommand.ts} | 44 +++++----- packages/e2e-test/src/index.ts | 11 ++- packages/e2e-test/src/lib/helpers.ts | 87 ------------------- 4 files changed, 31 insertions(+), 115 deletions(-) rename packages/e2e-test/src/commands/{run.ts => runCommand.ts} (94%) diff --git a/packages/e2e-test/src/commands/index.ts b/packages/e2e-test/src/commands/index.ts index fd0ba59c30..cbcb9f3907 100644 --- a/packages/e2e-test/src/commands/index.ts +++ b/packages/e2e-test/src/commands/index.ts @@ -15,12 +15,12 @@ */ import { Command } from 'commander'; -import { run } from './run'; +import { runCommand } from './runCommand'; export function registerCommands(program: Command) { program .command('run') .option('--keep', 'Do not remove the temporary dir after tests complete') .description('Run e2e tests') - .action(run); + .action(runCommand); } diff --git a/packages/e2e-test/src/commands/run.ts b/packages/e2e-test/src/commands/runCommand.ts similarity index 94% rename from packages/e2e-test/src/commands/run.ts rename to packages/e2e-test/src/commands/runCommand.ts index 6df58349df..112937dbe7 100644 --- a/packages/e2e-test/src/commands/run.ts +++ b/packages/e2e-test/src/commands/runCommand.ts @@ -22,18 +22,12 @@ import killTree from 'tree-kill'; import { resolve as resolvePath, join as joinPath } from 'path'; import path from 'path'; -import { - spawnPiped, - runPlain, - waitFor, - waitForExit, - print, -} from '../lib/helpers'; +import { waitFor, print } from '../lib/helpers'; import mysql from 'mysql2/promise'; import pgtools from 'pgtools'; -import { findPaths } from '@backstage/cli-common'; +import { findPaths, runOutput, run } from '@backstage/cli-common'; import { OptionValues } from 'commander'; // eslint-disable-next-line no-restricted-syntax @@ -46,7 +40,7 @@ const templatePackagePaths = [ 'packages/create-app/templates/default-app/packages/backend/package.json.hbs', ]; -export async function run(opts: OptionValues) { +export async function runCommand(opts: OptionValues) { const rootDir = await fs.mkdtemp(resolvePath(os.tmpdir(), 'backstage-e2e-')); print(`CLI E2E test root: ${rootDir}\n`); @@ -67,7 +61,7 @@ export async function run(opts: OptionValues) { await createPlugin({ appDir, pluginId, select: 'backend-plugin' }); print(`Running 'yarn test:e2e' in newly created app with new plugin`); - await runPlain(['yarn', 'test:e2e'], { + await runOutput(['yarn', 'test:e2e'], { cwd: appDir, env: { ...process.env, CI: undefined }, }); @@ -75,13 +69,13 @@ export async function run(opts: OptionValues) { await switchToReact17(appDir); print(`Running 'yarn install' to install React 17`); - await runPlain(['yarn', 'install'], { cwd: appDir }); + await runOutput(['yarn', 'install'], { cwd: appDir }); print(`Running 'yarn tsc' with React 17`); - await runPlain(['yarn', 'tsc'], { cwd: appDir }); + await runOutput(['yarn', 'tsc'], { cwd: appDir }); print(`Running 'yarn test:e2e' with React 17`); - await runPlain(['yarn', 'test:e2e'], { + await runOutput(['yarn', 'test:e2e'], { cwd: appDir, env: { ...process.env, CI: undefined }, }); @@ -190,7 +184,7 @@ async function buildDistWorkspace(workspaceName: string, rootDir: string) { appendDeps(require('@backstage/create-app/package.json')); print(`Preparing workspace`); - await runPlain([ + await runOutput([ 'yarn', 'backstage-cli', 'build-workspace', @@ -209,7 +203,7 @@ async function buildDistWorkspace(workspaceName: string, rootDir: string) { } print('Installing workspace dependencies'); - await runPlain(['yarn', 'workspaces', 'focus', '--all', '--production'], { + await runOutput(['yarn', 'workspaces', 'focus', '--all', '--production'], { cwd: workspaceDir, }); @@ -257,7 +251,7 @@ async function createApp( workspaceDir: string, rootDir: string, ) { - const child = spawnPiped( + const child = run( [ 'node', resolvePath(workspaceDir, 'packages/create-app/bin/backstage-create-app'), @@ -265,6 +259,7 @@ async function createApp( ], { cwd: rootDir, + stdio: ['pipe', 'pipe', 'pipe'], }, ); @@ -278,7 +273,7 @@ async function createApp( child.stdin?.write(`${appName}\n`); print('Waiting for app create script to be done'); - await waitForExit(child); + await child.waitForExit(); const appDir = resolvePath(rootDir, appName); @@ -318,7 +313,7 @@ async function createApp( 'test:all', ]) { print(`Running 'yarn ${cmd}' in newly created app`); - await runPlain(['yarn', cmd], { cwd: appDir }); + await runOutput(['yarn', cmd], { cwd: appDir }); } return appDir; @@ -378,10 +373,11 @@ async function createPlugin(options: { select: string; }) { const { appDir, pluginId, select } = options; - const child = spawnPiped( + const child = run( ['yarn', 'new', '--select', select, '--option', `pluginId=${pluginId}`], { cwd: appDir, + stdio: ['pipe', 'pipe', 'pipe'], }, ); @@ -392,7 +388,7 @@ async function createPlugin(options: { }); print('Waiting for plugin create script to be done'); - await waitForExit(child); + await child.waitForExit(); const pluginDir = resolvePath( appDir, @@ -401,11 +397,11 @@ async function createPlugin(options: { ); print(`Running 'yarn tsc' in root for newly created plugin`); - await runPlain(['yarn', 'tsc'], { cwd: appDir }); + await runOutput(['yarn', 'tsc'], { cwd: appDir }); for (const cmd of [['lint'], ['test', '--no-watch']]) { print(`Running 'yarn ${cmd.join(' ')}' in newly created plugin`); - await runPlain(['yarn', ...cmd], { cwd: pluginDir }); + await runOutput(['yarn', ...cmd], { cwd: pluginDir }); } } finally { child.kill(); @@ -494,7 +490,7 @@ async function dropClientDatabases(client: string) { * Start serving the newly created backend and make sure that all db migrations works correctly */ async function testBackendStart(appDir: string, ...args: string[]) { - const child = spawnPiped(['yarn', 'workspace', 'backend', 'start', ...args], { + const child = run(['yarn', 'workspace', 'backend', 'start', ...args], { cwd: appDir, // Windows does not like piping stdin here, the child process will hang when requiring the 'process' module stdio: ['ignore', 'pipe', 'pipe'], @@ -586,7 +582,7 @@ async function testBackendStart(appDir: string, ...args: string[]) { } try { - await waitForExit(child); + await child.waitForExit(); } catch (error) { if (!successful) { throw new Error(`Backend failed to startup: ${stderr}`); diff --git a/packages/e2e-test/src/index.ts b/packages/e2e-test/src/index.ts index f3c63ae175..df299628b2 100644 --- a/packages/e2e-test/src/index.ts +++ b/packages/e2e-test/src/index.ts @@ -18,7 +18,6 @@ import { program } from 'commander'; import chalk from 'chalk'; import { registerCommands } from './commands'; import { version } from '../package.json'; -import { exitWithError } from './lib/helpers'; async function main(argv: string[]) { program.name('e2e-test').version(version); @@ -36,4 +35,12 @@ async function main(argv: string[]) { program.parse(argv); } -main(process.argv).catch(exitWithError); +main(process.argv).catch(err => { + process.stdout.write(`${err.name}: ${err.stack || err.message}\n`); + + if (typeof err.code === 'number') { + process.exit(err.code); + } else { + process.exit(1); + } +}); diff --git a/packages/e2e-test/src/lib/helpers.ts b/packages/e2e-test/src/lib/helpers.ts index fc5aefcff9..d310812794 100644 --- a/packages/e2e-test/src/lib/helpers.ts +++ b/packages/e2e-test/src/lib/helpers.ts @@ -14,77 +14,6 @@ * limitations under the License. */ -import { assertError } from '@backstage/errors'; -import { - spawn, - execFile as execFileCb, - SpawnOptions, - ChildProcess, -} from 'child_process'; -import { promisify } from 'util'; - -const execFile = promisify(execFileCb); - -export function spawnPiped(cmd: string[], options?: SpawnOptions) { - function pipeWithPrefix(stream: NodeJS.WriteStream, prefix = '') { - return (data: Buffer) => { - const prefixedMsg = data - .toString('utf8') - .trimEnd() - .replace(/^/gm, prefix); - stream.write(`${prefixedMsg}\n`, 'utf8'); - }; - } - - const child = spawn(cmd[0], cmd.slice(1), { - stdio: 'pipe', - shell: true, - ...options, - }); - child.on('error', exitWithError); - - const logPrefix = cmd.map(s => s.replace(/.+\//, '')).join(' '); - child.stdout?.on( - 'data', - pipeWithPrefix(process.stdout, `[${logPrefix}].out: `), - ); - child.stderr?.on( - 'data', - pipeWithPrefix(process.stderr, `[${logPrefix}].err: `), - ); - - return child; -} - -export async function runPlain(cmd: string[], options?: SpawnOptions) { - try { - const { stdout } = await execFile(cmd[0], cmd.slice(1), { - ...options, - shell: true, - }); - return stdout.trim(); - } catch (error) { - assertError(error); - if (error.stdout) { - process.stdout.write(error.stdout as Buffer); - } - if (error.stderr) { - process.stderr.write(error.stderr as Buffer); - } - throw error; - } -} - -export function exitWithError(err: Error & { code?: unknown }) { - process.stdout.write(`${err.name}: ${err.stack || err.message}\n`); - - if (typeof err.code === 'number') { - process.exit(err.code); - } else { - process.exit(1); - } -} - /** * Waits for fn() to be true * Checks every 100ms @@ -108,22 +37,6 @@ export function waitFor(fn: () => boolean, maxSeconds: number = 120) { }); } -export async function waitForExit(child: ChildProcess) { - if (child.exitCode !== null) { - throw new Error(`Child already exited with code ${child.exitCode}`); - } - await new Promise((resolve, reject) => - child.once('exit', code => { - if (code) { - reject(new Error(`Child exited with code ${code}`)); - } else { - print('Child finished'); - resolve(); - } - }), - ); -} - export function print(msg: string) { return process.stdout.write(`${msg}\n`); } From 43629b128d6f0e8885161fed152c777667419d8b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 14:22:27 +0100 Subject: [PATCH 03/10] techdocs-cli: update to use new run utils from cli-common Signed-off-by: Patrik Oldsberg --- .changeset/chilly-bikes-rule.md | 5 + .../techdocs-cli/src/commands/serve/mkdocs.ts | 8 +- .../techdocs-cli/src/commands/serve/serve.ts | 9 +- .../techdocs-cli/src/commands/serve/utils.ts | 17 ++-- .../techdocs-cli/src/lib/mkdocsServer.test.ts | 55 ++++++----- packages/techdocs-cli/src/lib/mkdocsServer.ts | 19 ++-- packages/techdocs-cli/src/lib/run.ts | 99 ------------------- 7 files changed, 57 insertions(+), 155 deletions(-) create mode 100644 .changeset/chilly-bikes-rule.md delete mode 100644 packages/techdocs-cli/src/lib/run.ts diff --git a/.changeset/chilly-bikes-rule.md b/.changeset/chilly-bikes-rule.md new file mode 100644 index 0000000000..894999cf56 --- /dev/null +++ b/.changeset/chilly-bikes-rule.md @@ -0,0 +1,5 @@ +--- +'@techdocs/cli': patch +--- + +Updated to use new utilities from `@backstage/cli-common`. diff --git a/packages/techdocs-cli/src/commands/serve/mkdocs.ts b/packages/techdocs-cli/src/commands/serve/mkdocs.ts index 00c5e5f0ed..3938764fed 100644 --- a/packages/techdocs-cli/src/commands/serve/mkdocs.ts +++ b/packages/techdocs-cli/src/commands/serve/mkdocs.ts @@ -18,7 +18,7 @@ import { OptionValues } from 'commander'; import openBrowser from 'react-dev-utils/openBrowser'; import { createLogger } from '../../lib/utility'; import { runMkdocsServer } from '../../lib/mkdocsServer'; -import { LogFunc, waitForSignal } from '../../lib/run'; +import { RunLogFunc } from '@backstage/cli-common'; import { getMkdocsYml } from '@backstage/plugin-techdocs-node'; import fs from 'fs-extra'; import { checkIfDockerIsOperational } from './utils'; @@ -45,7 +45,7 @@ export default async function serveMkdocs(opts: OptionValues) { // We want to open browser only once based on a log. let boolOpenBrowserTriggered = false; - const logFunc: LogFunc = data => { + const logFunc: RunLogFunc = data => { // Sometimes the lines contain an unnecessary extra new line in between const logLines = data.toString().split('\n'); const logPrefix = opts.docker ? '[docker/mkdocs]' : '[mkdocs]'; @@ -74,7 +74,7 @@ export default async function serveMkdocs(opts: OptionValues) { // Had me questioning this whole implementation for half an hour. // Commander stores --no-docker in cmd.docker variable - const childProcess = await runMkdocsServer({ + const childProcess = runMkdocsServer({ port: opts.port, dockerImage: opts.dockerImage, dockerEntrypoint: opts.dockerEntrypoint, @@ -85,7 +85,7 @@ export default async function serveMkdocs(opts: OptionValues) { }); // Keep waiting for user to cancel the process - await waitForSignal([childProcess]); + await childProcess.waitForExit(); if (configIsTemporary) { process.on('exit', async () => { diff --git a/packages/techdocs-cli/src/commands/serve/serve.ts b/packages/techdocs-cli/src/commands/serve/serve.ts index 27f432c22d..c325a99871 100644 --- a/packages/techdocs-cli/src/commands/serve/serve.ts +++ b/packages/techdocs-cli/src/commands/serve/serve.ts @@ -17,10 +17,9 @@ import { OptionValues } from 'commander'; import path from 'path'; import openBrowser from 'react-dev-utils/openBrowser'; -import { findPaths } from '@backstage/cli-common'; +import { findPaths, RunLogFunc } from '@backstage/cli-common'; import HTTPServer from '../../lib/httpServer'; import { runMkdocsServer } from '../../lib/mkdocsServer'; -import { LogFunc, waitForSignal } from '../../lib/run'; import { createLogger } from '../../lib/utility'; import { getMkdocsYml } from '@backstage/plugin-techdocs-node'; import fs from 'fs-extra'; @@ -83,7 +82,7 @@ export default async function serve(opts: OptionValues) { } let mkdocsServerHasStarted = false; - const mkdocsLogFunc: LogFunc = data => { + const mkdocsLogFunc: RunLogFunc = data => { // Sometimes the lines contain an unnecessary extra new line const logLines = data.toString().split('\n'); const logPrefix = opts.docker ? '[docker/mkdocs]' : '[mkdocs]'; @@ -107,7 +106,7 @@ export default async function serve(opts: OptionValues) { // https://github.com/mkdocs/mkdocs/issues/879#issuecomment-203536006 // Had me questioning this whole implementation for half an hour. logger.info('Starting mkdocs server.'); - const mkdocsChildProcess = await runMkdocsServer({ + const mkdocsChildProcess = runMkdocsServer({ port: opts.mkdocsPort, dockerImage: opts.dockerImage, dockerEntrypoint: opts.dockerEntrypoint, @@ -161,7 +160,7 @@ export default async function serve(opts: OptionValues) { ); }); - await waitForSignal([mkdocsChildProcess]); + await mkdocsChildProcess.waitForExit(); if (configIsTemporary) { process.on('exit', async () => { diff --git a/packages/techdocs-cli/src/commands/serve/utils.ts b/packages/techdocs-cli/src/commands/serve/utils.ts index 2b19b0fbc0..9ecb8bc29b 100644 --- a/packages/techdocs-cli/src/commands/serve/utils.ts +++ b/packages/techdocs-cli/src/commands/serve/utils.ts @@ -14,25 +14,22 @@ * limitations under the License. */ -import { promisify } from 'util'; import * as winston from 'winston'; -import { execFile } from 'child_process'; +import { runCheck } from '@backstage/cli-common'; export async function checkIfDockerIsOperational( logger: winston.Logger, ): Promise { logger.info('Checking Docker status...'); - try { - const runCheck = promisify(execFile); - await runCheck('docker', ['info'], { shell: true }); + const isOperational = await runCheck(['docker', 'info']); + if (isOperational) { logger.info( 'Docker is up and running. Proceed to starting up mkdocs server', ); return true; - } catch { - logger.error( - 'Docker is not running. Exiting. Please check status of Docker daemon with `docker info` before re-running', - ); - return false; } + logger.error( + 'Docker is not running. Exiting. Please check status of Docker daemon with `docker info` before re-running', + ); + return false; } diff --git a/packages/techdocs-cli/src/lib/mkdocsServer.test.ts b/packages/techdocs-cli/src/lib/mkdocsServer.test.ts index 83f9c9f382..5acd1784a3 100644 --- a/packages/techdocs-cli/src/lib/mkdocsServer.test.ts +++ b/packages/techdocs-cli/src/lib/mkdocsServer.test.ts @@ -15,9 +15,9 @@ */ import { runMkdocsServer } from './mkdocsServer'; -import { run } from './run'; +import { run } from '@backstage/cli-common'; -jest.mock('./run', () => { +jest.mock('@backstage/cli-common', () => { return { run: jest.fn(), }; @@ -29,12 +29,12 @@ describe('runMkdocsServer', () => { }); describe('docker', () => { - it('should run docker directly by default', async () => { - await runMkdocsServer({}); + it('should run docker directly by default', () => { + runMkdocsServer({}); expect(run).toHaveBeenCalledWith( - 'docker', expect.arrayContaining([ + 'docker', 'run', `${process.cwd()}:/content`, '8000:8000', @@ -47,26 +47,24 @@ describe('runMkdocsServer', () => { ); }); - it('should accept port option', async () => { - await runMkdocsServer({ port: '5678' }); + it('should accept port option', () => { + runMkdocsServer({ port: '5678' }); expect(run).toHaveBeenCalledWith( - 'docker', - expect.arrayContaining(['5678:5678', '0.0.0.0:5678']), + expect.arrayContaining(['docker', '5678:5678', '0.0.0.0:5678']), expect.objectContaining({}), ); }); - it('should accept custom docker image', async () => { - await runMkdocsServer({ dockerImage: 'my-org/techdocs' }); + it('should accept custom docker image', () => { + runMkdocsServer({ dockerImage: 'my-org/techdocs' }); expect(run).toHaveBeenCalledWith( - 'docker', - expect.arrayContaining(['my-org/techdocs']), + expect.arrayContaining(['docker', 'my-org/techdocs']), expect.objectContaining({}), ); }); - it('should accept custom docker options', async () => { - await runMkdocsServer({ + it('should accept custom docker options', () => { + runMkdocsServer({ dockerOptions: [ '--add-host=internal.host:192.168.11.12', '--name', @@ -75,8 +73,8 @@ describe('runMkdocsServer', () => { }); expect(run).toHaveBeenCalledWith( - 'docker', expect.arrayContaining([ + 'docker', 'run', '--rm', '-w', @@ -98,14 +96,14 @@ describe('runMkdocsServer', () => { ); }); - it('should accept additinoal mkdocs CLI parameters', async () => { - await runMkdocsServer({ + it('should accept additinoal mkdocs CLI parameters', () => { + runMkdocsServer({ mkdocsParameterClean: true, mkdocsParameterStrict: true, }); expect(run).toHaveBeenCalledWith( - 'docker', expect.arrayContaining([ + 'docker', 'serve', '--dev-addr', '0.0.0.0:8000', @@ -118,21 +116,24 @@ describe('runMkdocsServer', () => { }); describe('mkdocs', () => { - it('should run mkdocs if specified', async () => { - await runMkdocsServer({ useDocker: false }); + it('should run mkdocs if specified', () => { + runMkdocsServer({ useDocker: false }); expect(run).toHaveBeenCalledWith( - 'mkdocs', - expect.arrayContaining(['serve', '--dev-addr', '127.0.0.1:8000']), + expect.arrayContaining([ + 'mkdocs', + 'serve', + '--dev-addr', + '127.0.0.1:8000', + ]), expect.objectContaining({}), ); }); - it('should accept port option', async () => { - await runMkdocsServer({ useDocker: false, port: '5678' }); + it('should accept port option', () => { + runMkdocsServer({ useDocker: false, port: '5678' }); expect(run).toHaveBeenCalledWith( - 'mkdocs', - expect.arrayContaining(['127.0.0.1:5678']), + expect.arrayContaining(['mkdocs', '127.0.0.1:5678']), expect.objectContaining({}), ); }); diff --git a/packages/techdocs-cli/src/lib/mkdocsServer.ts b/packages/techdocs-cli/src/lib/mkdocsServer.ts index a793643367..929ee77fb1 100644 --- a/packages/techdocs-cli/src/lib/mkdocsServer.ts +++ b/packages/techdocs-cli/src/lib/mkdocsServer.ts @@ -14,30 +14,29 @@ * limitations under the License. */ -import { ChildProcess } from 'child_process'; -import { run, LogFunc } from './run'; +import { run, RunChildProcess, RunLogFunc } from '@backstage/cli-common'; -export const runMkdocsServer = async (options: { +export const runMkdocsServer = (options: { port?: string; useDocker?: boolean; dockerImage?: string; dockerEntrypoint?: string; dockerOptions?: string[]; - stdoutLogFunc?: LogFunc; - stderrLogFunc?: LogFunc; + stdoutLogFunc?: RunLogFunc; + stderrLogFunc?: RunLogFunc; mkdocsConfigFileName?: string; mkdocsParameterClean?: boolean; mkdocsParameterDirtyReload?: boolean; mkdocsParameterStrict?: boolean; -}): Promise => { +}): RunChildProcess => { const port = options.port ?? '8000'; const useDocker = options.useDocker ?? true; const dockerImage = options.dockerImage ?? 'spotify/techdocs'; if (useDocker) { - return await run( - 'docker', + return run( [ + 'docker', 'run', '--rm', '-w', @@ -69,9 +68,9 @@ export const runMkdocsServer = async (options: { ); } - return await run( - 'mkdocs', + return run( [ + 'mkdocs', 'serve', '--dev-addr', `127.0.0.1:${port}`, diff --git a/packages/techdocs-cli/src/lib/run.ts b/packages/techdocs-cli/src/lib/run.ts deleted file mode 100644 index 13ac7d9d4c..0000000000 --- a/packages/techdocs-cli/src/lib/run.ts +++ /dev/null @@ -1,99 +0,0 @@ -/* - * Copyright 2020 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -import { spawn, SpawnOptions, ChildProcess } from 'child_process'; - -export type LogFunc = (data: Buffer | string) => void; -type SpawnOptionsPartialEnv = Omit & { - env?: Partial; - // Pipe stdout to this log function - stdoutLogFunc?: LogFunc; - // Pipe stderr to this log function - stderrLogFunc?: LogFunc; -}; - -// TODO: Accept log functions to pipe logs with. -// Runs a child command, returning the child process instance. -// Use it along with waitForSignal to run a long running process e.g. mkdocs serve -export const run = async ( - name: string, - args: string[] = [], - options: SpawnOptionsPartialEnv = {}, -): Promise => { - const { stdoutLogFunc, stderrLogFunc } = options; - - const env: NodeJS.ProcessEnv = { - ...process.env, - FORCE_COLOR: 'true', - ...(options.env ?? {}), - }; - - // Refer: https://nodejs.org/api/child_process.html#child_process_subprocess_stdio - const stdio = [ - 'inherit', - stdoutLogFunc ? 'pipe' : 'inherit', - stderrLogFunc ? 'pipe' : 'inherit', - ] as ('inherit' | 'pipe')[]; - - const childProcess = spawn(name, args, { - stdio: stdio, - ...options, - env, - }); - - if (stdoutLogFunc && childProcess.stdout) { - childProcess.stdout.on('data', stdoutLogFunc); - } - if (stderrLogFunc && childProcess.stderr) { - childProcess.stderr.on('data', stderrLogFunc); - } - - return childProcess; -}; - -// Block indefinitely and wait for a signal to stop the child process(es) -// Throw error if any child process errors -// Resolves only when all processes exit with status code 0 -export async function waitForSignal( - childProcesses: Array, -): Promise { - const promises: Array> = []; - - for (const signal of ['SIGINT', 'SIGTERM'] as const) { - process.on(signal, () => { - childProcesses.forEach(childProcess => { - childProcess.kill(); - }); - }); - } - - childProcesses.forEach(childProcess => { - if (typeof childProcess.exitCode === 'number') { - if (childProcess.exitCode) { - throw new Error(`Non zero exit code from child process`); - } - return; - } - - promises.push( - new Promise((resolve, reject) => { - childProcess.once('error', reject); - childProcess.once('exit', resolve); - }), - ); - }); - - await Promise.all(promises); -} From 4e8c7261e92879429c733442e86cd9009c9d105d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 14:48:22 +0100 Subject: [PATCH 04/10] cli-node: update to use new run utils from cli-common Signed-off-by: Patrik Oldsberg --- .changeset/afraid-items-drum.md | 5 + packages/cli-node/src/git/GitUtils.ts | 20 ++-- .../src/monorepo/PackageGraph.test.ts | 2 +- .../cli-node/src/monorepo/PackageGraph.ts | 2 +- packages/cli-node/src/monorepo/isMonoRepo.ts | 2 +- .../cli-node/src/monorepo/isMonorepo.test.ts | 2 +- .../src/pacman/PackageManager.test.ts | 4 +- .../cli-node/src/pacman/PackageManager.ts | 5 +- .../cli-node/src/pacman/yarn/Yarn.test.ts | 4 +- packages/cli-node/src/pacman/yarn/Yarn.ts | 13 +-- packages/cli-node/src/paths.ts | 20 ++++ packages/cli-node/src/util.ts | 109 ------------------ 12 files changed, 52 insertions(+), 136 deletions(-) create mode 100644 .changeset/afraid-items-drum.md create mode 100644 packages/cli-node/src/paths.ts delete mode 100644 packages/cli-node/src/util.ts diff --git a/.changeset/afraid-items-drum.md b/.changeset/afraid-items-drum.md new file mode 100644 index 0000000000..94ddd608cd --- /dev/null +++ b/.changeset/afraid-items-drum.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli-node': patch +--- + +Updated to use new utilities from `@backstage/cli-common`. diff --git a/packages/cli-node/src/git/GitUtils.ts b/packages/cli-node/src/git/GitUtils.ts index 7b87487c56..7edb3581d6 100644 --- a/packages/cli-node/src/git/GitUtils.ts +++ b/packages/cli-node/src/git/GitUtils.ts @@ -15,23 +15,27 @@ */ import { assertError, ForwardedError } from '@backstage/errors'; -import { execFile, paths } from '../util'; +import { paths } from '../paths'; +import { runOutput } from '@backstage/cli-common'; /** * Run a git command, trimming the output splitting it into lines. */ export async function runGit(...args: string[]) { try { - const { stdout } = await execFile('git', args, { - shell: true, + const stdout = await runOutput(['git', ...args], { cwd: paths.targetRoot, }); return stdout.trim().split(/\r\n|\r|\n/); } catch (error) { assertError(error); - if (error.stderr || typeof error.code === 'number') { - const stderr = (error.stderr as undefined | Buffer)?.toString('utf8'); - const msg = stderr?.trim() ?? `with exit code ${error.code}`; + if ( + 'code' in error && + typeof (error as { code?: number }).code === 'number' + ) { + const code = (error as { code?: number }).code; + const stderr = (error as { stderr?: string }).stderr; + const msg = stderr?.trim() ?? `with exit code ${code}`; throw new Error(`git ${args[0]} failed, ${msg}`); } throw new ForwardedError('Unknown execution error', error); @@ -83,10 +87,8 @@ export class GitUtils { // silently fall back to using the ref directly if merge base is not available } - const { stdout } = await execFile('git', ['show', `${showRef}:${path}`], { - shell: true, + const stdout = await runOutput(['git', 'show', `${showRef}:${path}`], { cwd: paths.targetRoot, - maxBuffer: 1024 * 1024 * 50, }); return stdout; } diff --git a/packages/cli-node/src/monorepo/PackageGraph.test.ts b/packages/cli-node/src/monorepo/PackageGraph.test.ts index e2378b91c0..9bc9cbf72b 100644 --- a/packages/cli-node/src/monorepo/PackageGraph.test.ts +++ b/packages/cli-node/src/monorepo/PackageGraph.test.ts @@ -23,7 +23,7 @@ import { GitUtils } from '../git'; const mockListChangedFiles = jest.spyOn(GitUtils, 'listChangedFiles'); const mockReadFileAtRef = jest.spyOn(GitUtils, 'readFileAtRef'); -jest.mock('../util', () => ({ +jest.mock('../paths', () => ({ paths: { targetRoot: '/', resolveTargetRoot: (...paths: string[]) => resolvePath('/', ...paths), diff --git a/packages/cli-node/src/monorepo/PackageGraph.ts b/packages/cli-node/src/monorepo/PackageGraph.ts index 2f82af0808..76fbe9ba4f 100644 --- a/packages/cli-node/src/monorepo/PackageGraph.ts +++ b/packages/cli-node/src/monorepo/PackageGraph.ts @@ -16,7 +16,7 @@ import path from 'path'; import { getPackages, Package } from '@manypkg/get-packages'; -import { paths } from '../util'; +import { paths } from '../paths'; import { PackageRole } from '../roles'; import { GitUtils } from '../git'; import { Lockfile } from './Lockfile'; diff --git a/packages/cli-node/src/monorepo/isMonoRepo.ts b/packages/cli-node/src/monorepo/isMonoRepo.ts index f47ac4b9ad..da78c8dd53 100644 --- a/packages/cli-node/src/monorepo/isMonoRepo.ts +++ b/packages/cli-node/src/monorepo/isMonoRepo.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { paths } from '../util'; +import { paths } from '../paths'; import fs from 'fs-extra'; /** diff --git a/packages/cli-node/src/monorepo/isMonorepo.test.ts b/packages/cli-node/src/monorepo/isMonorepo.test.ts index 30c2d3ccbc..de0dae0893 100644 --- a/packages/cli-node/src/monorepo/isMonorepo.test.ts +++ b/packages/cli-node/src/monorepo/isMonorepo.test.ts @@ -19,7 +19,7 @@ import { createMockDirectory } from '@backstage/backend-test-utils'; const mockDir = createMockDirectory(); -jest.mock('../util', () => ({ +jest.mock('../paths', () => ({ paths: { resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args) }, })); diff --git a/packages/cli-node/src/pacman/PackageManager.test.ts b/packages/cli-node/src/pacman/PackageManager.test.ts index 3997da554c..dd35e74902 100644 --- a/packages/cli-node/src/pacman/PackageManager.test.ts +++ b/packages/cli-node/src/pacman/PackageManager.test.ts @@ -21,8 +21,8 @@ import { withLogCollector } from '@backstage/test-utils'; const mockDir = createMockDirectory(); -jest.mock('../util', () => ({ - ...jest.requireActual('../util'), +jest.mock('../paths', () => ({ + ...jest.requireActual('../paths'), paths: { resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args) }, })); diff --git a/packages/cli-node/src/pacman/PackageManager.ts b/packages/cli-node/src/pacman/PackageManager.ts index fc0aaa2c97..44c0849705 100644 --- a/packages/cli-node/src/pacman/PackageManager.ts +++ b/packages/cli-node/src/pacman/PackageManager.ts @@ -16,7 +16,8 @@ import { Yarn } from './yarn'; import { Lockfile } from './Lockfile'; -import { SpawnOptionsPartialEnv, paths } from '../util'; +import { paths } from '../paths'; +import { RunOptions } from '@backstage/cli-common'; import fs from 'fs-extra'; /** @@ -55,7 +56,7 @@ export interface PackageManager { getMonorepoPackages(): Promise; /** Uses the package manager to run a command in the repo. */ - run(args: string[], options?: SpawnOptionsPartialEnv): Promise; + run(args: string[], options?: RunOptions): Promise; /** * Executes the package manager's pack command to bundle the repo into an diff --git a/packages/cli-node/src/pacman/yarn/Yarn.test.ts b/packages/cli-node/src/pacman/yarn/Yarn.test.ts index 9584ef54b2..9bf866b821 100644 --- a/packages/cli-node/src/pacman/yarn/Yarn.test.ts +++ b/packages/cli-node/src/pacman/yarn/Yarn.test.ts @@ -19,8 +19,8 @@ import { Yarn } from './Yarn'; const mockDir = createMockDirectory(); -jest.mock('../../util', () => ({ - ...jest.requireActual('../../util'), +jest.mock('../../paths', () => ({ + ...jest.requireActual('../../paths'), paths: { resolveTargetRoot: (...args: string[]) => mockDir.resolve(...args) }, })); diff --git a/packages/cli-node/src/pacman/yarn/Yarn.ts b/packages/cli-node/src/pacman/yarn/Yarn.ts index 0a2b8d55ea..a6544344ca 100644 --- a/packages/cli-node/src/pacman/yarn/Yarn.ts +++ b/packages/cli-node/src/pacman/yarn/Yarn.ts @@ -23,7 +23,8 @@ import { PackageInfo, PackageManager } from '../PackageManager'; import { Lockfile } from '../Lockfile'; import { YarnVersion } from './types'; import fs from 'fs-extra'; -import { paths, run, execFile, SpawnOptionsPartialEnv } from '../../util'; +import { paths } from '../../paths'; +import { run, runOutput, RunOptions } from '@backstage/cli-common'; export class Yarn implements PackageManager { constructor(private readonly yarnVersion: YarnVersion) {} @@ -63,8 +64,8 @@ export class Yarn implements PackageManager { }); } - async run(args: string[], options?: SpawnOptionsPartialEnv) { - await run('yarn', args, options); + async run(args: string[], options?: RunOptions) { + await run(['yarn', ...args], options).waitForExit(); } async fetchPackageInfo(): Promise { @@ -98,8 +99,7 @@ function detectYarnVersion(dir?: string): Promise { const promise = Promise.resolve().then(async () => { try { - const { stdout } = await execFile('yarn', ['--version'], { - shell: true, + const stdout = await runOutput(['yarn', '--version'], { cwd, }); const versionString = stdout.trim(); @@ -109,9 +109,6 @@ function detectYarnVersion(dir?: string): Promise { return { version: versionString, codename }; } catch (error) { assertError(error); - if ('stderr' in error) { - process.stderr.write(error.stderr as Buffer); - } throw new ForwardedError('Failed to determine yarn version', error); } }); diff --git a/packages/cli-node/src/paths.ts b/packages/cli-node/src/paths.ts new file mode 100644 index 0000000000..2c658c27b3 --- /dev/null +++ b/packages/cli-node/src/paths.ts @@ -0,0 +1,20 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { findPaths } from '@backstage/cli-common'; + +/* eslint-disable-next-line no-restricted-syntax */ +export const paths = findPaths(__dirname); diff --git a/packages/cli-node/src/util.ts b/packages/cli-node/src/util.ts deleted file mode 100644 index eaf05855b1..0000000000 --- a/packages/cli-node/src/util.ts +++ /dev/null @@ -1,109 +0,0 @@ -/* - * Copyright 2020 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { - ChildProcess, - execFile as execFileCb, - spawn, - SpawnOptions, -} from 'child_process'; -import { promisify } from 'util'; -import { findPaths } from '@backstage/cli-common'; -import { ExitCodeError } from './errors'; - -export const execFile = promisify(execFileCb); - -/* eslint-disable-next-line no-restricted-syntax */ -export const paths = findPaths(__dirname); - -/** - * A function that can be used to log data from a child process - * - * @public - */ -export type LogFunc = (data: Buffer) => void; - -/** - * Options for running a child process - * - * @public - */ -export type SpawnOptionsPartialEnv = Omit & { - env?: Partial; - // Pipe stdout to this log function - stdoutLogFunc?: LogFunc; - // Pipe stderr to this log function - stderrLogFunc?: LogFunc; -}; - -// Runs a child command, returning a promise that is only resolved if the child exits with code 0. -export async function run( - name: string, - args: string[] = [], - options: SpawnOptionsPartialEnv = {}, -) { - const { stdoutLogFunc, stderrLogFunc } = options; - const env: NodeJS.ProcessEnv = { - ...process.env, - FORCE_COLOR: 'true', - ...(options.env ?? {}), - }; - - const stdio = [ - 'inherit', - stdoutLogFunc ? 'pipe' : 'inherit', - stderrLogFunc ? 'pipe' : 'inherit', - ] as ('inherit' | 'pipe')[]; - - const child = spawn(name, args, { - stdio, - shell: true, - ...options, - env, - }); - - if (stdoutLogFunc && child.stdout) { - child.stdout.on('data', stdoutLogFunc); - } - if (stderrLogFunc && child.stderr) { - child.stderr.on('data', stderrLogFunc); - } - - await waitForExit(child, name); -} - -async function waitForExit( - child: ChildProcess & { exitCode: number | null }, - name?: string, -): Promise { - if (typeof child.exitCode === 'number') { - if (child.exitCode) { - throw new ExitCodeError(child.exitCode, name); - } - return; - } - - await new Promise((resolve, reject) => { - child.once('error', error => reject(error)); - child.once('exit', code => { - if (code) { - reject(new ExitCodeError(code, name)); - } else { - resolve(); - } - }); - }); -} From 7fbac5cfac505ad7ca8ffd8bceb262d02d871dcb Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 15:53:03 +0100 Subject: [PATCH 05/10] cli: update to use new run utils from cli-common Signed-off-by: Patrik Oldsberg --- .changeset/tender-dancers-hunt.md | 5 + packages/cli/src/lib/run.ts | 121 ------------------ .../cli/src/lib/versioning/packages.test.ts | 50 ++++---- packages/cli/src/lib/versioning/packages.ts | 15 ++- packages/cli/src/lib/versioning/yarn.ts | 11 +- .../src/modules/build/lib/bundler/config.ts | 6 +- .../build/lib/packager/createDistWorkspace.ts | 10 +- .../cli/src/modules/info/commands/info.ts | 4 +- .../maintenance/commands/repo/clean.ts | 12 +- .../migrate/commands/packageLintConfigs.ts | 4 +- .../migrate/commands/versions/bump.test.ts | 116 ++++++++++------- .../modules/migrate/commands/versions/bump.ts | 4 +- .../migrate/commands/versions/migrate.test.ts | 69 +++++----- packages/cli/src/modules/migrate/lib/utils.ts | 6 +- packages/cli/src/modules/new/lib/tasks.ts | 14 +- .../src/modules/test/commands/package/test.ts | 6 +- .../src/modules/test/commands/repo/test.ts | 12 +- 17 files changed, 178 insertions(+), 287 deletions(-) create mode 100644 .changeset/tender-dancers-hunt.md delete mode 100644 packages/cli/src/lib/run.ts diff --git a/.changeset/tender-dancers-hunt.md b/.changeset/tender-dancers-hunt.md new file mode 100644 index 0000000000..e20ec43ae0 --- /dev/null +++ b/.changeset/tender-dancers-hunt.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli': patch +--- + +Updated to use new utilities from `@backstage/cli-common`. diff --git a/packages/cli/src/lib/run.ts b/packages/cli/src/lib/run.ts deleted file mode 100644 index e8602fcb18..0000000000 --- a/packages/cli/src/lib/run.ts +++ /dev/null @@ -1,121 +0,0 @@ -/* - * Copyright 2020 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { - SpawnOptions, - spawn, - ChildProcess, - execFile as execFileCb, -} from 'child_process'; -import { ExitCodeError } from './errors'; -import { promisify } from 'util'; -import { assertError, ForwardedError } from '@backstage/errors'; - -export const execFile = promisify(execFileCb); - -type LogFunc = (data: Buffer) => void; - -type SpawnOptionsPartialEnv = Omit & { - env?: Partial; - // Pipe stdout to this log function - stdoutLogFunc?: LogFunc; - // Pipe stderr to this log function - stderrLogFunc?: LogFunc; -}; - -// Runs a child command, returning a promise that is only resolved if the child exits with code 0. -export async function run( - name: string, - args: string[] = [], - options: SpawnOptionsPartialEnv = {}, -) { - const { stdoutLogFunc, stderrLogFunc } = options; - const env: NodeJS.ProcessEnv = { - ...process.env, - FORCE_COLOR: 'true', - ...(options.env ?? {}), - }; - - const stdio = [ - 'inherit', - stdoutLogFunc ? 'pipe' : 'inherit', - stderrLogFunc ? 'pipe' : 'inherit', - ] as ('inherit' | 'pipe')[]; - - const child = spawn(name, args, { - stdio, - shell: true, - ...options, - env, - }); - - if (stdoutLogFunc && child.stdout) { - child.stdout.on('data', stdoutLogFunc); - } - if (stderrLogFunc && child.stderr) { - child.stderr.on('data', stderrLogFunc); - } - - await waitForExit(child, name); -} - -export async function runPlain(cmd: string, ...args: string[]) { - try { - const { stdout } = await execFile(cmd, args, { shell: true }); - return stdout.trim(); - } catch (error) { - assertError(error); - if ('stderr' in error) { - process.stderr.write(error.stderr as Buffer); - } - if (typeof error.code === 'number') { - throw new ExitCodeError(error.code, [cmd, ...args].join(' ')); - } - throw new ForwardedError('Unknown execution error', error); - } -} - -export async function runCheck(cmd: string, ...args: string[]) { - try { - await execFile(cmd, args, { shell: true }); - return true; - } catch (error) { - return false; - } -} - -export async function waitForExit( - child: ChildProcess & { exitCode: number | null }, - name?: string, -): Promise { - if (typeof child.exitCode === 'number') { - if (child.exitCode) { - throw new ExitCodeError(child.exitCode, name); - } - return; - } - - await new Promise((resolve, reject) => { - child.once('error', error => reject(error)); - child.once('exit', code => { - if (code) { - reject(new ExitCodeError(code, name)); - } else { - resolve(); - } - }); - }); -} diff --git a/packages/cli/src/lib/versioning/packages.test.ts b/packages/cli/src/lib/versioning/packages.test.ts index de6e7e1ed2..e823ce8769 100644 --- a/packages/cli/src/lib/versioning/packages.test.ts +++ b/packages/cli/src/lib/versioning/packages.test.ts @@ -14,16 +14,17 @@ * limitations under the License. */ -import * as runObj from '../run'; +import * as runObj from '@backstage/cli-common'; import * as yarn from './yarn'; import { fetchPackageInfo, mapDependencies } from './packages'; import { createMockDirectory } from '@backstage/backend-test-utils'; import { NotFoundError } from '@backstage/errors'; -jest.mock('../run', () => { +jest.mock('@backstage/cli-common', () => { + const actual = jest.requireActual('@backstage/cli-common'); return { - run: jest.fn(), - execFile: jest.fn(), + ...actual, + runOutput: jest.fn(), }; }); @@ -39,42 +40,40 @@ describe('fetchPackageInfo', () => { }); it('should forward info for yarn classic', async () => { - jest.spyOn(runObj, 'execFile').mockResolvedValue({ - stdout: `{"type":"inspect","data":{"the":"data"}}`, - stderr: '', - }); + jest + .spyOn(runObj, 'runOutput') + .mockResolvedValue(`{"type":"inspect","data":{"the":"data"}}`); jest.spyOn(yarn, 'detectYarnVersion').mockResolvedValue('classic'); await expect(fetchPackageInfo('my-package')).resolves.toEqual({ the: 'data', }); - expect(runObj.execFile).toHaveBeenCalledWith( + expect(runObj.runOutput).toHaveBeenCalledWith([ 'yarn', - ['info', '--json', 'my-package'], - { shell: true }, - ); + 'info', + '--json', + 'my-package', + ]); }); it('should forward info for yarn berry', async () => { - jest - .spyOn(runObj, 'execFile') - .mockResolvedValue({ stdout: `{"the":"data"}`, stderr: '' }); + jest.spyOn(runObj, 'runOutput').mockResolvedValue(`{"the":"data"}`); jest.spyOn(yarn, 'detectYarnVersion').mockResolvedValue('berry'); await expect(fetchPackageInfo('my-package')).resolves.toEqual({ the: 'data', }); - expect(runObj.execFile).toHaveBeenCalledWith( + expect(runObj.runOutput).toHaveBeenCalledWith([ 'yarn', - ['npm', 'info', '--json', 'my-package'], - { shell: true }, - ); + 'npm', + 'info', + '--json', + 'my-package', + ]); }); it('should throw if no info with yarn classic', async () => { - jest - .spyOn(runObj, 'execFile') - .mockResolvedValue({ stdout: '', stderr: '' }); + jest.spyOn(runObj, 'runOutput').mockResolvedValue(''); jest.spyOn(yarn, 'detectYarnVersion').mockResolvedValue('classic'); await expect(fetchPackageInfo('my-package')).rejects.toThrow( @@ -83,9 +82,10 @@ describe('fetchPackageInfo', () => { }); it('should throw if no info with yarn berry', async () => { - jest - .spyOn(runObj, 'execFile') - .mockRejectedValue({ stdout: 'bla bla bla Response Code: 404 bla bla' }); + const error = new Error('Command failed'); + (error as Error & { stdout?: string }).stdout = + 'bla bla bla Response Code: 404 bla bla'; + jest.spyOn(runObj, 'runOutput').mockRejectedValue(error); jest.spyOn(yarn, 'detectYarnVersion').mockResolvedValue('berry'); await expect(fetchPackageInfo('my-package')).rejects.toThrow( diff --git a/packages/cli/src/lib/versioning/packages.ts b/packages/cli/src/lib/versioning/packages.ts index 223f6b8aab..11f1ba3041 100644 --- a/packages/cli/src/lib/versioning/packages.ts +++ b/packages/cli/src/lib/versioning/packages.ts @@ -17,7 +17,7 @@ import { minimatch } from 'minimatch'; import { getPackages } from '@manypkg/get-packages'; import { detectYarnVersion } from './yarn'; -import { execFile } from '../run'; +import { runOutput } from '@backstage/cli-common'; import { NotFoundError } from '@backstage/errors'; const DEP_TYPES = [ @@ -54,11 +54,7 @@ export async function fetchPackageInfo( const cmd = yarnVersion === 'classic' ? ['info'] : ['npm', 'info']; try { - const { stdout: output } = await execFile( - 'yarn', - [...cmd, '--json', name], - { shell: true }, - ); + const output = await runOutput(['yarn', ...cmd, '--json', name]); if (!output) { throw new NotFoundError( @@ -81,7 +77,12 @@ export async function fetchPackageInfo( throw error; } - if (error?.stdout.includes('Response Code: 404')) { + if ( + error instanceof Error && + 'stdout' in error && + typeof error.stdout === 'string' && + error.stdout.includes('Response Code: 404') + ) { throw new NotFoundError( `No package information found for package ${name}`, ); diff --git a/packages/cli/src/lib/versioning/yarn.ts b/packages/cli/src/lib/versioning/yarn.ts index b6c0383bc2..908ddca949 100644 --- a/packages/cli/src/lib/versioning/yarn.ts +++ b/packages/cli/src/lib/versioning/yarn.ts @@ -15,10 +15,7 @@ */ import { assertError, ForwardedError } from '@backstage/errors'; -import { execFile as execFileCb } from 'child_process'; -import { promisify } from 'util'; - -const execFile = promisify(execFileCb); +import { runOutput } from '@backstage/cli-common'; const versions = new Map>(); @@ -30,16 +27,12 @@ export function detectYarnVersion(dir?: string): Promise<'classic' | 'berry'> { const promise = Promise.resolve().then(async () => { try { - const { stdout } = await execFile('yarn', ['--version'], { - shell: true, + const stdout = await runOutput(['yarn', '--version'], { cwd, }); return stdout.trim().startsWith('1.') ? 'classic' : 'berry'; } catch (error) { assertError(error); - if ('stderr' in error) { - process.stderr.write(error.stderr as Buffer); - } throw new ForwardedError('Failed to determine yarn version', error); } }); diff --git a/packages/cli/src/modules/build/lib/bundler/config.ts b/packages/cli/src/modules/build/lib/bundler/config.ts index da444dff15..284d49d10a 100644 --- a/packages/cli/src/modules/build/lib/bundler/config.ts +++ b/packages/cli/src/modules/build/lib/bundler/config.ts @@ -29,7 +29,7 @@ import { paths as cliPaths } from '../../../../lib/paths'; import fs from 'fs-extra'; import { optimization as optimizationConfig } from './optimization'; import pickBy from 'lodash/pickBy'; -import { runPlain } from '../../../../lib/run'; +import { runOutput } from '@backstage/cli-common'; import { transforms } from './transforms'; import { version } from '../../../../lib/version'; import yn from 'yn'; @@ -78,14 +78,14 @@ async function readBuildInfo() { let commit: string | undefined; try { - commit = await runPlain('git', 'rev-parse', 'HEAD'); + commit = await runOutput(['git', 'rev-parse', 'HEAD']); } catch (error) { // ignore, see below } let gitVersion: string | undefined; try { - gitVersion = await runPlain('git', 'describe', '--always'); + gitVersion = await runOutput(['git', 'describe', '--always']); } catch (error) { // ignore, see below } diff --git a/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts b/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts index 971409cd93..cde4d666d0 100644 --- a/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts +++ b/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts @@ -25,7 +25,7 @@ import { tmpdir } from 'os'; import tar, { CreateOptions, FileOptions } from 'tar'; import partition from 'lodash/partition'; import { paths } from '../../../../lib/paths'; -import { run } from '../../../../lib/run'; +import { run } from '@backstage/cli-common'; import { dependencies as cliDependencies, devDependencies as cliDevDependencies, @@ -228,11 +228,11 @@ export async function createDistWorkspace( await runParallelWorkers({ items: customBuild, worker: async ({ name, dir, args }) => { - await run('yarn', ['run', 'build', ...(args || [])], { + await run(['yarn', 'run', 'build', ...(args || [])], { cwd: dir, stdoutLogFunc: prefixLogFunc(`${name}: `, 'stdout'), stderrLogFunc: prefixLogFunc(`${name}: `, 'stderr'), - }); + }).waitForExit(); }, }); } @@ -321,9 +321,9 @@ async function moveToDistWorkspace( console.log(`Repacking ${target.name} into dist workspace`); const archivePath = resolvePath(workspaceDir, archive); - await run('yarn', ['pack', '--filename', archivePath], { + await run(['yarn', 'pack', '--filename', archivePath], { cwd: target.dir, - }); + }).waitForExit(); const outputDir = relativePath(paths.targetRoot, target.dir); const absoluteOutputPath = resolvePath(workspaceDir, outputDir); diff --git a/packages/cli/src/modules/info/commands/info.ts b/packages/cli/src/modules/info/commands/info.ts index 416c995541..97090d8b9a 100644 --- a/packages/cli/src/modules/info/commands/info.ts +++ b/packages/cli/src/modules/info/commands/info.ts @@ -16,14 +16,14 @@ import { version as cliVersion } from '../../../../package.json'; import os from 'os'; -import { runPlain } from '../../../lib/run'; +import { runOutput } from '@backstage/cli-common'; import { paths } from '../../../lib/paths'; import { Lockfile } from '../../../lib/versioning'; import fs from 'fs-extra'; export default async () => { await new Promise(async () => { - const yarnVersion = await runPlain('yarn --version'); + const yarnVersion = await runOutput(['yarn', '--version']); const isLocal = fs.existsSync(paths.resolveOwn('./src')); const backstageFile = paths.resolveTargetRoot('backstage.json'); diff --git a/packages/cli/src/modules/maintenance/commands/repo/clean.ts b/packages/cli/src/modules/maintenance/commands/repo/clean.ts index 51c31129ea..c76076ad55 100644 --- a/packages/cli/src/modules/maintenance/commands/repo/clean.ts +++ b/packages/cli/src/modules/maintenance/commands/repo/clean.ts @@ -14,14 +14,11 @@ * limitations under the License. */ -import { execFile as execFileCb } from 'child_process'; import fs from 'fs-extra'; import { resolve as resolvePath } from 'path'; -import { promisify } from 'util'; import { PackageGraph } from '@backstage/cli-node'; import { paths } from '../../../../lib/paths'; - -const execFile = promisify(execFileCb); +import { run } from '@backstage/cli-common'; export async function command(): Promise { const packages = await PackageGraph.listTargetPackages(); @@ -44,12 +41,9 @@ export async function command(): Promise { await fs.remove(resolvePath(pkg.dir, 'dist-types')); await fs.remove(resolvePath(pkg.dir, 'coverage')); } else if (cleanScript) { - const result = await execFile('yarn', ['run', 'clean'], { + await run(['yarn', 'run', 'clean'], { cwd: pkg.dir, - shell: true, - }); - process.stdout.write(result.stdout); - process.stderr.write(result.stderr); + }).waitForExit(); } } }), diff --git a/packages/cli/src/modules/migrate/commands/packageLintConfigs.ts b/packages/cli/src/modules/migrate/commands/packageLintConfigs.ts index c8e8ef3a42..8f5e395b98 100644 --- a/packages/cli/src/modules/migrate/commands/packageLintConfigs.ts +++ b/packages/cli/src/modules/migrate/commands/packageLintConfigs.ts @@ -17,7 +17,7 @@ import fs from 'fs-extra'; import { resolve as resolvePath } from 'path'; import { PackageGraph } from '@backstage/cli-node'; -import { runPlain } from '../../../lib/run'; +import { runOutput } from '@backstage/cli-common'; const PREFIX = `module.exports = require('@backstage/cli/config/eslint-factory')`; @@ -84,6 +84,6 @@ export async function command() { } if (hasPrettier) { - await runPlain('prettier', '--write', ...configPaths); + await runOutput(['prettier', '--write', ...configPaths]); } } diff --git a/packages/cli/src/modules/migrate/commands/versions/bump.test.ts b/packages/cli/src/modules/migrate/commands/versions/bump.test.ts index 738610d09c..202704e030 100644 --- a/packages/cli/src/modules/migrate/commands/versions/bump.test.ts +++ b/packages/cli/src/modules/migrate/commands/versions/bump.test.ts @@ -15,7 +15,7 @@ */ import fs from 'fs-extra'; import { Command } from 'commander'; -import * as runObj from '../../../../lib/run'; +import * as runObj from '@backstage/cli-common'; import bump, { bumpBackstageJsonVersion, createVersionFinder } from './bump'; import { registerMswTestHooks, withLogCollector } from '@backstage/test-utils'; import { YarnInfoInspectData } from '../../../../lib/versioning/packages'; @@ -60,21 +60,22 @@ jest.mock('ora', () => ({ })); let mockDir: MockDirectory; -jest.mock('@backstage/cli-common', () => ({ - ...jest.requireActual('@backstage/cli-common'), - findPaths: () => ({ - resolveTargetRoot(filename: string) { - return mockDir.resolve(filename); - }, - get targetDir() { - return mockDir.path; - }, - }), -})); - -jest.mock('../../../../lib/run', () => { +jest.mock('@backstage/cli-common', () => { + const actual = jest.requireActual('@backstage/cli-common'); return { - run: jest.fn(), + ...actual, + findPaths: () => ({ + resolveTargetRoot(filename: string) { + return mockDir.resolve(filename); + }, + get targetDir() { + return mockDir.path; + }, + }), + run: jest.fn().mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + }), }; }); @@ -184,7 +185,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -222,8 +226,7 @@ describe('bump', () => { expect(runObj.run).toHaveBeenCalledTimes(1); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -277,7 +280,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -318,8 +324,7 @@ describe('bump', () => { expect(mockFetchPackageInfo).toHaveBeenCalledWith('@backstage/theme'); expect(runObj.run).not.toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -373,7 +378,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -421,8 +429,7 @@ describe('bump', () => { expect(runObj.run).toHaveBeenCalledTimes(1); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -477,7 +484,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -526,14 +536,14 @@ describe('bump', () => { expect(mockFetchPackageInfo).toHaveBeenCalledWith('@backstage/core'); expect(runObj.run).toHaveBeenCalledTimes(2); - expect(runObj.run).toHaveBeenCalledWith('yarn', [ + expect(runObj.run).toHaveBeenCalledWith([ + 'yarn', 'plugin', 'import', 'https://versions.backstage.io/v1/releases/0.0.1/yarn-plugin', ]); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -587,7 +597,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/releases/999.0.1/manifest.json', @@ -656,7 +669,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -763,7 +779,10 @@ describe('bump', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -813,8 +832,7 @@ describe('bump', () => { expect(runObj.run).toHaveBeenCalledTimes(1); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -873,7 +891,10 @@ describe('bump', () => { }); mockFetchPackageInfo.mockRejectedValue(new NotFoundError('Nope')); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://versions.backstage.io/v1/tags/main/manifest.json', @@ -1094,7 +1115,10 @@ describe('environment variables', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://custom.example.com/v1/tags/main/manifest.json', @@ -1131,8 +1155,7 @@ describe('environment variables', () => { expect(runObj.run).toHaveBeenCalledTimes(1); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -1186,7 +1209,10 @@ describe('environment variables', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); const { log: logs } = await withLogCollector(['log', 'warn'], async () => { await bump({ pattern: null, release: 'main' } as unknown as Command); @@ -1215,8 +1241,7 @@ describe('environment variables', () => { expect(runObj.run).toHaveBeenCalledTimes(1); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); @@ -1253,7 +1278,10 @@ describe('environment variables', () => { }, }); - jest.spyOn(runObj, 'run').mockResolvedValue(undefined); + jest.spyOn(runObj, 'run').mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + } as any); worker.use( rest.get( 'https://custom.example.com/v1/tags/main/manifest.json', @@ -1291,14 +1319,14 @@ describe('environment variables', () => { ]); expect(runObj.run).toHaveBeenCalledTimes(2); - expect(runObj.run).toHaveBeenCalledWith('yarn', [ + expect(runObj.run).toHaveBeenCalledWith([ + 'yarn', 'plugin', 'import', 'https://custom.example.com/v1/releases/1.5.0/yarn-plugin', ]); expect(runObj.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + ['yarn', 'install'], expect.any(Object), ); }); diff --git a/packages/cli/src/modules/migrate/commands/versions/bump.ts b/packages/cli/src/modules/migrate/commands/versions/bump.ts index 19dcaae28f..cad477188b 100644 --- a/packages/cli/src/modules/migrate/commands/versions/bump.ts +++ b/packages/cli/src/modules/migrate/commands/versions/bump.ts @@ -41,7 +41,7 @@ import { } from '@backstage/release-manifests'; import { migrateMovedPackages } from './migrate'; import { runYarnInstall } from '../../lib/utils'; -import { run } from '../../../../lib/run'; +import { run } from '@backstage/cli-common'; const DEP_TYPES = [ 'dependencies', @@ -135,7 +135,7 @@ export default async (opts: OptionValues) => { ? `${env.BACKSTAGE_VERSIONS_BASE_URL}/v1/releases/${releaseManifest.releaseVersion}/yarn-plugin` : `https://versions.backstage.io/v1/releases/${releaseManifest.releaseVersion}/yarn-plugin`; - await run('yarn', ['plugin', 'import', yarnPluginUrl]); + await run(['yarn', 'plugin', 'import', yarnPluginUrl]).waitForExit(); console.log(); } diff --git a/packages/cli/src/modules/migrate/commands/versions/migrate.test.ts b/packages/cli/src/modules/migrate/commands/versions/migrate.test.ts index 9ff00fd285..936d10e24c 100644 --- a/packages/cli/src/modules/migrate/commands/versions/migrate.test.ts +++ b/packages/cli/src/modules/migrate/commands/versions/migrate.test.ts @@ -17,7 +17,7 @@ import { MockDirectory, createMockDirectory, } from '@backstage/backend-test-utils'; -import * as run from '../../../../lib/run'; +import * as runObj from '@backstage/cli-common'; import migrate from './migrate'; import { withLogCollector } from '@backstage/test-utils'; import fs from 'fs-extra'; @@ -33,21 +33,22 @@ jest.mock('chalk', () => ({ })); let mockDir: MockDirectory; -jest.mock('@backstage/cli-common', () => ({ - ...jest.requireActual('@backstage/cli-common'), - findPaths: () => ({ - resolveTargetRoot(filename: string) { - return mockDir.resolve(filename); - }, - get targetDir() { - return mockDir.path; - }, - }), -})); - -jest.mock('../../../../lib/run', () => { +jest.mock('@backstage/cli-common', () => { + const actual = jest.requireActual('@backstage/cli-common'); return { - run: jest.fn(), + ...actual, + findPaths: () => ({ + resolveTargetRoot(filename: string) { + return mockDir.resolve(filename); + }, + get targetDir() { + return mockDir.path; + }, + }), + run: jest.fn().mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + }), }; }); @@ -58,8 +59,15 @@ function expectLogsToMatch(receivedLogs: String[], expected: String[]): void { describe('versions:migrate', () => { mockDir = createMockDirectory(); + beforeEach(() => { + (runObj.run as jest.Mock).mockReturnValue({ + exitCode: null, + waitForExit: jest.fn().mockResolvedValue(undefined), + }); + }); + afterEach(() => { - jest.resetAllMocks(); + (runObj.run as jest.Mock).mockClear(); }); it('should bump to the moved version when the package is moved', async () => { @@ -116,8 +124,6 @@ describe('versions:migrate', () => { }, }); - jest.spyOn(run, 'run').mockResolvedValue(undefined); - const { warn, log: logs } = await withLogCollector(async () => { await migrate({}); }); @@ -136,10 +142,9 @@ describe('versions:migrate', () => { 'Could not find package.json for @backstage/theme@^1.0.0 in b (dependencies)', ]); - expect(run.run).toHaveBeenCalledTimes(1); - expect(run.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + expect(runObj.run).toHaveBeenCalledTimes(1); + expect(runObj.run).toHaveBeenCalledWith( + ['yarn', 'install'], expect.any(Object), ); @@ -227,16 +232,13 @@ describe('versions:migrate', () => { }, }); - jest.spyOn(run, 'run').mockResolvedValue(undefined); - await withLogCollector(async () => { await migrate({}); }); - expect(run.run).toHaveBeenCalledTimes(1); - expect(run.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + expect(runObj.run).toHaveBeenCalledTimes(1); + expect(runObj.run).toHaveBeenCalledWith( + ['yarn', 'install'], expect.any(Object), ); @@ -259,7 +261,7 @@ describe('versions:migrate', () => { ); }); - it('should replaces the occurrences of changed packages, and is careful', async () => { + it('should replace occurrences of changed packages, and is careful', async () => { mockDir.setContent({ 'package.json': JSON.stringify({ workspaces: { @@ -314,16 +316,13 @@ describe('versions:migrate', () => { }, }); - jest.spyOn(run, 'run').mockResolvedValue(undefined); - await withLogCollector(async () => { await migrate({}); }); - expect(run.run).toHaveBeenCalledTimes(1); - expect(run.run).toHaveBeenCalledWith( - 'yarn', - ['install'], + expect(runObj.run).toHaveBeenCalledTimes(1); + expect(runObj.run).toHaveBeenCalledWith( + ['yarn', 'install'], expect.any(Object), ); diff --git a/packages/cli/src/modules/migrate/lib/utils.ts b/packages/cli/src/modules/migrate/lib/utils.ts index 5b7b544c87..955cefde1d 100644 --- a/packages/cli/src/modules/migrate/lib/utils.ts +++ b/packages/cli/src/modules/migrate/lib/utils.ts @@ -16,7 +16,7 @@ import ora from 'ora'; import chalk from 'chalk'; -import { run } from '../../../lib/run'; +import { run } from '@backstage/cli-common'; export async function runYarnInstall() { const spinner = ora({ @@ -27,7 +27,7 @@ export async function runYarnInstall() { const installOutput = new Array(); try { - await run('yarn', ['install'], { + await run(['yarn', 'install'], { env: { FORCE_COLOR: 'true', // We filter out all of the npm_* environment variables that are added when @@ -41,7 +41,7 @@ export async function runYarnInstall() { }, stdoutLogFunc: data => installOutput.push(data), stderrLogFunc: data => installOutput.push(data), - }); + }).waitForExit(); spinner.succeed(); } catch (error) { spinner.fail(); diff --git a/packages/cli/src/modules/new/lib/tasks.ts b/packages/cli/src/modules/new/lib/tasks.ts index 2922f34f35..b0003e1053 100644 --- a/packages/cli/src/modules/new/lib/tasks.ts +++ b/packages/cli/src/modules/new/lib/tasks.ts @@ -16,11 +16,8 @@ import chalk from 'chalk'; import ora from 'ora'; -import { promisify } from 'util'; -import { exec as execCb } from 'child_process'; import { assertError } from '@backstage/errors'; - -const exec = promisify(execCb); +import { run } from '@backstage/cli-common'; const TASK_NAME_MAX_LENGTH = 14; @@ -71,16 +68,11 @@ export class Task { ) { try { await Task.forItem('executing', command, async () => { - await exec(command, { cwd: options?.cwd }); + const parts = command.trim().split(/\s+/); + await run(parts, { cwd: options?.cwd }).waitForExit(); }); } catch (error) { assertError(error); - if (error.stderr) { - process.stderr.write(error.stderr as Buffer); - } - if (error.stdout) { - process.stdout.write(error.stdout as Buffer); - } if (options?.optional) { Task.error(`Warning: Failed to execute command ${chalk.cyan(command)}`); } else { diff --git a/packages/cli/src/modules/test/commands/package/test.ts b/packages/cli/src/modules/test/commands/package/test.ts index e7ef0af302..1ecf5b395e 100644 --- a/packages/cli/src/modules/test/commands/package/test.ts +++ b/packages/cli/src/modules/test/commands/package/test.ts @@ -16,7 +16,7 @@ import { Command, OptionValues } from 'commander'; import { paths } from '../../../../lib/paths'; -import { runCheck } from '../../../../lib/run'; +import { runCheck } from '@backstage/cli-common'; function includesAnyOf(hayStack: string[], ...needles: string[]) { for (const needle of needles) { @@ -55,8 +55,8 @@ export default async (_opts: OptionValues, cmd: Command) => { !includesAnyOf(args, '--watch', '--watchAll') ) { const isGitRepo = () => - runCheck('git', 'rev-parse', '--is-inside-work-tree'); - const isMercurialRepo = () => runCheck('hg', '--cwd', '.', 'root'); + runCheck(['git', 'rev-parse', '--is-inside-work-tree']); + const isMercurialRepo = () => runCheck(['hg', '--cwd', '.', 'root']); if ((await isGitRepo()) || (await isMercurialRepo())) { args.push('--watch'); diff --git a/packages/cli/src/modules/test/commands/repo/test.ts b/packages/cli/src/modules/test/commands/repo/test.ts index f0520d29e2..fd18ba09fe 100644 --- a/packages/cli/src/modules/test/commands/repo/test.ts +++ b/packages/cli/src/modules/test/commands/repo/test.ts @@ -22,7 +22,7 @@ import { relative as relativePath } from 'path'; import { Command, OptionValues } from 'commander'; import { Lockfile, PackageGraph } from '@backstage/cli-node'; import { paths } from '../../../../lib/paths'; -import { runCheck, runPlain } from '../../../../lib/run'; +import { runCheck, runOutput } from '@backstage/cli-common'; import { isChildPath } from '@backstage/cli-common'; import { SuccessCache } from '../../../../lib/cache/SuccessCache'; @@ -63,14 +63,14 @@ async function readPackageTreeHashes(graph: PackageGraph) { ...pkg, path: relativePath(paths.targetRoot, pkg.dir), })); - const output = await runPlain( + const output = await runOutput([ 'git', 'ls-tree', - '--format="%(objectname)=%(path)"', + '--format=%(objectname)=%(path)', 'HEAD', '--', ...pkgs.map(pkg => pkg.path), - ); + ]); const map = new Map( output @@ -175,8 +175,8 @@ export async function command(opts: OptionValues, cmd: Command): Promise { !hasFlags('--coverage', '--watch', '--watchAll') ) { const isGitRepo = () => - runCheck('git', 'rev-parse', '--is-inside-work-tree'); - const isMercurialRepo = () => runCheck('hg', '--cwd', '.', 'root'); + runCheck(['git', 'rev-parse', '--is-inside-work-tree']); + const isMercurialRepo = () => runCheck(['hg', '--cwd', '.', 'root']); if ((await isGitRepo()) || (await isMercurialRepo())) { isSingleWatchMode = true; From ab13a897441e886a81f46ffdcbf4a9ceef02ef6d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 17:11:50 +0100 Subject: [PATCH 06/10] cli-common: polish API surface for run Signed-off-by: Patrik Oldsberg --- packages/cli-common/report.api.md | 6 ++-- packages/cli-common/src/index.ts | 2 +- packages/cli-common/src/run.ts | 33 ++++++++----------- .../build/lib/packager/createDistWorkspace.ts | 4 +-- packages/cli/src/modules/migrate/lib/utils.ts | 4 +-- .../techdocs-cli/src/commands/serve/mkdocs.ts | 8 ++--- .../techdocs-cli/src/commands/serve/serve.ts | 8 ++--- packages/techdocs-cli/src/lib/mkdocsServer.ts | 14 ++++---- 8 files changed, 37 insertions(+), 42 deletions(-) diff --git a/packages/cli-common/report.api.md b/packages/cli-common/report.api.md index a2aba06228..175ec3cb2b 100644 --- a/packages/cli-common/report.api.md +++ b/packages/cli-common/report.api.md @@ -53,13 +53,13 @@ export interface RunChildProcess extends ChildProcess { } // @public -export type RunLogFunc = (data: Buffer) => void; +export type RunOnOutput = (data: Buffer) => void; // @public export type RunOptions = Omit & { env?: Partial; - stdoutLogFunc?: RunLogFunc; - stderrLogFunc?: RunLogFunc; + onStdout?: RunOnOutput; + onStderr?: RunOnOutput; stdio?: SpawnOptions['stdio']; }; diff --git a/packages/cli-common/src/index.ts b/packages/cli-common/src/index.ts index 632aff9eb6..d11601b044 100644 --- a/packages/cli-common/src/index.ts +++ b/packages/cli-common/src/index.ts @@ -30,6 +30,6 @@ export { runCheck, type RunChildProcess, type RunOptions, - type RunLogFunc, + type RunOnOutput, } from './run'; export { ExitCodeError } from './errors'; diff --git a/packages/cli-common/src/run.ts b/packages/cli-common/src/run.ts index 9a0a0975c1..b64d2bb94b 100644 --- a/packages/cli-common/src/run.ts +++ b/packages/cli-common/src/run.ts @@ -24,7 +24,7 @@ import { assertError } from '@backstage/errors'; * * @public */ -export type RunLogFunc = (data: Buffer) => void; +export type RunOnOutput = (data: Buffer) => void; /** * Options for running a child process with {@link run} or related functions. @@ -33,8 +33,8 @@ export type RunLogFunc = (data: Buffer) => void; */ export type RunOptions = Omit & { env?: Partial; - stdoutLogFunc?: RunLogFunc; - stderrLogFunc?: RunLogFunc; + onStdout?: RunOnOutput; + onStderr?: RunOnOutput; stdio?: SpawnOptions['stdio']; }; @@ -69,12 +69,7 @@ export function run(args: string[], options: RunOptions = {}): RunChildProcess { const [name, ...cmdArgs] = args; - const { - stdoutLogFunc, - stderrLogFunc, - stdio: customStdio, - ...spawnOptions - } = options; + const { onStdout, onStderr, stdio: customStdio, ...spawnOptions } = options; const env: NodeJS.ProcessEnv = { ...process.env, FORCE_COLOR: 'true', @@ -85,8 +80,8 @@ export function run(args: string[], options: RunOptions = {}): RunChildProcess { customStdio ?? ([ 'inherit', - stdoutLogFunc ? 'pipe' : 'inherit', - stderrLogFunc ? 'pipe' : 'inherit', + onStdout ? 'pipe' : 'inherit', + onStderr ? 'pipe' : 'inherit', ] as ('inherit' | 'pipe')[]); const child = spawn(name, cmdArgs, { @@ -95,11 +90,11 @@ export function run(args: string[], options: RunOptions = {}): RunChildProcess { env, }) as RunChildProcess; - if (stdoutLogFunc && child.stdout) { - child.stdout.on('data', stdoutLogFunc); + if (onStdout && child.stdout) { + child.stdout.on('data', onStdout); } - if (stderrLogFunc && child.stderr) { - child.stderr.on('data', stderrLogFunc); + if (onStderr && child.stderr) { + child.stderr.on('data', onStderr); } const commandName = args.join(' '); @@ -175,13 +170,13 @@ export async function runOutput( try { await run(args, { ...options, - stdoutLogFunc: data => { + onStdout: data => { stdoutChunks.push(data); - options?.stdoutLogFunc?.(data); + options?.onStdout?.(data); }, - stderrLogFunc: data => { + onStderr: data => { stderrChunks.push(data); - options?.stderrLogFunc?.(data); + options?.onStderr?.(data); }, }).waitForExit(); diff --git a/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts b/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts index cde4d666d0..90635f1a73 100644 --- a/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts +++ b/packages/cli/src/modules/build/lib/packager/createDistWorkspace.ts @@ -230,8 +230,8 @@ export async function createDistWorkspace( worker: async ({ name, dir, args }) => { await run(['yarn', 'run', 'build', ...(args || [])], { cwd: dir, - stdoutLogFunc: prefixLogFunc(`${name}: `, 'stdout'), - stderrLogFunc: prefixLogFunc(`${name}: `, 'stderr'), + onStdout: prefixLogFunc(`${name}: `, 'stdout'), + onStderr: prefixLogFunc(`${name}: `, 'stderr'), }).waitForExit(); }, }); diff --git a/packages/cli/src/modules/migrate/lib/utils.ts b/packages/cli/src/modules/migrate/lib/utils.ts index 955cefde1d..c85f56dabb 100644 --- a/packages/cli/src/modules/migrate/lib/utils.ts +++ b/packages/cli/src/modules/migrate/lib/utils.ts @@ -39,8 +39,8 @@ export async function runYarnInstall() { ), ), }, - stdoutLogFunc: data => installOutput.push(data), - stderrLogFunc: data => installOutput.push(data), + onStdout: data => installOutput.push(data), + onStderr: data => installOutput.push(data), }).waitForExit(); spinner.succeed(); } catch (error) { diff --git a/packages/techdocs-cli/src/commands/serve/mkdocs.ts b/packages/techdocs-cli/src/commands/serve/mkdocs.ts index 3938764fed..cd1072a88f 100644 --- a/packages/techdocs-cli/src/commands/serve/mkdocs.ts +++ b/packages/techdocs-cli/src/commands/serve/mkdocs.ts @@ -18,7 +18,7 @@ import { OptionValues } from 'commander'; import openBrowser from 'react-dev-utils/openBrowser'; import { createLogger } from '../../lib/utility'; import { runMkdocsServer } from '../../lib/mkdocsServer'; -import { RunLogFunc } from '@backstage/cli-common'; +import { RunOnOutput } from '@backstage/cli-common'; import { getMkdocsYml } from '@backstage/plugin-techdocs-node'; import fs from 'fs-extra'; import { checkIfDockerIsOperational } from './utils'; @@ -45,7 +45,7 @@ export default async function serveMkdocs(opts: OptionValues) { // We want to open browser only once based on a log. let boolOpenBrowserTriggered = false; - const logFunc: RunLogFunc = data => { + const logFunc: RunOnOutput = data => { // Sometimes the lines contain an unnecessary extra new line in between const logLines = data.toString().split('\n'); const logPrefix = opts.docker ? '[docker/mkdocs]' : '[mkdocs]'; @@ -80,8 +80,8 @@ export default async function serveMkdocs(opts: OptionValues) { dockerEntrypoint: opts.dockerEntrypoint, dockerOptions: opts.dockerOption, useDocker: opts.docker, - stdoutLogFunc: logFunc, - stderrLogFunc: logFunc, + onStdout: logFunc, + onStderr: logFunc, }); // Keep waiting for user to cancel the process diff --git a/packages/techdocs-cli/src/commands/serve/serve.ts b/packages/techdocs-cli/src/commands/serve/serve.ts index c325a99871..a0d0030c6b 100644 --- a/packages/techdocs-cli/src/commands/serve/serve.ts +++ b/packages/techdocs-cli/src/commands/serve/serve.ts @@ -17,7 +17,7 @@ import { OptionValues } from 'commander'; import path from 'path'; import openBrowser from 'react-dev-utils/openBrowser'; -import { findPaths, RunLogFunc } from '@backstage/cli-common'; +import { findPaths, RunOnOutput } from '@backstage/cli-common'; import HTTPServer from '../../lib/httpServer'; import { runMkdocsServer } from '../../lib/mkdocsServer'; import { createLogger } from '../../lib/utility'; @@ -82,7 +82,7 @@ export default async function serve(opts: OptionValues) { } let mkdocsServerHasStarted = false; - const mkdocsLogFunc: RunLogFunc = data => { + const mkdocsLogFunc: RunOnOutput = data => { // Sometimes the lines contain an unnecessary extra new line const logLines = data.toString().split('\n'); const logPrefix = opts.docker ? '[docker/mkdocs]' : '[mkdocs]'; @@ -112,8 +112,8 @@ export default async function serve(opts: OptionValues) { dockerEntrypoint: opts.dockerEntrypoint, dockerOptions: opts.dockerOption, useDocker: opts.docker, - stdoutLogFunc: mkdocsLogFunc, - stderrLogFunc: mkdocsLogFunc, + onStdout: mkdocsLogFunc, + onStderr: mkdocsLogFunc, mkdocsConfigFileName: mkdocsYmlPath, mkdocsParameterClean: opts.mkdocsParameterClean, mkdocsParameterDirtyReload: opts.mkdocsParameterDirtyreload, diff --git a/packages/techdocs-cli/src/lib/mkdocsServer.ts b/packages/techdocs-cli/src/lib/mkdocsServer.ts index 929ee77fb1..c557015256 100644 --- a/packages/techdocs-cli/src/lib/mkdocsServer.ts +++ b/packages/techdocs-cli/src/lib/mkdocsServer.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { run, RunChildProcess, RunLogFunc } from '@backstage/cli-common'; +import { run, RunChildProcess, RunOnOutput } from '@backstage/cli-common'; export const runMkdocsServer = (options: { port?: string; @@ -22,8 +22,8 @@ export const runMkdocsServer = (options: { dockerImage?: string; dockerEntrypoint?: string; dockerOptions?: string[]; - stdoutLogFunc?: RunLogFunc; - stderrLogFunc?: RunLogFunc; + onStdout?: RunOnOutput; + onStderr?: RunOnOutput; mkdocsConfigFileName?: string; mkdocsParameterClean?: boolean; mkdocsParameterDirtyReload?: boolean; @@ -62,8 +62,8 @@ export const runMkdocsServer = (options: { ...(options.mkdocsParameterStrict ? ['--strict'] : []), ], { - stdoutLogFunc: options.stdoutLogFunc, - stderrLogFunc: options.stderrLogFunc, + onStdout: options.onStdout, + onStderr: options.onStderr, }, ); } @@ -82,8 +82,8 @@ export const runMkdocsServer = (options: { ...(options.mkdocsParameterStrict ? ['--strict'] : []), ], { - stdoutLogFunc: options.stdoutLogFunc, - stderrLogFunc: options.stderrLogFunc, + onStdout: options.onStdout, + onStderr: options.onStderr, }, ); }; From 688f070676386081029d54ec5a34ec4438b2aef3 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 18:11:45 +0100 Subject: [PATCH 07/10] codemods,repo-tools: update to use new run utils from cli-common Signed-off-by: Patrik Oldsberg --- .changeset/funny-papayas-rest.md | 6 +++ packages/codemods/src/action.ts | 39 +++++-------------- packages/codemods/src/errors.ts | 14 +------ .../api-reports/generateTypeDeclarations.ts | 39 +++++++++++-------- 4 files changed, 39 insertions(+), 59 deletions(-) create mode 100644 .changeset/funny-papayas-rest.md diff --git a/.changeset/funny-papayas-rest.md b/.changeset/funny-papayas-rest.md new file mode 100644 index 0000000000..95f52966ec --- /dev/null +++ b/.changeset/funny-papayas-rest.md @@ -0,0 +1,6 @@ +--- +'@backstage/repo-tools': patch +'@backstage/codemods': patch +--- + +Updated to use new utilities from `@backstage/cli-common`. diff --git a/packages/codemods/src/action.ts b/packages/codemods/src/action.ts index b955ab17ea..467dd35f82 100644 --- a/packages/codemods/src/action.ts +++ b/packages/codemods/src/action.ts @@ -15,11 +15,9 @@ */ import { relative as relativePath } from 'path'; -import { spawn } from 'child_process'; import { OptionValues } from 'commander'; -import { findPaths } from '@backstage/cli-common'; +import { findPaths, run } from '@backstage/cli-common'; import { platform } from 'os'; -import { ExitCodeError } from './errors'; // eslint-disable-next-line no-restricted-syntax const paths = findPaths(__dirname); @@ -51,42 +49,25 @@ export function createCodemodAction(name: string) { console.log(`Running jscodeshift with these arguments: ${args.join(' ')}`); - let command; + let commandArgs: string[]; if (platform() === 'win32') { - command = 'jscodeshift'; + commandArgs = ['jscodeshift', ...args]; } else { // jscodeshift ships a slightly broken bin script with windows // line endings so we need to execute it using node rather than // letting the `#!/usr/bin/env node` take care of it - command = process.argv0; - args.unshift(require.resolve('.bin/jscodeshift')); + commandArgs = [ + process.argv0, + require.resolve('.bin/jscodeshift'), + ...args, + ]; } - const child = spawn(command, args, { - stdio: 'inherit', - shell: true, + await run(commandArgs, { env: { ...process.env, FORCE_COLOR: 'true', }, - }); - - if (typeof child.exitCode === 'number') { - if (child.exitCode) { - throw new ExitCodeError(child.exitCode, name); - } - return; - } - - await new Promise((resolve, reject) => { - child.once('error', error => reject(error)); - child.once('exit', code => { - if (code) { - reject(new ExitCodeError(code, name)); - } else { - resolve(); - } - }); - }); + }).waitForExit(); }; } diff --git a/packages/codemods/src/errors.ts b/packages/codemods/src/errors.ts index 2f67b94ae1..e27f66895c 100644 --- a/packages/codemods/src/errors.ts +++ b/packages/codemods/src/errors.ts @@ -15,6 +15,7 @@ */ import chalk from 'chalk'; +import { ExitCodeError } from '@backstage/cli-common'; export class CustomError extends Error { get name(): string { @@ -22,19 +23,6 @@ export class CustomError extends Error { } } -export class ExitCodeError extends CustomError { - readonly code: number; - - constructor(code: number, command?: string) { - if (command) { - super(`Command '${command}' exited with code ${code}`); - } else { - super(`Child exited with code ${code}`); - } - this.code = code; - } -} - export function exitWithError(error: Error): never { if (error instanceof ExitCodeError) { process.stderr.write(`\n${chalk.red(error.message)}\n\n`); diff --git a/packages/repo-tools/src/commands/api-reports/api-reports/generateTypeDeclarations.ts b/packages/repo-tools/src/commands/api-reports/api-reports/generateTypeDeclarations.ts index d7304553c0..a51c9ed17b 100644 --- a/packages/repo-tools/src/commands/api-reports/api-reports/generateTypeDeclarations.ts +++ b/packages/repo-tools/src/commands/api-reports/api-reports/generateTypeDeclarations.ts @@ -15,7 +15,7 @@ */ import fs from 'fs-extra'; -import { spawnSync } from 'child_process'; +import { run, ExitCodeError } from '@backstage/cli-common'; import { paths as cliPaths } from '../../../lib/paths'; /** @@ -30,21 +30,26 @@ import { paths as cliPaths } from '../../../lib/paths'; */ export async function generateTypeDeclarations(tsconfigFilePath: string) { await fs.remove(cliPaths.resolveTargetRoot('dist-types')); - const { status } = spawnSync( - 'yarn', - [ - 'tsc', - ['--project', tsconfigFilePath], - ['--skipLibCheck', 'false'], - ['--incremental', 'false'], - ].flat(), - { - stdio: 'inherit', - shell: true, - cwd: cliPaths.targetRoot, - }, - ); - if (status !== 0) { - process.exit(status || undefined); + try { + await run( + [ + 'yarn', + 'tsc', + '--project', + tsconfigFilePath, + '--skipLibCheck', + 'false', + '--incremental', + 'false', + ], + { + cwd: cliPaths.targetRoot, + }, + ).waitForExit(); + } catch (error) { + if (error instanceof ExitCodeError) { + process.exit(error.code); + } + throw error; } } From 788ed91649c7d90bac415e3675d7c26d63a5d87e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 18:55:47 +0100 Subject: [PATCH 08/10] cli-common: cover run utils with tests Signed-off-by: Patrik Oldsberg --- packages/cli-common/src/run.test.ts | 310 ++++++++++++++++++++++++++++ 1 file changed, 310 insertions(+) create mode 100644 packages/cli-common/src/run.test.ts diff --git a/packages/cli-common/src/run.test.ts b/packages/cli-common/src/run.test.ts new file mode 100644 index 0000000000..04df5fe05f --- /dev/null +++ b/packages/cli-common/src/run.test.ts @@ -0,0 +1,310 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { run, runOutput, runCheck } from './run'; +import { ExitCodeError } from './errors'; + +describe('run', () => { + const activeChildren: Array<{ kill: () => void }> = []; + + afterEach(() => { + // Clean up any active child processes + activeChildren.forEach(child => { + try { + child.kill(); + } catch { + // Ignore errors during cleanup + } + }); + activeChildren.length = 0; + jest.restoreAllMocks(); + }); + + describe('run', () => { + it('should throw error for empty args', () => { + expect(() => run([])).toThrow('run requires at least one argument'); + }); + + it('should run a successful command', async () => { + const child = run(['node', '--version']); + activeChildren.push(child); + await expect(child.waitForExit()).resolves.not.toThrow(); + }); + + it('should throw ExitCodeError for non-zero exit code', async () => { + const child = run(['node', '--eval', 'process.exit(1)']); + activeChildren.push(child); + await expect(child.waitForExit()).rejects.toThrow(ExitCodeError); + await expect(child.waitForExit()).rejects.toThrow( + /Command 'node --eval process\.exit\(1\)' exited with code 1/, + ); + }); + + it('should call onStdout callback', async () => { + const stdoutChunks: Buffer[] = []; + const child = run(['node', '--version'], { + onStdout: data => stdoutChunks.push(data), + }); + activeChildren.push(child); + await child.waitForExit(); + expect(stdoutChunks.length).toBeGreaterThan(0); + const output = Buffer.concat(stdoutChunks).toString(); + expect(output).toMatch(/v\d+\.\d+\.\d+/); + }); + + it('should call onStderr callback', async () => { + const stderrChunks: Buffer[] = []; + const child = run(['node', '--eval', 'console.error("test error")'], { + onStderr: data => stderrChunks.push(data), + }); + activeChildren.push(child); + await child.waitForExit(); + expect(stderrChunks.length).toBeGreaterThan(0); + const output = Buffer.concat(stderrChunks).toString(); + expect(output).toContain('test error'); + }); + + it('should use custom stdio', async () => { + const child = run(['node', '--version'], { + stdio: 'pipe', + }); + activeChildren.push(child); + expect(child.stdout).toBeTruthy(); + expect(child.stderr).toBeTruthy(); + await child.waitForExit(); + }); + + it('should use custom env', async () => { + const customEnv = { CUSTOM_VAR: 'test-value' }; + const child = run( + ['node', '--eval', 'console.log(process.env.CUSTOM_VAR)'], + { + env: customEnv, + onStdout: data => { + const output = data.toString(); + expect(output).toContain('test-value'); + }, + }, + ); + activeChildren.push(child); + await child.waitForExit(); + }); + + it('should set FORCE_COLOR in env', async () => { + const child = run( + ['node', '--eval', 'console.log(process.env.FORCE_COLOR)'], + { + onStdout: data => { + const output = data.toString(); + expect(output.trim()).toBe('true'); + }, + }, + ); + activeChildren.push(child); + await child.waitForExit(); + }); + + it('should handle process already exited', async () => { + const child = run(['node', '--version']); + activeChildren.push(child); + // Wait for it to complete + await child.waitForExit(); + await expect( + Promise.race([child.waitForExit(), 'pending']), + ).resolves.not.toBe('pending'); + }); + + it('should handle signal handlers cleanup', async () => { + const child = run(['node', '--version']); + activeChildren.push(child); + const originalListeners = process.listenerCount('SIGINT'); + await child.waitForExit(); + // Signal handlers should be cleaned up + expect(process.listenerCount('SIGINT')).toBe(originalListeners); + }); + + it('should kill child process on SIGINT', async () => { + const child = run(['node', '--eval', 'setTimeout(() => {}, 10000)']); + activeChildren.push(child); + const killSpy = jest.spyOn(child, 'kill'); + // Start waiting (this registers signal handlers) + const waitPromise = child.waitForExit(); + // Simulate SIGINT + process.emit('SIGINT' as any, 'SIGINT'); + // Give it a moment + await new Promise(resolve => setTimeout(resolve, 100)); + expect(killSpy).toHaveBeenCalled(); + killSpy.mockRestore(); + // Clean up + child.kill(); + // Wait for cleanup + try { + await Promise.race([ + waitPromise, + new Promise(resolve => setTimeout(resolve, 100)), + ]); + } catch { + // Expected to fail + } + }); + + it('should kill child process on SIGTERM', async () => { + const child = run(['node', '--eval', 'setTimeout(() => {}, 10000)']); + activeChildren.push(child); + const killSpy = jest.spyOn(child, 'kill'); + // Start waiting (this registers signal handlers) + const waitPromise = child.waitForExit(); + // Simulate SIGTERM + process.emit('SIGTERM' as any, 'SIGTERM'); + // Give it a moment + await new Promise(resolve => setTimeout(resolve, 100)); + expect(killSpy).toHaveBeenCalled(); + killSpy.mockRestore(); + // Clean up + child.kill(); + // Wait for cleanup + try { + await Promise.race([ + waitPromise, + new Promise(resolve => setTimeout(resolve, 100)), + ]); + } catch { + // Expected to fail + } + }); + + it('should not kill already killed process on signal', async () => { + const child = run(['node', '--version']); + activeChildren.push(child); + await child.waitForExit(); + const killSpy = jest.spyOn(child, 'kill'); + // Simulate SIGINT after process has exited + process.emit('SIGINT' as any, 'SIGINT'); + await new Promise(resolve => setTimeout(resolve, 100)); + // Should not be called since process already exited + expect(killSpy).not.toHaveBeenCalled(); + killSpy.mockRestore(); + }); + + it('should handle process error', async () => { + const child = run(['nonexistent-command-12345']); + activeChildren.push(child); + await expect(child.waitForExit()).rejects.toThrow(); + }); + }); + + describe('runOutput', () => { + it('should throw error for empty args', async () => { + await expect(runOutput([])).rejects.toThrow( + 'runOutput requires at least one argument', + ); + }); + + it('should return stdout', async () => { + const output = await runOutput([ + 'node', + '--eval', + 'console.log("test output")', + ]); + expect(output).toBe('test output'); + }); + + it('should trim output', async () => { + const output = await runOutput([ + 'node', + '--eval', + 'console.log(" test output ")', + ]); + expect(output).toBe('test output'); + }); + + it('should attach stdout to error on failure', async () => { + let error: Error | undefined; + try { + await runOutput([ + 'node', + '--eval', + 'console.log("stdout before error"); process.exit(1)', + ]); + } catch (err) { + error = err; + } + expect(error).toBeInstanceOf(ExitCodeError); + expect((error as Error & { stdout?: string }).stdout).toContain( + 'stdout before error', + ); + }); + + it('should attach stderr to error on failure', async () => { + let error: Error | undefined; + try { + await runOutput([ + 'node', + '--eval', + 'console.error("stderr error"); process.exit(1)', + ]); + } catch (err) { + error = err; + } + expect(error).toBeInstanceOf(ExitCodeError); + expect((error as Error & { stderr?: string }).stderr).toContain( + 'stderr error', + ); + }); + + it('should call custom onStdout callback', async () => { + const customChunks: Buffer[] = []; + const output = await runOutput( + ['node', '--eval', 'console.log("test")'], + { + onStdout: data => customChunks.push(data), + }, + ); + expect(output).toBe('test'); + expect(customChunks.length).toBeGreaterThan(0); + }); + + it('should call custom onStderr callback', async () => { + const customChunks: Buffer[] = []; + await runOutput( + ['node', '--eval', 'console.error("error"); console.log("ok")'], + { + onStderr: data => customChunks.push(data), + }, + ); + expect(customChunks.length).toBeGreaterThan(0); + const errorOutput = Buffer.concat(customChunks).toString(); + expect(errorOutput).toContain('error'); + }); + }); + + describe('runCheck', () => { + it('should return true for successful command', async () => { + const result = await runCheck(['node', '--version']); + expect(result).toBe(true); + }); + + it('should return false for failed command', async () => { + const result = await runCheck(['node', '--eval', 'process.exit(1)']); + expect(result).toBe(false); + }); + + it('should return false for nonexistent command', async () => { + const result = await runCheck(['nonexistent-command-12345']); + expect(result).toBe(false); + }); + }); +}); From 6359995329b0130952b4bad64aba1f3918fe37b3 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 29 Nov 2025 23:20:46 +0100 Subject: [PATCH 09/10] cli-common: cleanup and polish waitForExit Signed-off-by: Patrik Oldsberg --- packages/cli-common/src/run.test.ts | 38 +++++++++++++-- packages/cli-common/src/run.ts | 75 +++++++++++++++++------------ 2 files changed, 78 insertions(+), 35 deletions(-) diff --git a/packages/cli-common/src/run.test.ts b/packages/cli-common/src/run.test.ts index 04df5fe05f..01c6c915d6 100644 --- a/packages/cli-common/src/run.test.ts +++ b/packages/cli-common/src/run.test.ts @@ -122,9 +122,41 @@ describe('run', () => { activeChildren.push(child); // Wait for it to complete await child.waitForExit(); - await expect( - Promise.race([child.waitForExit(), 'pending']), - ).resolves.not.toBe('pending'); + // Call waitForExit again - should return immediately + await expect(child.waitForExit()).resolves.not.toThrow(); + }); + + it('should handle multiple simultaneous calls to waitForExit', async () => { + const child = run(['node', '--version']); + activeChildren.push(child); + // Call waitForExit multiple times simultaneously + const [result1, result2, result3] = await Promise.all([ + child.waitForExit(), + child.waitForExit(), + child.waitForExit(), + ]); + // All should resolve successfully + expect(result1).toBeUndefined(); + expect(result2).toBeUndefined(); + expect(result3).toBeUndefined(); + }); + + it('should handle multiple simultaneous calls to waitForExit with error', async () => { + const child = run(['node', '--eval', 'process.exit(1)']); + activeChildren.push(child); + // Call waitForExit multiple times simultaneously + const promises = [ + child.waitForExit(), + child.waitForExit(), + child.waitForExit(), + ]; + // All should reject with the same error + for (const promise of promises) { + await expect(promise).rejects.toThrow(ExitCodeError); + await expect(promise).rejects.toThrow( + /Command 'node --eval process\.exit\(1\)' exited with code 1/, + ); + } }); it('should handle signal handlers cleanup', async () => { diff --git a/packages/cli-common/src/run.ts b/packages/cli-common/src/run.ts index b64d2bb94b..9bdfbcd1f6 100644 --- a/packages/cli-common/src/run.ts +++ b/packages/cli-common/src/run.ts @@ -99,49 +99,60 @@ export function run(args: string[], options: RunOptions = {}): RunChildProcess { const commandName = args.join(' '); - let signalHandlersRegistered = false; - const handleSignal = () => { - if (!child.killed && child.exitCode === null) { - child.kill(); - } - }; + let waitPromise: Promise | undefined; child.waitForExit = async (): Promise => { - // Register signal handlers to kill child process on SIGINT/SIGTERM - if (!signalHandlersRegistered) { - for (const signal of ['SIGINT', 'SIGTERM'] as const) { - process.on(signal, handleSignal); - } - signalHandlersRegistered = true; + if (waitPromise) { + return waitPromise; } - try { + waitPromise = new Promise((resolve, reject) => { if (typeof child.exitCode === 'number') { if (child.exitCode) { - throw new ExitCodeError(child.exitCode, commandName); + reject(new ExitCodeError(child.exitCode, commandName)); + } else { + resolve(); } return; } - await new Promise((resolve, reject) => { - child.once('error', reject); - child.once('exit', code => { - if (code) { - reject(new ExitCodeError(code, commandName)); - } else { - resolve(); - } - }); - }); - } finally { - // Clean up signal handlers when done waiting - if (signalHandlersRegistered) { - for (const signal of ['SIGINT', 'SIGTERM'] as const) { - process.removeListener(signal, handleSignal); - } - signalHandlersRegistered = false; + function onError(error: Error) { + cleanup(); + reject(error); } - } + + function onExit(code: number | null) { + cleanup(); + if (code) { + reject(new ExitCodeError(code, commandName)); + } else { + resolve(); + } + } + + function onSignal() { + if (!child.killed && child.exitCode === null) { + child.kill(); + } + } + + function cleanup() { + for (const signal of ['SIGINT', 'SIGTERM'] as const) { + process.removeListener(signal, onSignal); + } + child.removeListener('error', onError); + child.removeListener('exit', onExit); + } + + child.once('error', onError); + child.once('exit', onExit); + + for (const signal of ['SIGINT', 'SIGTERM'] as const) { + process.addListener(signal, onSignal); + } + }); + + return waitPromise; }; return child; From e750be29b5854a2f9f144eaaaa1cce25c4fc8a2d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 30 Nov 2025 10:26:53 +0100 Subject: [PATCH 10/10] cli: remove redundant util Signed-off-by: Patrik Oldsberg --- .../lib/execution/executePortableTemplate.ts | 28 +++++++++++++------ packages/cli/src/modules/new/lib/tasks.ts | 23 --------------- 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/packages/cli/src/modules/new/lib/execution/executePortableTemplate.ts b/packages/cli/src/modules/new/lib/execution/executePortableTemplate.ts index 6d34e35e4c..8e31f740cd 100644 --- a/packages/cli/src/modules/new/lib/execution/executePortableTemplate.ts +++ b/packages/cli/src/modules/new/lib/execution/executePortableTemplate.ts @@ -24,6 +24,7 @@ import { } from '../types'; import { installNewPackage } from './installNewPackage'; import { writeTemplateContents } from './writeTemplateContents'; +import { run } from '@backstage/cli-common'; type ExecuteNewTemplateOptions = { config: PortableTemplateConfig; @@ -54,14 +55,25 @@ export async function executePortableTemplate( } if (!options.skipInstall) { - await Task.forCommand('yarn install', { - cwd: targetDir, - optional: true, - }); - await Task.forCommand('yarn lint --fix', { - cwd: targetDir, - optional: true, - }); + for (const command of [ + ['yarn', 'install'], + ['yarn', 'lint', '--fix'], + ]) { + const commandStr = command.join(' '); + try { + await Task.forItem('executing', commandStr, async () => { + await run(command, { + cwd: targetDir, + stdio: 'ignore', + }).waitForExit(); + }); + } catch (error) { + assertError(error); + Task.error( + `Warning: Failed to execute command '${commandStr}', ${error}`, + ); + } + } } Task.log(); diff --git a/packages/cli/src/modules/new/lib/tasks.ts b/packages/cli/src/modules/new/lib/tasks.ts index b0003e1053..f3620d242f 100644 --- a/packages/cli/src/modules/new/lib/tasks.ts +++ b/packages/cli/src/modules/new/lib/tasks.ts @@ -16,8 +16,6 @@ import chalk from 'chalk'; import ora from 'ora'; -import { assertError } from '@backstage/errors'; -import { run } from '@backstage/cli-common'; const TASK_NAME_MAX_LENGTH = 14; @@ -61,25 +59,4 @@ export class Task { throw error; } } - - static async forCommand( - command: string, - options?: { cwd?: string; optional?: boolean }, - ) { - try { - await Task.forItem('executing', command, async () => { - const parts = command.trim().split(/\s+/); - await run(parts, { cwd: options?.cwd }).waitForExit(); - }); - } catch (error) { - assertError(error); - if (options?.optional) { - Task.error(`Warning: Failed to execute command ${chalk.cyan(command)}`); - } else { - throw new Error( - `Failed to execute command '${chalk.cyan(command)}', ${error}`, - ); - } - } - } }