diff --git a/CHANGELOG.md b/CHANGELOG.md index 5556ddf4a6..3ece1e0846 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ - Build with OCaml 5.1.1. https://github.com/rescript-lang/rescript-compiler/pull/6641 +#### :nail_care: Polish + +- Make the `--help` arg be prioritized in the CLI, so correctly prints help message and skip other commands. https://github.com/rescript-lang/rescript-compiler/pull/6667 + # 11.1.0-rc.3 #### :nail_care: Polish diff --git a/jscomp/build_tests/cli_help/input.js b/jscomp/build_tests/cli_help/input.js index d2ebcde3c7..a92edbf4b3 100755 --- a/jscomp/build_tests/cli_help/input.js +++ b/jscomp/build_tests/cli_help/input.js @@ -1,7 +1,7 @@ // @ts-check const assert = require("assert"); -const child_process = require("child_process"); +const { exec } = require("../utils.js"); const cliHelp = "Usage: rescript \n" + @@ -72,194 +72,204 @@ const dumpHelp = "Usage: rescript dump [target]\n" + "`rescript dump` dumps the information for the target\n"; -// Shows build help with --help arg -let out = child_process.spawnSync(`../../../rescript`, ["build", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, buildHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); +async function test() { + { + // Shows build help with --help arg + const out = await exec(`../../../rescript`, ["build", "--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, buildHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// FIXME: Help works incorrectly in watch mode -out = child_process.spawnSync(`../../../rescript`, ["build", "-w", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -// FIXME: Shouldn't have "Start compiling" for help -assert.equal(out.stdout, ">>>> Start compiling\n" + buildHelp); -// FIXME: Don't run the watcher when showing help -assert.match( - out.stderr, - new RegExp( - "Uncaught Exception Error: ENOENT: no such file or directory, watch 'bsconfig.json'\n" - ) -); -// FIXME: Should be 0 -assert.equal(out.status, 1); + { + const out = await exec(`../../../rescript`, ["build", "-w", "--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, buildHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// FIXME: Has the same problem with `rescript -w` -out = child_process.spawnSync(`../../../rescript`, ["-w", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, ">>>> Start compiling\n" + buildHelp); -assert.match( - out.stderr, - new RegExp( - "Uncaught Exception Error: ENOENT: no such file or directory, watch 'bsconfig.json'\n" - ) -); + { + const out = await exec(`../../../rescript`, ["-w", "--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cliHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows cli help with --help arg even if there are invalid arguments after it -out = child_process.spawnSync(`../../../rescript`, ["--help", "-w"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, cliHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows cli help with --help arg even if there are invalid arguments after it + const out = await exec(`../../../rescript`, ["--help", "-w"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cliHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows build help with -h arg -out = child_process.spawnSync(`../../../rescript`, ["build", "-h"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, buildHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows build help with -h arg + const out = await exec(`../../../rescript`, ["build", "-h"], { + cwd: __dirname, + }); + assert.equal(out.stdout, buildHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Exits with build help with unknown arg -out = child_process.spawnSync(`../../../rescript`, ["build", "-foo"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, ""); -assert.equal(out.stderr, 'Error: Unknown option "-foo".\n' + buildHelp); -assert.equal(out.status, 2); + { + // Exits with build help with unknown arg + const out = await exec(`../../../rescript`, ["build", "-foo"], { + cwd: __dirname, + }); + assert.equal(out.stdout, ""); + assert.equal(out.stderr, 'Error: Unknown option "-foo".\n' + buildHelp); + assert.equal(out.status, 2); + } -// Shows cli help with --help arg -out = child_process.spawnSync(`../../../rescript`, ["--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, cliHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows cli help with --help arg + const out = await exec(`../../../rescript`, ["--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cliHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows cli help with -h arg -out = child_process.spawnSync(`../../../rescript`, ["-h"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, cliHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows cli help with -h arg + const out = await exec(`../../../rescript`, ["-h"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cliHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows cli help with help command -out = child_process.spawnSync(`../../../rescript`, ["help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, cliHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows cli help with -h arg + const out = await exec(`../../../rescript`, ["help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cliHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Exits with cli help with unknown command -out = child_process.spawnSync(`../../../rescript`, ["built"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, ""); -assert.equal(out.stderr, `Error: Unknown command "built".\n` + cliHelp); -assert.equal(out.status, 2); + { + // Exits with cli help with unknown command + const out = await exec(`../../../rescript`, ["built"], { + cwd: __dirname, + }); + assert.equal(out.stdout, ""); + assert.equal(out.stderr, `Error: Unknown command "built".\n` + cliHelp); + assert.equal(out.status, 2); + } -// Exits with build help with unknown args -out = child_process.spawnSync(`../../../rescript`, ["-foo"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, ""); -assert.equal(out.stderr, 'Error: Unknown option "-foo".\n' + buildHelp); -assert.equal(out.status, 2); + { + // Exits with build help with unknown args + const out = await exec(`../../../rescript`, ["-foo"], { + cwd: __dirname, + }); + assert.equal(out.stdout, ""); + assert.equal(out.stderr, 'Error: Unknown option "-foo".\n' + buildHelp); + assert.equal(out.status, 2); + } -// Shows clean help with --help arg -out = child_process.spawnSync(`../../../rescript`, ["clean", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, cleanHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows clean help with --help arg + const out = await exec(`../../../rescript`, ["clean", "--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cleanHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows clean help with -h arg -out = child_process.spawnSync(`../../../rescript`, ["clean", "-h"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, cleanHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows clean help with -h arg + const out = await exec(`../../../rescript`, ["clean", "-h"], { + cwd: __dirname, + }); + assert.equal(out.stdout, cleanHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Exits with clean help with unknown arg -out = child_process.spawnSync(`../../../rescript`, ["clean", "-foo"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, ""); -assert.equal(out.stderr, 'Error: Unknown option "-foo".\n' + cleanHelp); -assert.equal(out.status, 2); + { + // Exits with clean help with unknown arg + const out = await exec(`../../../rescript`, ["clean", "-foo"], { + cwd: __dirname, + }); + assert.equal(out.stdout, ""); + assert.equal(out.stderr, 'Error: Unknown option "-foo".\n' + cleanHelp); + assert.equal(out.status, 2); + } -// Shows format help with --help arg -out = child_process.spawnSync(`../../../rescript`, ["format", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, formatHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows format help with --help arg + const out = await exec(`../../../rescript format`, ["--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, formatHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows format help with -h arg -out = child_process.spawnSync(`../../../rescript`, ["format", "-h"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, formatHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows format help with -h arg + const out = await exec(`../../../rescript format`, ["-h"], { + cwd: __dirname, + }); + assert.equal(out.stdout, formatHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows convert help with --help arg -out = child_process.spawnSync(`../../../rescript`, ["convert", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, convertHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows convert help with --help arg + const out = await exec(`../../../rescript convert`, ["--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, convertHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows convert help with -h arg -out = child_process.spawnSync(`../../../rescript`, ["convert", "-h"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, convertHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows convert help with -h arg + const out = await exec(`../../../rescript convert`, ["-h"], { + cwd: __dirname, + }); + assert.equal(out.stdout, convertHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows dump help with --help arg -out = child_process.spawnSync(`../../../rescript`, ["dump", "--help"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, dumpHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows dump help with --help arg + const out = await exec(`../../../rescript dump`, ["--help"], { + cwd: __dirname, + }); + assert.equal(out.stdout, dumpHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } -// Shows dump help with -h arg -out = child_process.spawnSync(`../../../rescript`, ["dump", "-h"], { - encoding: "utf8", - cwd: __dirname, -}); -assert.equal(out.stdout, dumpHelp); -assert.equal(out.stderr, ""); -assert.equal(out.status, 0); + { + // Shows dump help with -h arg + const out = await exec(`../../../rescript dump`, ["-h"], { + cwd: __dirname, + }); + assert.equal(out.stdout, dumpHelp); + assert.equal(out.stderr, ""); + assert.equal(out.status, 0); + } +} + +void test(); diff --git a/jscomp/build_tests/utils.js b/jscomp/build_tests/utils.js new file mode 100644 index 0000000000..0b18871321 --- /dev/null +++ b/jscomp/build_tests/utils.js @@ -0,0 +1,52 @@ +const child_process = require("child_process"); + +const signals = { + SIGINT: 2, + SIGQUIT: 3, + SIGKILL: 9, + SIGTERM: 15, +}; + +/** + * @param {string} command + * @param {Array} args + * @param {child_process.SpawnOptions} [options] + */ +async function exec(command, args, options) { + const stdoutChunks = []; + const stderrChunks = []; + + const subprocess = child_process.spawn(command, args, { + stdio: ["ignore", "pipe", "pipe"], + ...options, + }); + + subprocess.stdout.on("data", chunk => { + stdoutChunks.push(chunk); + }); + + subprocess.stderr.on("data", chunk => { + stderrChunks.push(chunk); + }); + + return await new Promise((resolve, reject) => { + subprocess.once("error", err => { + reject(err); + }); + + subprocess.once("close", (exitCode, signal) => { + const stdout = Buffer.concat(stdoutChunks).toString("utf8"); + const stderr = Buffer.concat(stderrChunks).toString("utf8"); + + let code = exitCode ?? 1; + if (signals[signal]) { + // + 128 is standard POSIX practice, see also https://nodejs.org/api/process.html#exit-codes + code = signals[signal] + 128; + } + + resolve({ code, stdout, stderr }); + }); + }); +} + +exports.exec = exec; diff --git a/rescript b/rescript index a0ba2963a1..20c3557ff5 100755 --- a/rescript +++ b/rescript @@ -67,66 +67,69 @@ process.on("SIGUSR2", exitProcess); process.on("SIGTERM", exitProcess); process.on("SIGHUP", exitProcess); -const process_argv = process.argv; -const maybeSubcommand = process_argv[2]; +const args = process.argv.slice(2); +const argPatterns = { + help: ['help', '-h', '-help', '--help'], + version: ['version', '-v', '-version', '--version'], +}; -if (!maybeSubcommand) { - bsb.build([]); -} else { - switch (maybeSubcommand) { +const helpArgIndex = args.findIndex(arg => argPatterns.help.includes(arg)); +const firstPositionalArgIndex = args.findIndex(arg => !arg.startsWith("-")); + +if (helpArgIndex !== -1 && (firstPositionalArgIndex === -1 || helpArgIndex <= firstPositionalArgIndex)) { + console.log(helpMessage); + +} else if (argPatterns.version.includes(args[0])) { + console.log(require("./package.json").version); + +} else if (firstPositionalArgIndex !== -1) { + const subcmd = args[firstPositionalArgIndex]; + const subcmdArgs = args.slice(firstPositionalArgIndex + 1); + + switch (subcmd) { case "info": { - bsb.info(process_argv.slice(3)); + bsb.info(subcmdArgs); break; } case "clean": { - bsb.clean(process_argv.slice(3)); + bsb.clean(subcmdArgs); break; } case "build": { - bsb.build(process_argv.slice(3)); + bsb.build(subcmdArgs); break; } - case "format": + case "format": { require("./scripts/rescript_format.js").main( - process.argv.slice(3), + subcmdArgs, rescript_exe, bsc_exe ); break; - case "dump": + } + case "dump": { require("./scripts/rescript_dump.js").main( - process.argv.slice(3), + subcmdArgs, rescript_exe, bsc_exe ); break; - case "convert": + } + case "convert": { require("./scripts/rescript_convert.js").main( - process.argv.slice(3), + subcmdArgs, rescript_exe, bsc_exe ); break; - case "-h": - case "-help": - case "--help": - case "help": - console.log(helpMessage); - break; - case "-v": - case "-version": - case "--version": - case "version": - console.log(require("./package.json").version); - break; - default: - if (maybeSubcommand.startsWith("-")) { - bsb.build(process_argv.slice(2)); - } else { - console.error( - `Error: Unknown command "${maybeSubcommand}".\n${helpMessage}` - ); - process.exit(2); - } + } + default: { + console.error( + `Error: Unknown command "${subcmd}".\n${helpMessage}` + ); + process.exit(2); + } } +} else { + bsb.build(args); } diff --git a/scripts/ciTest.js b/scripts/ciTest.js index 9716c139b6..4c55996805 100644 --- a/scripts/ciTest.js +++ b/scripts/ciTest.js @@ -5,6 +5,8 @@ var fs = require("fs"); var duneBinDir = require("./dune").duneBinDir; +const { exec } = require("../jscomp/build_tests/utils.js"); + var ounitTest = false; var mochaTest = false; var bsbTest = false; @@ -37,7 +39,7 @@ if (all) { formatTest = true; } -function runTests() { +async function runTests() { if (ounitTest) { cp.execSync(path.join(duneBinDir, "ounit_tests"), { stdio: [0, 1, 2], @@ -56,7 +58,7 @@ function runTests() { console.log("Doing build_tests"); var buildTestDir = path.join(__dirname, "..", "jscomp", "build_tests"); var files = fs.readdirSync(buildTestDir); - files.forEach(function (file) { + for (const file of files) { var testDir = path.join(buildTestDir, file); if (file === "node_modules" || !fs.lstatSync(testDir).isDirectory()) { return; @@ -65,23 +67,18 @@ function runTests() { console.warn(`input.js does not exist in ${testDir}`); } else { console.log(`testing ${file}`); + // note existsSync test already ensure that it is a directory - cp.exec( - `node input.js`, - { cwd: testDir, encoding: "utf8" }, - function (error, stdout, stderr) { - console.log(stdout); + const out = await exec(`node`, ["input.js"], { cwd: testDir }); + console.log(out.stdout); - if (error !== null) { - console.log(`❌ error in ${file} with stderr:\n`, stderr); - throw error; - } else { - console.log("✅ success in", file); - } - } - ); + if (out.code === 0) { + console.log("✅ success in", file); + } else { + console.log(`❌ error in ${file} with stderr:\n`, out.stderr); + } } - }); + } } if (formatTest) { @@ -92,12 +89,4 @@ function runTests() { } } -function main() { - try { - runTests(); - } catch (err) { - console.error(err); - process.exit(2); - } -} -main(); +runTests(); diff --git a/scripts/rescript_bsb.js b/scripts/rescript_bsb.js index a64415ffe4..7b79cb1ce4 100644 --- a/scripts/rescript_bsb.js +++ b/scripts/rescript_bsb.js @@ -6,6 +6,9 @@ var os = require("os"); const child_process = require("child_process"); const rescript_exe = require("./bin_path").rescript_exe; +const cwd = process.cwd(); +const lockFileName = path.join(cwd, ".bsb.lock"); + /** * @typedef {Object} ProjectFiles * @property {Array} dirs @@ -18,35 +21,47 @@ const rescript_exe = require("./bin_path").rescript_exe; * @property {fs.FSWatcher} watcher */ -const cwd = process.cwd(); -const lockFileName = path.join(cwd, ".bsb.lock"); - -let isBuilding = false; +/** + * @type {child_process.ChildProcess | null} + */ +let ownerProcess = null; function releaseBuild() { - if (isBuilding) { + if (ownerProcess) { + ownerProcess.kill("SIGHUP"); try { - fs.unlinkSync(lockFileName); - } catch (err) {} - isBuilding = false; + fs.rmSync(lockFileName); + } catch {} + ownerProcess = null; } } -// We use [~perm:0o664] rather than our usual default perms, [0o666], because -// lock files shouldn't rely on the umask to disallow tampering by other. -function acquireBuild() { - if (isBuilding) { - return false; +/** + * We use [~perm:0o664] rather than our usual default perms, [0o666], because + * lock files shouldn't rely on the umask to disallow tampering by other. + * + * @param {Array} args + * @param {child_process.SpawnOptions} [options] + */ +function acquireBuild(args, options) { + if (ownerProcess) { + return null; } else { try { - const fid = fs.openSync(lockFileName, "wx", 0o664); - fs.closeSync(fid); - isBuilding = true; + ownerProcess = child_process.spawn(rescript_exe, args, { + stdio: "inherit", + ...options, + }); + fs.writeFileSync(lockFileName, ownerProcess.pid.toString(), { + encoding: "utf8", + flag: "wx", + mode: 0o664, + }); } catch (err) { if (err.code === "EEXIST") { console.warn(lockFileName, "already exists, try later"); } else console.log(err); } - return isBuilding; + return ownerProcess; } } @@ -55,26 +70,17 @@ function acquireBuild() { * @param {(code: number) => void} [maybeOnClose] */ function delegate(args, maybeOnClose) { - /** - * @type {child_process.ChildProcess} - */ let p; - if (acquireBuild()) { - try { - p = child_process.spawn(rescript_exe, args, { - stdio: "inherit", - }); - } catch (e) { - if (e.code === "ENOENT") { - // when bsb is actually not found - console.error(String(e)); - } + if ((p = acquireBuild(args))) { + p.once("error", e => { + console.error(String(e)); releaseBuild(); process.exit(2); - } + }); + // The 'close' event will always emit after 'exit' was already emitted, or // 'error' if the child failed to spawn. - p.on("close", code => { + p.once("close", code => { releaseBuild(); const exitCode = code === null ? 1 : code; if (maybeOnClose) { @@ -379,17 +385,17 @@ Please pick a different one using the \`-ws [host:]port\` flag from bsb.`); } else { dlog(`Rebuilding since ${reasonsToRebuild}`); } - if (acquireBuild()) { + let p; + if ( + (p = acquireBuild(rescriptWatchBuildArgs, { + stdio: ["inherit", "inherit", "pipe"], + })) + ) { logStartCompiling(); - child_process - .spawn(rescript_exe, rescriptWatchBuildArgs, { - stdio: ["inherit", "inherit", "pipe"], - }) - // @ts-ignore - .on("data", function (s) { - outputError(s, "ninja: error"); - }) - .on("exit", buildFinishedCallback) + p.on("data", function (s) { + outputError(s, "ninja: error"); + }) + .once("exit", buildFinishedCallback) .stderr.setEncoding("utf8"); // This is important to clean up all // previous queued events @@ -493,6 +499,10 @@ function build(args) { }); return; } + if (args.some(arg => ["help", "-h", "-help", "--help"].includes(arg))) { + delegate(["build", "-h"]); + return; + } if (args.includes("-w")) { watch(args); return;