From f8aafe9265e4240b5e5bf2b365ae5991407ee67d Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 1 Nov 2026 22:44:43 +0000 Subject: [PATCH] Address review on findVersion and the export graphics lookup findVersion takes headless as a plain optional argument, the helper is checkInstallType, the warning is checkMissingBuiltinIcons and the full install lookup is inlined into exportData. Co-Authored-By: Claude Opus 5.5 --- packages/host/src/export.ts | 35 ++++++++++++---------------------- packages/host/src/server.ts | 19 +++++++++---------- test/host/export.js | 38 +++++-------------------------------- test/host/server.js | 15 ++++++++++----- 4 files changed, 36 insertions(+), 71 deletions(-) diff --git a/packages/host/src/export.ts b/packages/host/src/export.ts index 8be4843a..e77ff46e 100644 --- a/packages/host/src/export.ts +++ b/packages/host/src/export.ts @@ -97,6 +97,7 @@ async function loadZip(server: FactorioServer, modVersions: Map, /** Where icon files are read from during an export. */ interface ExportFiles { + /** Mod zips opened during the export, closed when it ends. */ zipCache: ZipCache; /** Data dir of a full install to fall back to for builtin mod files. */ graphicsDataDir?: string; @@ -525,23 +526,6 @@ async function exportLocale( return mergedLocales; } -/** - * Find a full install to read builtin mod graphics from - * - * @param server - The server the export runs on. - * @param files - Updated with the data dir found, or why there is none. - * @internal - */ -async function findGraphicsDataDir(server: FactorioServer, files: ExportFiles) { - const majorMinor = server.version!.replace(/\.\d+$/, "") as lib.MajorMinorVersion; - try { - [files.graphicsDataDir] = await findVersion(server._factorioDir, majorMinor, { headless: false }); - } catch (err: any) { - // Without it the export still has everything but the base game icons - files.graphicsError = err.message; - } -} - /** * Export locale, icons and metadata for the given factorio server * @@ -555,7 +539,13 @@ async function findGraphicsDataDir(server: FactorioServer, files: ExportFiles) { export async function exportData(server: FactorioServer) { const files: ExportFiles = { zipCache: new Map() }; if (await isHeadless(server.dataPath())) { - await findGraphicsDataDir(server, files); + const majorMinor = server.version!.replace(/\.\d+$/, "") as lib.MajorMinorVersion; + try { + [files.graphicsDataDir] = await findVersion(server._factorioDir, majorMinor, false); + } catch (err: any) { + // Without it the export still has everything but the base game icons + files.graphicsError = err.message; + } } try { return await buildExport(server, files); @@ -566,14 +556,14 @@ export async function exportData(server: FactorioServer) { } /** - * Warn about builtin mod icon files that could not be found + * Warn if any builtin mod icon files could not be found * * @param server - The server the export ran on. * @param files - Where icon files were read from. * @param iconCache - Icons loaded by the export, null for missing files. * @internal */ -function warnMissingBuiltinIcons(server: FactorioServer, files: ExportFiles, iconCache: IconCache) { +function checkMissingBuiltinIcons(server: FactorioServer, files: ExportFiles, iconCache: IconCache) { const builtinModNames = ["core", ...lib.ModPack.getBuiltinModNames(server.version!)]; let missing = 0; for (const [iconPath, icon] of iconCache) { @@ -703,7 +693,7 @@ async function buildExport(server: FactorioServer, files: ExportFiles) { const count = await exportIcons(server, modVersions, files, prototypesWithIcons, baseName, state); server._logger.info(`Exported ${count} ${baseName} icons`); } - warnMissingBuiltinIcons(server, files, state.iconCache); + checkMissingBuiltinIcons(server, files, state.iconCache); // Crop sheet to actual used height and write single spritesheet + metadata. const usedRows = Math.ceil(state.pos / (SHEET_WIDTH / SHEET_ICON_SIZE)); @@ -722,7 +712,6 @@ async function buildExport(server: FactorioServer, files: ExportFiles) { export const _createSheetState = createSheetState; export const _exportIcons = exportIcons; export const _exportLocale = exportLocale; -export const _findGraphicsDataDir = findGraphicsDataDir; export const _loadSimpleIcon = loadSimpleIcon; export const _loadLayeredIcon = loadLayeredIcon; -export const _warnMissingBuiltinIcons = warnMissingBuiltinIcons; +export const _checkMissingBuiltinIcons = checkMissingBuiltinIcons; diff --git a/packages/host/src/server.ts b/packages/host/src/server.ts index a3abe32b..1e1526a5 100644 --- a/packages/host/src/server.ts +++ b/packages/host/src/server.ts @@ -262,8 +262,7 @@ export async function checkForUpdates( * @param targetVersion - * Version to look for, supports the special value "latest" for the * latest version available. - * @param options - Optional parameters. - * @param options.headless - + * @param headless - * Only consider headless installs if true, or only full installs if * false. * @returns Array with path to data dir and version found. @@ -271,23 +270,23 @@ export async function checkForUpdates( export async function findVersion( factorioDir: string, targetVersion: lib.TargetVersion, - options: { headless?: boolean } = {}, + headless?: boolean, ): Promise<[string, lib.FullVersion]> { let kind = ""; - if (options.headless !== undefined) { - kind = options.headless ? "headless " : "full "; + if (headless !== undefined) { + kind = headless ? "headless " : "full "; } - const wrongKind = async (dir: string) => ( - options.headless !== undefined && await isHeadless(path.join(dir, "data")) !== options.headless + const checkInstallType = async (dir: string) => ( + headless === undefined || await isHeadless(path.join(dir, "data")) === headless ); // Check if this is a direct installation dir const directVersion = await getFactorioVersion(factorioDir); if (directVersion !== null) { - if (await wrongKind(factorioDir)) { + if (!await checkInstallType(factorioDir)) { throw new Error( `Unable to find a ${kind}install of Factorio ${targetVersion}: ${factorioDir} is a direct ` + - `(single-version) install of the ${options.headless ? "full" : "headless"} build.` + `(single-version) install of the ${headless ? "full" : "headless"} build.` ); } if (targetVersion === "latest" || directVersion === targetVersion || directVersion.startsWith(targetVersion)) { @@ -306,7 +305,7 @@ export async function findVersion( // Avoid any fancy directory detection in order to follow symlinks const fullPath = path.join(factorioDir, name); const version = await getFactorioVersion(fullPath); - if (version === null || await wrongKind(fullPath)) { + if (version === null || !await checkInstallType(fullPath)) { continue; } diff --git a/test/host/export.js b/test/host/export.js index 0d7a2806..b5f12579 100644 --- a/test/host/export.js +++ b/test/host/export.js @@ -3,8 +3,7 @@ import path from "node:path"; import { FactorioServer } from "@clusterio/host/dist/node/src/server.js"; import { - _createSheetState, _exportIcons, _exportLocale, _findGraphicsDataDir, _loadLayeredIcon, _loadSimpleIcon, - _warnMissingBuiltinIcons, + _checkMissingBuiltinIcons, _createSheetState, _exportIcons, _exportLocale, _loadLayeredIcon, _loadSimpleIcon, } from "@clusterio/host/dist/node/src/export.js"; @@ -122,36 +121,9 @@ describe("host/src/export", function() { assert.equal(icon, null); }); }); - describe("findGraphicsDataDir()", function() { - const installs = path.join("test", "file", "factorio"); - const withInstalls = (factorioDir, version = testServer.version) => Object.create(testServer, { - _factorioDir: { value: factorioDir }, version: { value: version }, - }); - it("finds a full install matching the major and minor version of the server", async function() { - const files = { zipCache: new Map() }; - await _findGraphicsDataDir(withInstalls(path.join("test", "file", "factorio"), "0.1.1"), files); - assert.deepEqual(files, { zipCache: new Map(), graphicsDataDir: path.join(installs, "0.1.2", "data") }); - }); - it("skips headless installs of the same version", async function() { - const files = { zipCache: new Map() }; - await _findGraphicsDataDir(withInstalls(path.join("test", "file", "factorio-mixed")), files); - assert.equal(files.graphicsDataDir, graphicsDataDir); - }); - it("records why when there is no full install", async function() { - const files = { zipCache: new Map() }; - await _findGraphicsDataDir(withInstalls(installs, "0.2.5"), files); - assert.equal(files.graphicsDataDir, undefined); - assert.equal(files.graphicsError, "Unable to find full Factorio version 0.2"); - }); - it("records why when the directory does not exist", async function() { - const files = { zipCache: new Map() }; - await _findGraphicsDataDir(withInstalls(path.join("test", "file", "missing")), files); - assert.match(files.graphicsError, /ENOENT/); - }); - }); - describe("warnMissingBuiltinIcons()", function() { + describe("checkMissingBuiltinIcons()", function() { it("warns once with the number of missing builtin files", function() { - _warnMissingBuiltinIcons(testServer, { zipCache: new Map() }, new Map([ + _checkMissingBuiltinIcons(testServer, { zipCache: new Map() }, new Map([ ["__base__/graphics/icons/a.png", null], ["__core__/graphics/icons/b.png", null], ["__base__/graphics/icons/c.png", {}], @@ -162,7 +134,7 @@ describe("host/src/export", function() { assert.doesNotMatch(warnings[0], /headless/); }); it("explains the headless build when no full install was found", function() { - _warnMissingBuiltinIcons(testServer, { zipCache: new Map(), graphicsError: "Not found" }, new Map([ + _checkMissingBuiltinIcons(testServer, { zipCache: new Map(), graphicsError: "Not found" }, new Map([ ["__base__/graphics/icons/a.png", null], ])); assert.equal(warnings.length, 1); @@ -170,7 +142,7 @@ describe("host/src/export", function() { assert.match(warnings[0], /Add a full install of Factorio 0\.1 to host\.factorio_directory/); }); it("does not warn when no builtin files are missing", function() { - _warnMissingBuiltinIcons(testServer, { zipCache: new Map() }, new Map([ + _checkMissingBuiltinIcons(testServer, { zipCache: new Map() }, new Map([ ["__base__/graphics/icons/c.png", {}], ["__some_mod__/graphics/icons/d.png", null], ])); diff --git a/test/host/server.js b/test/host/server.js index a1cc1c33..043aae0c 100644 --- a/test/host/server.js +++ b/test/host/server.js @@ -105,20 +105,25 @@ describe("host/server", function() { ); }); }); - describe("headless option", function() { + describe("headless argument", function() { const mixedDir = path.join("test", "file", "factorio-mixed"); it("should find only full installs when false", async function() { - const [dir] = await hostServer.findVersion(mixedDir, "0.1.2", { headless: false }); + const [dir] = await hostServer.findVersion(mixedDir, "0.1.2", false); assert.equal(dir, path.join(mixedDir, "full", "data")); }); it("should find only headless installs when true", async function() { - const [dir] = await hostServer.findVersion(mixedDir, "0.1.2", { headless: true }); + const [dir] = await hostServer.findVersion(mixedDir, "0.1.2", true); assert.equal(dir, path.join(mixedDir, "headless", "data")); }); + it("should skip installs of the other build when matching a partial version", async function() { + const installDir = path.join("test", "file", "factorio"); + const [dir, version] = await hostServer.findVersion(installDir, "0.1", false); + assert.deepEqual([dir, version], [path.join(installDir, "0.1.2", "data"), "0.1.2"]); + }); it("should reject a direct install of the other build", async function() { const installDir = path.join(mixedDir, "headless"); await assert.rejects( - hostServer.findVersion(installDir, "0.1.2", { headless: false }), + hostServer.findVersion(installDir, "0.1.2", false), new Error( `Unable to find a full install of Factorio 0.1.2: ${installDir} is a direct ` + "(single-version) install of the headless build." @@ -127,7 +132,7 @@ describe("host/server", function() { }); it("should reject if no install of that build has the version", async function() { await assert.rejects( - hostServer.findVersion(path.join("test", "file", "factorio"), "0.1.2", { headless: true }), + hostServer.findVersion(path.join("test", "file", "factorio"), "0.1.2", true), new Error("Unable to find headless Factorio version 0.1.2") ); });