From 4a3ab473b120e8c26bd148f1d5ce59dab8ef855a Mon Sep 17 00:00:00 2001 From: Elliot Goodrich Date: Sat, 18 May 2024 13:54:02 +0100 Subject: [PATCH] Improve `tsc` dyndep creation Instead of having a `runTSC.mts` script that calls into `tsc`, have ninja call `tsc` directly (or at least have node call the `tsc` entry point) and then redirect output to `parseOutput.mts`. This was done mainly due to ease of debugging. Previously when something went wrong it was non-trivial to find out the actual command line arguments that `runTSC.mts` used to call `tsc`. After this, it is much easier to see the arguments passed to `tsc` and to copy and paste the raw command into a terminal for additional debugging. Additionally, because we `cd` into the correct directory inside `runTSC.mts` it meant that `tsc` would refuse to write files outside of the `rootDir` and we couldn't use the `$builddir` for writing temporary timestamp files. The down side of this change is that we need to write the return code into the end of the stream and then parse it in `parseOutput.mts` to return the same code. This was quite tricky on Windows but it seems to be working now. --- integration/src/tsc.test.mts | 106 ++++++++++++++++++++------- integration/src/util.mts | 30 ++++++-- package-lock.json | 2 +- packages/tsc/package.json | 2 +- packages/tsc/src/parseOutput.mts | 82 +++++++++++++++++++++ packages/tsc/src/runTSC.mts | 119 ------------------------------- packages/tsc/src/tsc.ts | 36 +++++++--- 7 files changed, 214 insertions(+), 163 deletions(-) create mode 100644 packages/tsc/src/parseOutput.mts delete mode 100644 packages/tsc/src/runTSC.mts diff --git a/integration/src/tsc.test.mts b/integration/src/tsc.test.mts index e787eba..9c4d208 100644 --- a/integration/src/tsc.test.mts +++ b/integration/src/tsc.test.mts @@ -1,7 +1,7 @@ import { beforeEach, test, describe } from "node:test"; import { strict as assert } from "node:assert"; -import { NinjaBuilder } from "@ninjutsu-build/core"; -import { makeTSCRule } from "@ninjutsu-build/tsc"; +import { NinjaBuilder, getInput, validations } from "@ninjutsu-build/core"; +import { makeTSCRule, makeTypeCheckRule } from "@ninjutsu-build/tsc"; import { writeFileSync, mkdirSync, @@ -9,9 +9,8 @@ import { symlinkSync, existsSync, } from "node:fs"; -import { execSync, spawnSync } from "node:child_process"; import { join } from "node:path/posix"; -import { getDeps } from "./util.mjs"; +import { getDeps, callNinja, callNinjaWithFailure } from "./util.mjs"; import { relative as relativeNative, sep } from "node:path"; import { fileURLToPath } from "node:url"; @@ -51,7 +50,8 @@ describe("tsc tests", () => { "export function negate(n: number): number { return -n; }\n", ); - const add = "add.cts"; + // Include a space in the filename to check we are escaping characters + const add = "add together.cts"; writeFileSync( join(dir, add), "export function add(a: number, b: number): number { return a + b; }\n", @@ -63,7 +63,9 @@ describe("tsc tests", () => { const subtract = (() => { const impDir = join(dir, "imp"); mkdirSync(impDir); - const subtract = "subtract.cts"; + + // Use `$` in the file name to check character escaping + const subtract = "$ubtract.cts"; writeFileSync( join(impDir, subtract), "export function subtract(a: number, b: number): number { return a - b; }\n", @@ -84,8 +86,8 @@ describe("tsc tests", () => { writeFileSync( join(dir, script), "import { negate } from './negate.mjs';\n" + - "import { add } from './add.cjs';\n" + - "import { subtract } from './src/subtract.cjs';\n" + + "import { add } from './add together.cjs';\n" + + "import { subtract } from './src/$ubtract.cjs';\n" + "console.log(negate(1));\n" + "console.log(add(2, 3));\n" + "console.log(subtract(4, 5));\n", @@ -96,44 +98,65 @@ describe("tsc tests", () => { const script2 = "script.cts"; writeFileSync( join(dir, script2), - "const { add } = require('./add.cjs');\n" + - "const { subtract } = require('./src/subtract.cjs');\n" + + "const { add } = require('./add together.cjs');\n" + + "const { subtract } = require('./src/$ubtract.cjs');\n" + "console.log(add('2', 3));\n" + "console.log(subtract(4, 5));\n", ); + const err1 = "err1.cts"; + writeFileSync(join(dir, err1), "const x: number = null;"); + + const err2 = "err2.cts"; + writeFileSync(join(dir, err2), "import { bad } from './add together.cjs';"); + const ninja = new NinjaBuilder({}, dir); const tsc = makeTSCRule(ninja); + const typecheck = makeTypeCheckRule(ninja); const output = tsc({ in: [script], compilerOptions }); const output2 = tsc({ in: [script2], compilerOptions }); + const [stamp] = typecheck({ + in: [script], + out: "typecheck.stamp", + compilerOptions, + }); + ninja.default(...output, ...output2, stamp[validations]); + const err1Target = ninja.phony({ + out: "err1", + in: tsc({ in: [err1], compilerOptions })[0], + }); + const err2Target = ninja.phony({ + out: "err2", + in: tsc({ in: [err2], compilerOptions })[0], + }); writeFileSync(join(dir, "build.ninja"), ninja.output); { - const { stdout, stderr, status } = spawnSync( - "ninja", - ["-d", "keepdepfile"], - { cwd: dir }, - ); - const stdoutStr = stdout.toString(); - assert.strictEqual(stderr.toString(), ""); - assert.strictEqual(status, 0, stdoutStr); - assert.match(stdoutStr, /Compiling script.mts/); - assert.match(stdoutStr, /Compiling script.cts/); + const stdout = callNinja(dir); + assert.match(stdout, /Compiling script.mts/); + assert.match(stdout, /Compiling script.cts/); + assert.match(stdout, /Typechecking script.mts/); } - for (const out of [...output, ...output2]) { + for (const out of [output, output2, getInput(stamp)].flat()) { assert.strictEqual(existsSync(join(dir, out)), true); } - assert.strictEqual( - execSync("ninja", { cwd: dir }).toString().trim(), - "ninja: no work to do.", - ); + assert.strictEqual(callNinja(dir).trimEnd(), "ninja: no work to do."); const deps = getDeps(dir); assert.deepEqual( new Set(Object.keys(deps)), - new Set([...output, ...output2]), + new Set([...output, ...output2, stamp[validations]]), ); + + { + // Check that we have the same dependencies whether we typecheck or + // generate code/types if we have the same inputs + const lhs = deps[output[0]]; + const rhs = deps[stamp[validations]]; + assert.deepEqual(lhs.sort(), rhs.sort()); + } + for (const out of output) { assert.notStrictEqual(deps[out].indexOf(negate), -1, `Missing ${negate}`); assert.notStrictEqual(deps[out].indexOf(add), -1, `Missing ${add}`); @@ -158,7 +181,36 @@ describe("tsc tests", () => { `${subtract} should be missing`, ); } - }); + { + const { stdout, stderr } = callNinjaWithFailure(dir, err1Target); + assert.deepEqual(stderr, ""); + assert(stdout.includes("Compiling err1.cts"), stdout); + assert( + stdout.includes( + "error TS2322: Type 'null' is not assignable to type 'number'", + ), + stdout, + ); + + // Check that we correctly cull most of the input + assert(stdout.split("\n").length < 10, stdout); + } + + { + const { stdout, stderr } = callNinjaWithFailure(dir, err2Target); + assert.deepEqual(stderr, ""); + assert(stdout.includes("Compiling err2.cts"), stdout); + assert( + stdout.includes( + "error TS2305: Module '\"./add together.cjs\"' has no exported member 'bad'", + ), + stdout, + ); + + // Check that we correctly cull most of the input + assert(stdout.split("\n").length < 10, stdout); + } + }); // TODO: Check the `incremental` flag works correctly }); diff --git a/integration/src/util.mts b/integration/src/util.mts index cb80c64..9f2fae0 100644 --- a/integration/src/util.mts +++ b/integration/src/util.mts @@ -23,16 +23,38 @@ export function getDeps(cwd: string): Record { return deps; } -export function callNinja(cwd: string): string { - const { stdout, stderr, status } = spawnSync("ninja", ["-d", "keepdepfile"], { - cwd, - }); +export function callNinja(cwd: string, target?: string): string { + const extra = target === undefined ? [] : [target]; + const { stdout, stderr, status } = spawnSync( + "ninja", + ["-d", "keepdepfile", ...extra], + { + cwd, + }, + ); const stdoutStr = stdout.toString(); assert.strictEqual(stderr.toString(), ""); assert.strictEqual(status, 0, stdoutStr); return stdoutStr; } +export function callNinjaWithFailure( + cwd: string, + target?: string, +): { stdout: string; stderr: string } { + const extra = target === undefined ? [] : [target]; + const { stdout, stderr, status } = spawnSync( + "ninja", + ["-d", "keepdepfile", ...extra], + { + cwd, + }, + ); + const stdoutStr = stdout.toString(); + assert.notStrictEqual(status, 0, stdout.toString()); + return { stdout: stdoutStr, stderr: stderr.toString() }; +} + export function depsMatch( actual: Record, expected: Record, diff --git a/package-lock.json b/package-lock.json index 256bde6..d9b92cd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2057,7 +2057,7 @@ }, "packages/tsc": { "name": "@ninjutsu-build/tsc", - "version": "0.12.2", + "version": "0.12.3", "license": "MIT", "dependencies": { "typescript": "^5.2.2" diff --git a/packages/tsc/package.json b/packages/tsc/package.json index 649f285..bb4e229 100644 --- a/packages/tsc/package.json +++ b/packages/tsc/package.json @@ -1,6 +1,6 @@ { "name": "@ninjutsu-build/tsc", - "version": "0.12.2", + "version": "0.12.3", "description": "Create a ninjutsu-build rule for running the TypeScript compiler (tsc)", "author": "Elliot Goodrich", "scripts": { diff --git a/packages/tsc/src/parseOutput.mts b/packages/tsc/src/parseOutput.mts new file mode 100644 index 0000000..3ba2e6b --- /dev/null +++ b/packages/tsc/src/parseOutput.mts @@ -0,0 +1,82 @@ +import { createInterface } from "node:readline"; +import { isAbsolute, relative } from "node:path"; +import { realpath } from "node:fs/promises"; +import { writeFileSync } from "node:fs"; +import { parseArgs } from "node:util"; + +const cwd = process.cwd(); + +async function convertToPath(line: string): Promise { + // Look at the canonical path to resolve symlinks and find the original + // path. This will allow us to handle dependencies across monorepos. + let path = await realpath(line); + + // Relative paths can be returned immediately + if (!isAbsolute(path)) { + return path.replaceAll(" ", "\\_").replaceAll("$", "\\$"); + } + + // Absolute paths are most likely references to things inside `node_modules`, + // but could be absolute paths given by the user to something else. If the + // path is within the current working directory, replace with the relative + // path, otherwise keep it as absolute. + { + const relativeAttempt = relative(cwd, path); + if ( + relativeAttempt && + !relativeAttempt.startsWith("..") && + !isAbsolute(relativeAttempt) + ) { + path = relativeAttempt; + } + } + return path + .replaceAll("\\", "/") + .replaceAll(" ", "\\ ") + .replaceAll("$", "\\$"); +} + +async function main() { + const { + positionals: [out], + values: { touch }, + } = parseArgs({ + allowPositionals: true, + options: { touch: { type: "boolean" } }, + }); + + const lines: string[] = []; + for await (const line of createInterface({ + input: process.stdin, + })) { + lines.push(line); + } + + // Assume the last line passes in the return code of `tsc` + const rc = Number.parseInt(lines.pop() ?? "1"); + if (rc === 0) { + const paths = await Promise.all( + lines.filter((l) => l !== "").map(convertToPath), + ); + + // The ".depfile" suffix must match what's used in `node.ts` + writeFileSync(out + ".depfile", out + ": " + paths.join(" ")); + if (touch) { + writeFileSync(out, ""); + } + } else { + // Drop the `--listFiles` content printed at the end until we get to the + // error messages. Assume that all the paths end in `ts` (e.g. `.d.ts` + // or `.d.mts`) and keep searching until we find something that doesn't. + let i = lines.length - 1; + for (; i >= 0; --i) { + if (!lines[i].endsWith("ts")) { + break; + } + } + console.log(lines.slice(0, i + 1).join("\n")); + } + process.exit(rc); +} + +await main(); diff --git a/packages/tsc/src/runTSC.mts b/packages/tsc/src/runTSC.mts deleted file mode 100644 index 2accae3..0000000 --- a/packages/tsc/src/runTSC.mts +++ /dev/null @@ -1,119 +0,0 @@ -import { exec } from "node:child_process"; -import { argv } from "node:process"; -import { writeFileSync } from "node:fs"; -import { isAbsolute, relative } from "node:path"; -import { realpathSync } from "node:fs"; -import { promisify } from "node:util"; - -function parseArgs(args: readonly string[]): { - tsc: string; - depfile?: string; - out?: string; - touch?: string; - tsArgs: readonly string[]; - input: readonly string[]; -} { - let tsc: string | undefined = undefined; - let depfile: string | undefined = undefined; - let touch: string | undefined = undefined; - let out: string | undefined = undefined; - let input: string[] = []; - let tsArgs: string[] = []; - for (let i = 2; i < argv.length; ++i) { - switch (argv[i]) { - case "--tsc": - if (++i < argv.length) { - tsc = argv[i]; - } - break; - case "--depfile": - if (++i < argv.length) { - depfile = argv[i]; - } - break; - case "--out": - if (++i < argv.length) { - out = argv[i]; - } - break; - case "--touch": - if (++i < argv.length) { - touch = argv[i]; - } - break; - default: { - const splitIndex = args.indexOf("--", i); - if (splitIndex === -1) { - throw new Error("'--' must come after flags but before input files"); - } - tsArgs = args.slice(i, splitIndex); - input = args.slice(splitIndex + 1); - i = argv.length; - } - } - } - if (tsc === undefined) { - throw new Error("--tsc must be specified"); - } - if ((depfile === undefined) !== (out === undefined)) { - throw new Error( - "Either both --depfile and --out are specified, or neither are!", - ); - } - return { tsc, depfile, out, touch, tsArgs, input }; -} - -async function run(): Promise { - try { - const { tsc, depfile, touch, out, tsArgs, input } = parseArgs(argv); - if (depfile !== undefined) { - const { stdout } = await promisify(exec)( - `node ${tsc} ${tsArgs.concat(input).join(" ")}`, - ); - const lines = stdout.split("\n").map((l) => l.trim()); - let deps = out + ":"; - const cwd = process.cwd(); - const makeRelative = (path: string) => { - if (!isAbsolute(path)) { - return path; - } - - // Absolute paths are most likely references to things inside `node_modules`, - // but could be absolute paths given by the user to something else. If the - // path is within the current working directory, replace with the relative - // path, otherwise keep it as absolute. - const relativeAttempt = relative(cwd, path); - return relativeAttempt && - !relativeAttempt.startsWith("..") && - !isAbsolute(relativeAttempt) - ? relativeAttempt - : path; - }; - for (const line of lines) { - if (line !== "") { - // Look at the canonical path to resolve symlinks and find the original - // path. This will allow us to handle dependencies acros monorepos. - deps += - " " + makeRelative(realpathSync(line)).replaceAll("\\", "/").trim(); - } - } - - writeFileSync(depfile, deps); - } - - if (touch !== undefined) { - writeFileSync(touch, ""); - } - } catch (e: unknown) { - process.exitCode = 1; - const error = e as { stdout: string }; - const lines = error.stdout.split("\n"); - // Print only error lines, attempting to filtering everything that is emitted - // using `--listFiles` - console.log( - lines.filter((line) => line.includes("): error TS")).join("\n"), - ); - } -} - -await run(); diff --git a/packages/tsc/src/tsc.ts b/packages/tsc/src/tsc.ts index acba3c4..03c4ce4 100644 --- a/packages/tsc/src/tsc.ts +++ b/packages/tsc/src/tsc.ts @@ -18,22 +18,35 @@ import ts from "typescript"; import { platform } from "os"; import { relative, resolve } from "node:path"; +// In order to pipe to $out we need to run with `cmd /c` on Windows. +const prefix = platform() === "win32" ? "cmd /c " : ""; + // Use `node.exe` on windows to avoid the `winpty node` alias and for a // small increase in performance. const node = platform() === "win32" ? "node.exe" : "node"; -function getRunTSCPath(ninja: NinjaBuilder): string { +// Use `call` + delayed expansion on windows otherwise we get the +// `errorLevel` of whatever command was run before. See this answer +// for more details https://stackoverflow.com/a/11178012 +const echoErrCode = + platform() === "win32" ? "call echo %^^errorLevel%" : "echo $?"; +const next = platform() === "win32" ? " &" : ";"; + +function getParseOutputPath(ninja: NinjaBuilder): string { + // Replacing backslashes is not necessary because we pass these paths to node, + // which understands both. It's just a bit easier on Windows to be able to + // copy and paste commands results into a non-cmd shell. return relative( resolve(process.cwd(), ninja.outputDir), - require.resolve("./runTSC.mjs"), - ); + require.resolve("./parseOutput.mjs"), + ).replaceAll("\\", "/"); } function getTSCPath(ninja: NinjaBuilder): string { return relative( resolve(process.cwd(), ninja.outputDir), require.resolve("typescript/bin/tsc"), - ); + ).replaceAll("\\", "/"); } function compilerOptionToArray( @@ -83,9 +96,6 @@ export function compilerOptionsToString( return compilerOptionsToArray(compilerOptions).join(" "); } -// In order to pipe to $out we need to run with `cmd /c` on Windows. -const prefix = platform() === "win32" ? "cmd /c " : ""; - /** * Create a rule in the specified `ninja` builder with the specified `name` that will * run `tsc` to type check all TypeScript files provided to `in`, and write an empty file @@ -135,9 +145,11 @@ export function makeTypeCheckRule( const typecheck = ninja.rule(name, { command: prefix + - `${node} ${getRunTSCPath(ninja)} --tsc ${getTSCPath( + `(${node} ${getTSCPath( ninja, - )} --touch $out --out $out --depfile $out.depfile --listFiles --noEmit $args -- $in`, + )} --listFiles --noEmit $args $in${next} ${echoErrCode}) | ${node} ${getParseOutputPath( + ninja, + )} $out --touch`, description: "Typechecking $in", in: needs[]>(), out: needs(), @@ -243,9 +255,11 @@ export function makeTSCRule( const tsc = ninja.rule(name, { command: prefix + - `${node} ${getRunTSCPath(ninja)} --tsc ${getTSCPath( + `(${node} ${getTSCPath( + ninja, + )} --listFiles $args $in${next} ${echoErrCode}) | ${node} ${getParseOutputPath( ninja, - )} --out $out --depfile $out.depfile --listFiles $args -- $in`, + )} $out`, description: "Compiling $in", depfile: "$out.depfile", deps: "gcc",