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 <noreply@anthropic.com>

packages/host/src/export.ts

@@ -97,6 +97,7 @@ async function loadZip(server: FactorioServer, modVersions: Map<string, string>,
 
 /** 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;

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;
 		}
 

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],
 			]));

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")
 				);
 			});