Skip to content

Commit

Permalink
Improve tsc dyndep creation
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
elliotgoodrich committed May 18, 2024
1 parent 572f6c3 commit 4a3ab47
Show file tree
Hide file tree
Showing 7 changed files with 214 additions and 163 deletions.
106 changes: 79 additions & 27 deletions integration/src/tsc.test.mts
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
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,
rmSync,
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";

Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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}`);
Expand All @@ -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
});
30 changes: 26 additions & 4 deletions integration/src/util.mts
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,38 @@ export function getDeps(cwd: string): Record<string, string[]> {
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<string, string[]>,
expected: Record<string, (string | RegExp)[]>,
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/tsc/package.json
Original file line number Diff line number Diff line change
@@ -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": {
Expand Down
82 changes: 82 additions & 0 deletions packages/tsc/src/parseOutput.mts
Original file line number Diff line number Diff line change
@@ -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<string> {
// 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();
Loading

0 comments on commit 4a3ab47

Please sign in to comment.