diff --git a/changes/cli-command-first.breaking.md b/changes/cli-command-first.breaking.md new file mode 100644 index 0000000000..c98a4d96e6 --- /dev/null +++ b/changes/cli-command-first.breaking.md @@ -0,0 +1 @@ +- `rescript` needs the command before its options and folder, as in `rescript build -v ./pkg`, and reports an unknown command even when `--help` or `--version` follows it. diff --git a/rewatch-ocaml/cli.ml b/rewatch-ocaml/cli.ml index 2f7733a0d6..e5cbe56183 100644 --- a/rewatch-ocaml/cli.ml +++ b/rewatch-ocaml/cli.ml @@ -287,8 +287,25 @@ let root = ~version:("rescript " ^ Rewatch_version.version) ~doc:"Fast, Simple, Fully Typed JavaScript from the Future" ~man in - Cmd.group info ~default:(build_term ~watch:false) - [build; watch; clean; format; compiler_args; help] + let commands = [build; watch; clean; format; compiler_args; help] in + (* Options before a command name make Cmdliner read the name as the folder of + the default build, so [rescript -v watch] would build a folder named + watch. *) + let default = + Term.term_result ~usage:true + (let+ command = build_term ~watch:false in + match command with + | Build {folder} + when List.exists (fun cmd -> Cmd.name cmd = folder) commands -> + Error + (`Msg + (Printf.sprintf + "options go after the command: rescript %s [OPTION]… (write \ + ./%s for a folder named %s)" + folder folder folder)) + | command -> Ok command) + in + Cmd.group info ~default commands type evaluation = Run of command | Exit of int @@ -296,95 +313,20 @@ exception Parse_error of string exception Help exception Version -(* Bare project folders must select the default build command, even though - Cmdliner otherwise treats them as unknown commands. Move global options - behind the selected command and expand short help/version clusters because - Cmdliner's standard display options only provide long names. A root display - request takes precedence over otherwise invalid implicit-build arguments. *) +(* Cmdliner's standard display options only have long names. Accept -h and -V + as aliases, and print help as plain text instead of through a pager, up to a + "--" that ends the options. *) let normalize_argv argv = - let is_short_global_cluster argument = - let length = String.length argument in - length > 1 - && argument.[0] = '-' - && argument.[1] <> '-' - && String.for_all - (function - | 'v' | 'q' | 'h' | 'V' -> true - | _ -> false) - (String.sub argument 1 (length - 1)) - in - let short_cluster_contains character argument = - is_short_global_cluster argument - && String.contains_from argument 1 character - in - let is_global = function - | "--verbose" | "--quiet" | "--help" | "--version" -> true - | argument -> - is_short_global_cluster argument - || String.starts_with ~prefix:"--help=" argument - in - let requests_help argument = - argument = "--help" - || String.starts_with ~prefix:"--help=" argument - || short_cluster_contains 'h' argument - in - let requests_version argument = - argument = "--version" || short_cluster_contains 'V' argument - in - let is_command = function - | "build" | "watch" | "clean" | "format" | "compiler-args" | "help" -> true - | _ -> false - in - let rec normalize_display_options = function + let rec normalize = function | [] -> [] | "--" :: rest -> "--" :: rest - | argument :: rest when String.starts_with ~prefix:"--help=" argument -> - argument :: normalize_display_options rest - | argument :: rest when requests_help argument -> - "--help=plain" :: normalize_display_options rest - | argument :: rest when requests_version argument -> - "--version" :: normalize_display_options rest - | argument :: rest -> argument :: normalize_display_options rest - in - let explicit_command arguments = - let rec loop globals = function - | [] | "--" :: _ -> None - | argument :: rest when is_global argument -> - loop (argument :: globals) rest - | command :: rest when is_command command -> - Some (List.rev globals, command, rest) - | _ -> None - in - loop [] arguments - in - let partition_implicit arguments = - let rec loop globals others = function - | [] -> (List.rev globals, List.rev others) - | "--" :: rest -> (List.rev globals, List.rev_append others ("--" :: rest)) - | argument :: rest when is_global argument -> - loop (argument :: globals) others rest - | argument :: rest -> loop globals (argument :: others) rest - in - loop [] [] arguments + | ("-h" | "--help") :: rest -> "--help=plain" :: normalize rest + | "-V" :: rest -> "--version" :: normalize rest + | argument :: rest -> argument :: normalize rest in match Array.to_list argv with | [] -> argv - | executable :: arguments -> - let routed = - match explicit_command arguments with - | Some (globals, command, rest) -> - executable :: command :: (globals @ rest) - | None -> - let globals, others = partition_implicit arguments in - if - List.exists - (fun argument -> - requests_help argument || requests_version argument) - globals - then executable :: globals - else executable :: "build" :: (globals @ others) - in - Array.of_list (normalize_display_options routed) + | executable :: arguments -> Array.of_list (executable :: normalize arguments) (* Evaluates the command line and returns Cmdliner's result with the diagnostics it wrote, or [Error] for arguments that are not UTF-8. Help is diff --git a/rewatch-ocaml/tests/check_command_validation.sh b/rewatch-ocaml/tests/check_command_validation.sh index bb5d9d1d8b..93888d452b 100755 --- a/rewatch-ocaml/tests/check_command_validation.sh +++ b/rewatch-ocaml/tests/check_command_validation.sh @@ -474,7 +474,7 @@ run_build_output_case() { build_project="$work/$name-build" cp -R "$fixture" "$build_project" if [ "$mode" = quiet ]; then - run_rescript -q build "$build_project" + run_rescript build -q "$build_project" elif [ "$mode" = forced-color ]; then CLICOLOR_FORCE=1 run_rescript build "$build_project" else @@ -582,12 +582,15 @@ run_build_output_case quiet-warning 0 \ "$root/rewatch-ocaml/tests/warning-replay" quiet run_case build-subcommand-version accept build --version -run_case clustered-global-version accept -vV build -run_case clustered-global-verbosity accept -vvvvv build "$project" -run_case clustered-global-version-before-help accept -Vh build -run_case clustered-global-help-before-version accept -hV build -run_case clustered-subcommand-help-before-version accept build -hV -run_case clustered-subcommand-version-before-help accept build -Vh +run_case short-help accept -h +run_case short-version accept -V +run_case clustered-verbosity accept build -vvvvv "$project" +run_case options-before-command exit2 -v build +require_error_contains options-before-command \ + 'options go after the command: rescript build' +run_case folder-named-like-command reject -v ./build +require_error_contains folder-named-like-command \ + "Project folder './build' does not exist." "$rescript" build --help=groff >"$work/help-groff.out" if ! grep -F '.\" Pipe this output to groff' \ "$work/help-groff.out" >/dev/null; then @@ -597,11 +600,17 @@ if ! grep -F '.\" Pipe this output to groff' \ fi checked=$((checked + 1)) run_case implicit-conflicting-verbosity exit2 -v -q -require_error_contains implicit-conflicting-verbosity 'Usage: rescript build' +require_error_contains implicit-conflicting-verbosity \ + 'verbose cannot be used together with --quiet' run_case build-conflicting-verbosity exit2 build --verbose --quiet -run_case unknown-command reject bogus-command -require_error_contains unknown-command \ - "Project folder 'bogus-command' does not exist. Run 'rescript --help' for the available commands." +# Cmdliner quotes the command name when colors are off and styles it +# otherwise, so the name is checked apart from the message. +for display in "" --help -V; do + name=unknown-command${display:+-$display} + run_case "$name" exit2 bogus-command ${display:+"$display"} + require_error_contains "$name" 'unknown command' + require_error_contains "$name" 'bogus-command' +done run_case filter-perl-quoting accept build --filter '\QA.res\E' "$project" run_case filter-invalid exit2 build --filter '(' "$project" require_error_contains filter-invalid 'invalid regular expression' @@ -989,7 +998,7 @@ if [ "$rescript_status" -ne 0 ] || \ fi check_output_snapshot clean-redirected checked=$((checked + 1)) -run_rescript -q clean "$project" +run_rescript clean -q "$project" if [ "$rescript_status" -ne 0 ] || \ [ -s "$work/rescript.out" ] || [ -s "$work/rescript.err" ]; then echo "Quiet redirected clean emitted output" >&2 @@ -1122,7 +1131,7 @@ wait_for_file "$work/watch-dependency-recovery/src/A.js" terminate_and_wait "$dependency_recovery_pid" "dependency-recovery watcher" checked=$((checked + 1)) -"$rescript" -v watch "$work/watch-dependency-install" \ +"$rescript" watch -v "$work/watch-dependency-install" \ >"$work/watch-dependency-install.out" \ 2>"$work/watch-dependency-install.err" & dependency_install_pid=$! @@ -1150,7 +1159,7 @@ checked=$((checked + 1)) directory_link "$work/watch-dependency-fallback/packages/dep" \ "$work/node_modules/dep" -"$rescript" -v watch "$work/watch-dependency-fallback" \ +"$rescript" watch -v "$work/watch-dependency-fallback" \ >"$work/watch-dependency-fallback.out" \ 2>"$work/watch-dependency-fallback.err" & dependency_fallback_pid=$! @@ -1256,7 +1265,7 @@ checked=$((checked + 1)) quiet_watch_project="$work/quiet-watch" quiet_watch_marker="$quiet_watch_project/after-build.log" REWATCH_WATCH_FILTER_MARKER="$quiet_watch_marker" \ - "$rescript" -q watch \ + "$rescript" watch -q \ --after-build "node $command_work/watch-filter-marker.js" \ "$quiet_watch_project" \ >"$work/quiet-watch.out" 2>"$work/quiet-watch.err" & diff --git a/rewatch-ocaml/tests/check_interactive_output.sh b/rewatch-ocaml/tests/check_interactive_output.sh index 0b9ccf6e38..ed73733073 100755 --- a/rewatch-ocaml/tests/check_interactive_output.sh +++ b/rewatch-ocaml/tests/check_interactive_output.sh @@ -183,7 +183,7 @@ if $windows_posix_shell; then fi echo "Checking quiet interactive builds..." -run_in_terminal "$work/quiet.tty" -q build "$(command_path "$work/project")" +run_in_terminal "$work/quiet.tty" build -q "$(command_path "$work/project")" if tr '\r' '\n' <"$work/quiet.tty" \ | grep -E '(Cleaned|Parsed|Parsing\.\.\.|Compiled|Compiling\.\.\.|Finished .*compilation)' >/dev/null; then echo "A quiet interactive build emitted progress" >&2 @@ -216,7 +216,7 @@ if ! cmp -s "$work/expected-clean" "$work/clean.phases"; then fi echo "Checking quiet interactive cleans..." -run_in_terminal "$work/clean-quiet.tty" -q clean "$(command_path "$work/project")" +run_in_terminal "$work/clean-quiet.tty" clean -q "$(command_path "$work/project")" if tr '\r' '\n' <"$work/clean-quiet.tty" | grep -E '(Cleaning|Cleaned)' >/dev/null; then echo "A quiet interactive clean emitted progress" >&2 cat "$work/clean-quiet.tty" >&2 diff --git a/rewatch-ocaml/tests/check_verbose_output.sh b/rewatch-ocaml/tests/check_verbose_output.sh index 2cea41b861..30b2cdf29f 100755 --- a/rewatch-ocaml/tests/check_verbose_output.sh +++ b/rewatch-ocaml/tests/check_verbose_output.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Checks the DEBUG and TRACE events of `rescript -v build` and `-vv build` +# Checks the DEBUG and TRACE events of `rescript build -v` and `build -vv` # against snapshots/verbose_output. Only the event lines are compared, sorted, # so parallel compilation does not reorder them. set -eu @@ -45,7 +45,7 @@ capture() { project="$work/$level" ( cd "$project" - "$rescript" "-$level" build >"$work/$name.stdout" 2>"$work/$name.stderr" + "$rescript" build "-$level" >"$work/$name.stdout" 2>"$work/$name.stderr" ) if [ -s "$work/$name.stderr" ]; then echo "$name unexpectedly wrote verbose build output to stderr" >&2 diff --git a/rewatch-ocaml/tests/run.sh b/rewatch-ocaml/tests/run.sh index 44b53c7744..4dac2c2b2e 100644 --- a/rewatch-ocaml/tests/run.sh +++ b/rewatch-ocaml/tests/run.sh @@ -1077,7 +1077,7 @@ test ! -f "$watch_basic/src/New.js" # A shallow watch on the nearest existing ancestor must advance one directory # at a time until a configured nested source root exists. The slash in the JSON # path also exercises native separator normalization on Windows. -"$port" -v watch "$missing_nested_source" \ +"$port" watch -v "$missing_nested_source" \ >"$missing_nested_source/watch.log" 2>&1 & missing_nested_source_pid=$! background_pids="$background_pids $missing_nested_source_pid" @@ -1725,7 +1725,7 @@ awk 'BEGIN { for (i = 1; i <= 200000; i++) printf "let v%d = (%d, \"%d\")\n", i, >"$second_interrupt/src/Slow.res" # A second build waits for the first one's build lock and then runs. -"$port" -vv build "$second_interrupt" >"$second_interrupt/first.log" 2>&1 & +"$port" build -vv "$second_interrupt" >"$second_interrupt/first.log" 2>&1 & lock_first_pid=$! background_pids="$background_pids $lock_first_pid" # Logged when the parsed module is handed to the compiler. @@ -1759,7 +1759,7 @@ if ! $windows_posix_shell; then rm "$second_interrupt/src/Slow.js" mkdir -p "$second_interrupt/tmp" TMPDIR="$second_interrupt/tmp" \ - "$port" -vv build "$second_interrupt" >"$second_interrupt/build.log" 2>&1 & + "$port" build -vv "$second_interrupt" >"$second_interrupt/build.log" 2>&1 & second_interrupt_pid=$! background_pids="$background_pids $second_interrupt_pid" # Logged when the parsed module is handed to the compiler. @@ -1860,7 +1860,7 @@ if ! $windows_posix_shell; then expected_status=${interrupt_signal#*:} interrupt_build="$work/interrupt-build-$signal_name" slow_project "$interrupt_build" - "$port" -vv build "$interrupt_build" >"$interrupt_build/build.log" 2>&1 & + "$port" build -vv "$interrupt_build" >"$interrupt_build/build.log" 2>&1 & interrupt_build_pid=$! background_pids="$background_pids $interrupt_build_pid" if ! wait_for_text "$interrupt_build/build.log" 'compile dirty: Slow'; then @@ -1886,7 +1886,7 @@ fi # watcher's compiler work like a signal, before the queued dependent. lock_interrupt="$work/lock-interrupt" slow_project "$lock_interrupt" -"$port" -vv watch "$lock_interrupt" >"$lock_interrupt/watch.log" 2>&1 & +"$port" watch -vv "$lock_interrupt" >"$lock_interrupt/watch.log" 2>&1 & lock_interrupt_pid=$! background_pids="$background_pids $lock_interrupt_pid" if ! wait_for_text "$lock_interrupt/watch.log" 'compile dirty: Slow'; then @@ -1909,7 +1909,7 @@ fi # becoming the baseline of the next one. atomic_save="$work/atomic-save-rebuild" slow_project "$atomic_save" -"$port" -vv watch "$atomic_save" >"$atomic_save/watch.log" 2>&1 & +"$port" watch -vv "$atomic_save" >"$atomic_save/watch.log" 2>&1 & atomic_save_pid=$! background_pids="$background_pids $atomic_save_pid" if ! wait_for_text "$atomic_save/watch.log" 'Finished initial compilation' 1 1200; then diff --git a/rewatch/tests/watch/06-watch-missing-source-folder.sh b/rewatch/tests/watch/06-watch-missing-source-folder.sh index d05721cff2..8daab5233c 100755 --- a/rewatch/tests/watch/06-watch-missing-source-folder.sh +++ b/rewatch/tests/watch/06-watch-missing-source-folder.sh @@ -26,7 +26,7 @@ node -e " " # Start watcher and capture output (use -vv to get error logs) -rewatch_bg -vv watch > rewatch.log 2>&1 & +rewatch_bg watch -vv > rewatch.log 2>&1 & success "Watcher Started" # Wait for initial build to complete diff --git a/tests/build_tests/cli_help/input.js b/tests/build_tests/cli_help/input.js index e7d8be653a..ded42fa80b 100755 --- a/tests/build_tests/cli_help/input.js +++ b/tests/build_tests/cli_help/input.js @@ -369,12 +369,11 @@ await test(["-h"], { stdout: cliHelp, stderr: "", status: 0 }); // Shows cli help with -h arg await test(["help"], { stdout: cliHelp, stderr: "", status: 0 }); -// Exits with cli help with unknown command -// Exits with build usage on unknown args +// Exits with cli usage on unknown args await test(["--foo"], { stdout: "", stderr: - "Usage: rescript build [--help] [OPTION]… [FOLDER]\n" + + "Usage: rescript [--help] [COMMAND] …\n" + "rescript: unknown option --foo. Did you mean -f?\n", status: 2, }); diff --git a/tests/rewatch_ounit_tests/cli_tests.ml b/tests/rewatch_ounit_tests/cli_tests.ml index b9d2ca4524..6e9e888cff 100644 --- a/tests/rewatch_ounit_tests/cli_tests.ml +++ b/tests/rewatch_ounit_tests/cli_tests.ml @@ -39,40 +39,34 @@ let tests = | Cli.Build _ -> true | _ -> false) "no subcommand defaults to build"; + check (rejects ["someFolder"]) "a bare folder is an unknown command"; check - ((build_options ["someFolder"]).folder = "someFolder") - "a bare folder uses the implicit build command"; + ((build_options ["--prod"; "someFolder"]).folder = "someFolder") + "a folder after an option uses the default build command"; check - ((build_options ["my-project"; "-v"]).folder = "my-project") - "a trailing global verbosity flag keeps the implicit folder"; - check - ((build_options ["--"; "-v"]).folder = "-v") + ((build_options ["build"; "--"; "-v"]).folder = "-v") "double dash preserves an option-looking folder"; check - (shows_help ["some-folder"; "--help"]) - "implicit command folder help uses global help"; + (rejects ["some-folder"; "--help"]) + "help does not hide an unknown command"; + check + (rejects ["some-folder"; "-h"]) + "short help does not hide an unknown command"; check - (shows_help ["some-folder"; "-h"]) - "short implicit command help uses global help"; + (rejects ["some-folder"; "-V"]) + "version does not hide an unknown command"; + check (shows_help ["-h"]) "-h is short for --help"; + check (shows_version ["-V"]) "-V is short for --version"; check (shows_help ["build"; "--help"]) "explicit build displays command help"; check (shows_help ["build"; "-h"]) "explicit build accepts short command help"; + check (rejects ["-v"; "watch"]) "options before a command name are rejected"; + check (rejects ["--prod"; "build"]) "options before build are rejected"; check - (match parse ["-vvvv"; "watch"] with - | Cli.Watch _ -> true - | _ -> false) - "leading verbosity before watch"; - check - ((build_options ["-vvvvv"; "build"]).verbosity = 5) - "arbitrarily long clustered verbosity remains global"; + ((build_options ["-v"; "./watch"]).folder = "./watch") + "a folder named like a command can be built by its path"; check - (shows_help ["-Vh"; "build"]) - "help takes precedence over version in a leading cluster"; - check - (shows_help ["--version"; "build"; "--help"]) - "subcommand help takes precedence over a leading version option"; - check - (shows_help ["--version"; "build"; "--help=plain"]) - "formatted subcommand help takes precedence over a leading version option"; + ((build_options ["build"; "-vvvvv"]).verbosity = 5) + "clustered verbosity after the command is counted"; check (shows_help ["--help=plain"]) "formatted root help remains a root display option"; @@ -84,12 +78,6 @@ let tests = | Cli.Build _ -> true | _ -> false) "build accepts a trailing verbosity flag"; - check - (shows_version ["-V"; "build"]) - "a leading short version flag has global precedence"; - check - (shows_version ["some-folder"; "-V"]) - "implicit build extracts a trailing global version flag"; check (shows_version ["--version"]) "the long global version flag is accepted"; check (shows_version ["build"; "-V"]) @@ -123,12 +111,12 @@ let tests = | _ -> false) "clean parses --prod"; check - (match parse ["-q"; "clean"] with + (match parse ["clean"; "-q"] with | Cli.Clean {verbosity = -1; prod = false; folder = "."} -> true | _ -> false) - "clean retains the global quiet level"; + "clean parses the quiet level"; check (build_options ["--prod"]).prod - "--prod selects the implicit build command"; + "--prod selects the default build command"; check ((build_options ["build"; "--features"; " native , web "]).features = Some ["native"; "web"])