diff --git a/action.yml b/action.yml index 0d120eb..2caa965 100644 --- a/action.yml +++ b/action.yml @@ -29,7 +29,7 @@ inputs: required: false default: "error" debug: - description: "Enable debugging when there are issues with action. Minor performance penalty." + description: "Enable debugging when there are issues with action. Also uploads the action logs and manifests as a workflow artifact, which is otherwise done only when the action fails. Minor performance penalty." required: false default: "false" @@ -37,9 +37,15 @@ outputs: cache-hit: description: "A boolean value to indicate a cache was found for the packages requested." package-version-list: - description: "The main requested packages and versions that are installed. Represented as a sorted comma delimited list with equals delimit on the package version (i.e. =,=,...). Packages installed for a non-native architecture include it (i.e. :=)." + description: > + The main requested packages and versions that are installed. + Represented as a sorted comma delimited list with equals delimit on the package version (i.e. =,=,...). + Packages installed for a non-native architecture include it (i.e. :=). all-package-version-list: - description: "All the pulled in packages and versions, including dependencies, that are installed. Represented as a sorted comma delimited list with equals delimit on the package version (i.e. =,=,...). Packages installed for a non-native architecture include it (i.e. :=)." + description: > + All the pulled in packages and versions, including dependencies, that are installed. + Represented as a sorted comma delimited list with equals delimit on the package version (i.e. =,=,...). + Packages installed for a non-native architecture include it (i.e. :=). runs: using: "node24" diff --git a/src/action.ts b/src/action.ts index 3f08c8c..13636a0 100644 --- a/src/action.ts +++ b/src/action.ts @@ -95,8 +95,9 @@ export class ActionRunner { const cacheHit = manifest !== undefined; // Needed on a cache hit too, to resolve the native arch for outputs. + // apt-fast only speeds up installs, so skip probing for it on a hit. const installManager = await createPackageManager( - true, + !cacheHit, this.logger, this.logger, ); diff --git a/src/cache.ts b/src/cache.ts index b36285a..91859c4 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -31,6 +31,15 @@ export function resolveCacheDir( */ const NO_TIMEOUT_MS = -1; +/** + * Shell script that extracts each archive given as an argument onto `/`. + * + * Paths are passed as positional arguments, not interpolated, so they are + * never parsed by the shell. + */ +const EXTRACT_ARCHIVES_SCRIPT = + 'for archive do tar --extract --file="$archive" --directory=/ --keep-directory-symlink || exit 1; done'; + /** Maintainer scripts run on restore when install scripts are enabled. */ type InstallScriptType = "preinst" | "postinst"; @@ -258,12 +267,15 @@ export class Cache { } } - for (const { packageName, archivePath } of archivePaths) { - await this.extractArchive(archivePath); - - if (executeInstallScripts) { + if (executeInstallScripts) { + // Each package's scripts must run before the next package is extracted. + for (const { packageName, archivePath } of archivePaths) { + await this.extractArchives([archivePath]); await this.runInstallScripts(packageName); } + } else { + // One process for all archives avoids a sudo and tar spawn per package. + await this.extractArchives(archivePaths.map((item) => item.archivePath)); } // Return every installed package so outputs can report dependencies too. @@ -271,23 +283,23 @@ export class Cache { } /** - * Extracts a package archive onto the root filesystem. + * Extracts package archives, in order, onto the root filesystem. * * Runs system tar under sudo because package files live in root-owned - * directories. Fails on any entry error instead of skipping it. + * directories. All archives share one sudo shell so the process cost is paid + * once. Stops at the first archive with any entry error. * - * @param archivePath Absolute path to the package archive. + * @param archivePaths Absolute paths to the package archives. */ - private async extractArchive(archivePath: string): Promise { + private async extractArchives( + archivePaths: readonly string[], + ): Promise { + if (archivePaths.length === 0) { + return; + } await this.commandRunner.run( "sudo", - [ - "tar", - "--extract", - `--file=${archivePath}`, - "--directory=/", - "--keep-directory-symlink", - ], + ["sh", "-c", EXTRACT_ARCHIVES_SCRIPT, "sh", ...archivePaths], { timeoutMs: NO_TIMEOUT_MS }, ); } diff --git a/src/index.ts b/src/index.ts index 8a63177..fd4f6fe 100644 --- a/src/index.ts +++ b/src/index.ts @@ -75,7 +75,9 @@ function getInputs(): ActionInputs { async function main(): Promise { const cacheDir = CACHE_DEFAULT_DIRNAME; const inputs = getInputs(); - const instruments = new Instruments(cacheDir, inputs.debug || core.isDebug()); + const debug = inputs.debug || core.isDebug(); + const instruments = new Instruments(cacheDir, debug); + let failed = false; try { ensureBundledScriptsExecutable(); const commandRunner = new DefaultCommandRunner( @@ -96,12 +98,16 @@ async function main(): Promise { } catch (error) { const message = error instanceof Error ? error.message : String(error); core.setFailed(message); + failed = true; } finally { - // Logs are diagnostics, so a failed upload shouldn't fail the action. - await instruments.artifacts.upload().catch((error: unknown) => { - const message = error instanceof Error ? error.message : String(error); - core.warning(`Failed to upload diagnostic artifacts: ${message}`); - }); + // The upload takes over a second, so only pay for it when the logs are + // wanted. Logs are diagnostics, so a failed upload shouldn't fail the action. + if (debug || failed) { + await instruments.artifacts.upload().catch((error: unknown) => { + const message = error instanceof Error ? error.message : String(error); + core.warning(`Failed to upload diagnostic artifacts: ${message}`); + }); + } } } diff --git a/test/action-runner.test.ts b/test/action-runner.test.ts index ee1d0ef..9df6a17 100644 --- a/test/action-runner.test.ts +++ b/test/action-runner.test.ts @@ -63,6 +63,28 @@ describe("ActionRunner", () => { expect(cache.archiveAndSave).not.toHaveBeenCalled(); }); + it("runAction cache hit creates package manager without apt-fast", async () => { + const { runner, cache } = createRunner(); + const packageNames = ActionPackageNames.fromInput("curl"); + const cacheKey = new CacheKey("v1", "0", process.arch, packageNames); + cache.loadAndRestore.mockResolvedValue( + new Manifest(new Date(), [], cacheKey), + ); + vi.mocked(createPackageManager).mockResolvedValue({ + getNativeArchitecture: vi.fn().mockResolvedValue(NATIVE_ARCH), + } as never); + + await runner.runAction({ + packages: "curl", + version: "v1", + executeInstallScripts: false, + emptyPackagesBehavior: "error", + debug: false, + }); + + expect(vi.mocked(createPackageManager).mock.calls[0]?.[0]).toBe(false); + }); + it("runAction cache miss archives installed packages with their files", async () => { // Unlikely to have dpkg install scripts on the test host. const installedPackage = createPackageName("test-pkg-not-real", "1.0.0"); diff --git a/test/cache-restore.test.ts b/test/cache-restore.test.ts index 991f524..1d988ef 100644 --- a/test/cache-restore.test.ts +++ b/test/cache-restore.test.ts @@ -87,6 +87,8 @@ async function writeCache(archivedPackages = INSTALL_ORDER): Promise { interface TarCalls { created: [archive: string, files: string[]][]; extracted: string[]; + /** Number of commands run that extracted archives. */ + extractCommands?: number; } const argValue = (args: string[], flag: string) => @@ -100,8 +102,10 @@ function recordTar(calls: TarCalls) { if (args.includes("--create")) { calls.created.push([archive, String(options.input ?? "").split("\0")]); } - if (args.includes("--extract")) { - calls.extracted.push(archive); + // Extraction runs as `sh -c