Repository navigation
Conversation
davesnx
left a comment
There was a problem hiding this comment.
Independent review of dbbafcb against main (0f5ca93). Built with dune build -p query-json @install, ran @runtest @fmt (exit 0, 797 tests), exercised both binaries directly, and re-measured startup with hyperfine -N (20 runs: about 2x faster at -n 1, same direction and magnitude as the table above).
Findings (inline): three should-fix, one nit. None blocks the split itself; the sibling lookup and the install.sh -r <old-tag> regression are the two worth fixing before merge.
Checked and clean
cli/repl.mlmatches the four positional cases of the oldcli.mlarm by arm, including<inline>/<stdin>labels, error printing, exit codes, and the parse-then-reconnect TTY order.- Piped stdin reaches the REPL through the
sh -chop;Filename.quote_commandhandles inline JSON with spaces and commas. - dune: no module overlap between the two executables,
Repl_ui.mlxbuilds under the mlx dialect,cli/test/dunedepends on both binaries. nmfinds nocamlMosaic*/camlCmarkit*symbols incli.exe; size 14 MB to 8.1 MB as stated.- Pre-existing, not introduced here: the CI "Upload artifacts" step runs only on
ubuntu-latestwhile the release step later expects macOS and Windows artifacts; Windows zips drop the.exeextension. Both are byte-identical onmain.
| 3 ms of module initialization per process, which every plain query would | ||
| otherwise pay. Look next to this executable first, then on PATH. *) | ||
| let run_repl args = | ||
| let dir = Filename.dirname Sys.executable_name in |
There was a problem hiding this comment.
should-fix. Sys.executable_name resolves through symlinks (/proc/self/exe on Linux), so for any symlink-based install the sibling check looks in the wrong directory and only the PATH fallback saves it. Reproduced with dune's own install tree:
$ readlink -f _build/install/default/bin/query-json
.../_build/default/cli/cli.exe # directory holds repl.exe, not query-json-repl
$ _build/install/default/bin/query-json --repl . cli/test/mock.json </dev/null
sh: 1: query-json-repl: not found
query-json-repl was not found. Install it next to query-json or on PATH.
exit 127
The shipped zip, install.sh and the npm postinstall place real files side by side, so they are fine; stow/nix/asdf-style layouts are not. Suggest also trying Filename.dirname Sys.argv.(0) (the path as invoked) before falling back to PATH.
There was a problem hiding this comment.
Fixed in 34c0986: the shim now resolves the binary itself, trying the directory of argv.(0) (when it has one), the directory of Sys.executable_name, then each PATH entry, and only then calls Sys.command with the absolute path. Verified: dune symlinked install tree with PATH= empty launches the REPL; a stray ./query-json-repl in the current directory is not picked up when query-json is found via PATH.
| "query-json-repl" | ||
| in | ||
| let code = Sys.command (Filename.quote_command exe args) in | ||
| if code = 127 then |
There was a problem hiding this comment.
should-fix. Sys.command goes through cmd.exe on Windows, where a missing program exits with 9009, not 127, so the hint is never printed on the Windows build that CI ships. Check code = 127 || code = 9009, or branch on Sys.win32.
nit. On Unix the shell prints its own sh: 1: query-json-repl: not found before the friendly hint. Checking Sys.file_exists on the resolved candidate (or searching PATH yourself) before calling Sys.command would avoid the double message.
There was a problem hiding this comment.
Fixed in 34c0986: no shell is involved in the lookup any more, so the 127-vs-9009 difference and the extra sh: not found line are both gone. A missing binary prints only the hint and exits 127 on every platform; the extension is .exe only when Sys.win32.
| unzip -q "$DOWNLOAD_DIR/$FILENAME.zip" -d "$DOWNLOAD_DIR" | ||
| mv "$DOWNLOAD_DIR/query-json" "$INSTALL_DIR/query-json" | ||
| chmod u+x "$INSTALL_DIR/query-json" | ||
| mv "$DOWNLOAD_DIR/query-json-repl" "$INSTALL_DIR/query-json-repl" |
There was a problem hiding this comment.
should-fix. The script runs under set -e (line 3). Pinning an older release with -r <tag> downloads a single-binary zip, so this mv fails after query-json was already moved and before the chmod runs, leaving a half-installed, non-executable binary:
mv: cannot stat '.../query-json-repl': No such file or directory
Guard with [ -f "$DOWNLOAD_DIR/query-json-repl" ] && mv ..., or state in the script that -r only supports releases from this change onward.
There was a problem hiding this comment.
Fixed in 34c0986: query-json is moved and made executable first; query-json-repl is moved only if the zip contained it. Re-ran the sequence against a single-binary directory: exit 0, query-json installed and executable.
Every
query-jsoninvocation, even--version, paid about 3 ms and 8 MB of module initialisation for the REPL's terminal UI stack:mosaic.uidepends oncmarkit, whoseCmarkit_databuilds its Unicode and HTML-entity tables eagerly at startup. On the small benchmark files that made jq 1.7 and jaq three to four times faster than query-json, purely on process startup.This moves the REPL into its own executable,
query-json-repl, and keepsquery-json --replworking by launching the sibling binary (next to the executable first, then onPATH; a missing binary prints a hint and exits 127). The JSON-loading cases for file, inline and stdin input move unchanged intocli/repl.ml; the TUI module becomesRepl_ui.mlxso the entry point can own theReplmodule name. Release zips,scripts/install.shand the npm postinstall ship both binaries.Startup,
hyperfine -N --warmup 5 --min-runs 40, release builds, Linux x86-64, OCaml 5.5.0~beta1, base0f5ca93vs headdbbafcb:query-json -n 1at0f5ca93query-json -n 1atdbbafcbjq -n 1(1.7)jaq -n 1(3.1.1)Allocation at exit (
OCAMLRUNPARAM=v=0x400) drops from 1,063,863 to 20,074 words;cli.exeshrinks from 14.2 MB to 8.1 MB andnmfinds nocamlMosaic*orcamlCmarkit*symbols in it. Ahello worldlinked withcmarkitalone starts in 3.1 ms, which isolates the cost.Checks at
dbbafcb:dune build @runtest @fmt(797 runtime, 77 reader, CLI Cram incl. a new--replcase that runs through the shim) exit 0; release@allexit 0;git diff --checkclean.Review note: the two-binary release path (
.github/workflows/ci.yml,scripts/install.sh,scripts/make-npm-release.js,scripts/release-postinstall.js) only runs on a tagged build and is not exercised locally.Risk: two-way door. A merge revert restores the single-binary CLI and packaging; nothing outside the repository changes.