Cmd API overhaul (#397)

* WIP Cmd API overhaul

* overhaul progress

* finished Cmd API

* refactor Cmd stuff

* error test coverage + tidying

* misc fixes

Author: Anton-4

build.roc

@@ -47,7 +47,6 @@ roc_version! = |roc_cmd|
     info!("Checking provided roc; executing `${roc_cmd} version`:")?
 
     Cmd.exec!(roc_cmd, ["version"])
-    |> Result.map_err(RocVersionCheckFailed)
 
 get_os_and_arch! : {} => Result OSAndArch _
 get_os_and_arch! = |{}|
@@ -78,7 +77,6 @@ build_stub_app_lib! = |roc_cmd, stub_lib_path|
     info!("Building stubbed app shared library ...")?
 
     Cmd.exec!(roc_cmd, ["build", "--lib", "platform/libapp.roc", "--output", stub_lib_path, "--optimize"])
-    |> Result.map_err(ErrBuildingAppStub)
 
 stub_file_extension : OSAndArch -> Str
 stub_file_extension = |os_and_arch|
@@ -130,7 +128,6 @@ cargo_build_host! = |debug_mode|
     args = cargo_build_args!({})?
 
     Cmd.exec!("cargo", args)
-    |> Result.map_err(ErrBuildingHostBinaries)
 
 copy_host_lib! : OSAndArch, Str => Result {} _
 copy_host_lib! = |os_and_arch, rust_target_folder|
@@ -142,7 +139,6 @@ copy_host_lib! = |os_and_arch, rust_target_folder|
     info!("Moving the prebuilt binary from ${host_build_path} to ${host_dest_path} ...")?
 
     Cmd.exec!("cp", [host_build_path, host_dest_path])
-    |> Result.map_err(ErrMovingPrebuiltLegacyBinary)
 
 preprocess_host! : Str, Str, Str => Result {} _
 preprocess_host! = |roc_cmd, stub_lib_path, rust_target_folder|
@@ -152,7 +148,6 @@ preprocess_host! = |roc_cmd, stub_lib_path, rust_target_folder|
     surgical_build_path = "${rust_target_folder}host"
 
     Cmd.exec!(roc_cmd, ["preprocess-host", surgical_build_path, "platform/main.roc", stub_lib_path])
-    |> Result.map_err(ErrPreprocessingSurgicalBinary)
 
 info! : Str => Result {} _
 info! = |msg|

ci/check_all_exposed_funs_tested.roc

@@ -106,39 +106,29 @@ is_function_unused! = |module_name, function_name|
     )?
 
     # Check if ripgrep is installed
-    rg_check_cmd = Cmd.new("rg") |> Cmd.arg("--version")
-    rg_check_output = Cmd.output!(rg_check_cmd)
-
-    when rg_check_output.status is
-        Ok(0) ->
-                unused_in_dir =
-                    search_dirs
-                    |> List.map_try!( |search_dir|
-                        # Skip searching if directory doesn't exist
-                        dir_exists = File.is_dir!(search_dir)?
-                        if !dir_exists then
-                            Ok(Bool.true) # Consider unused if we can't search
-                        else
-                            # Use ripgrep to search for the function pattern
-                            cmd =
-                                Cmd.new("rg")
-                                |> Cmd.arg("-q") # Quiet mode - we only care about exit code
-                                |> Cmd.arg(function_pattern)
-                                |> Cmd.arg(search_dir)
-
-                            status_res = Cmd.status!(cmd)
-
-                            # ripgrep returns status 0 if matches were found, 1 if no matches
-                            when status_res is
-                                Ok(0) -> Ok(Bool.false) # Function is used (not unused)
-                                _ -> Ok(Bool.true)
-                    )?
-
-                unused_in_dir
-                |> List.walk!(Bool.true, |state, is_unused_res| state && is_unused_res)
-                |> Ok
-        _ ->
-            err_s("Error: ripgrep (rg) is not installed or not available in PATH. Please install ripgrep to use this script. Full output: ${Inspect.to_str(rg_check_output)}")
+    _ = Cmd.exec!("rg", ["--version"]) ? |err| RipgrepNotInstalled(err)
+
+
+    unused_in_dir =
+        search_dirs
+        |> List.map_try!( |search_dir|
+            # Skip searching if directory doesn't exist
+            dir_exists = File.is_dir!(search_dir)?
+            if !dir_exists then
+                Ok(Bool.true) # Consider unused if we can't search
+            else
+                # Use ripgrep to search for the function pattern
+                cmd_res =
+                    Cmd.exec!("rg", ["-q", function_pattern, search_dir])
+
+                when cmd_res is
+                    Ok(_) -> Ok(Bool.false) # Function is used (not unused)
+                    _ -> Ok(Bool.true)
+        )?
+
+    unused_in_dir
+    |> List.walk!(Bool.true, |state, is_unused_res| state && is_unused_res)
+    |> Ok
 
 
 

ci/expect_scripts/cmd-test.exp

@@ -0,0 +1,27 @@
+#!/usr/bin/expect
+
+# uncomment line below for debugging
+# exp_internal 1
+
+set timeout 7
+
+source ./ci/expect_scripts/shared-code.exp
+
+spawn $env(TESTS_DIR)cmd-test
+
+
+set expected_output [normalize_output {
+cat: non_existent.txt: No such file or directory
+cat: non_existent.txt: No such file or directory
+cat: non_existent.txt: No such file or directory
+All tests passed.
+}]
+
+expect $expected_output {
+    expect eof {
+        check_exit_and_segfault
+    }
+}
+
+puts stderr "\nExpect script failed: output was not as expected. Diff the output with expected_output in this script. Alternatively, uncomment `exp_internal 1` to debug."
+exit 1

ci/expect_scripts/command.exp

@@ -12,13 +12,14 @@ spawn $env(EXAMPLES_DIR)command
 
 set expected_output [normalize_output {
 Hello
-Command output: Hi
-
-Command output: BAZ=DUCK
+\{stderr_utf8_lossy: "", stdout_utf8: "Hi
+"\}
+BAZ=DUCK
 FOO=BAR
 XYZ=ABC
-
-Yo
+cat: non_existent.txt: No such file or directory
+Exit code: 1
+\{stderr_bytes: \[\], stdout_bytes: \[72, 105, 10\]\}
 }]
 
 expect $expected_output {

ci/expect_scripts/path-test.exp

@@ -24,7 +24,6 @@ Path with replaced extension: test_file.new
 Extension replaced: Bool.true
 
 Testing Path file operations:
-test_path_bytes.txt exists: Bool.true
 Bytes written: \\\[72, 101, 108, 108, 111, 44, 32, 80, 97, 116, 104, 33\\\]
 Bytes read: \\\[72, 101, 108, 108, 111, 44, 32, 80, 97, 116, 104, 33\\\]
 Bytes match: Bool.true
@@ -35,13 +34,9 @@ UTF-8 content matches: Bool.true
 JSON content: {\"message\":\"Path test\",\"numbers\":\\\[1,2,3\\\]}
 JSON contains 'message' field: Bool.true
 JSON contains 'numbers' field: Bool.true
-File exists before delete: Bool.true
-File exists after delete: Bool.false
-
-Testing Path directory operations:
-Created directory: drwxr-xr-x \\d+ \\w+ (\\w+ )? *\\d+ \\w+ +\\d+ \\d+:\\d+ test_single_dir
-Is a directory: Bool.true
+File no longer exists: Bool.true
 
+Testing Path directory operations...
 Nested directory structure:
 test_parent
 test_parent/test_child
@@ -56,11 +51,10 @@ dr\[-rwx\]+ +\\d+ \\w+ (\\w+ )? *\\d+ \\w+ +\\d+ \\d+:\\d+ \\.\\.
 -\[-rwx\]+ +\\d+ \\w+ (\\w+ )? *\\d+ \\w+ +\\d+ \\d+:\\d+ file2\\.txt
 dr\[-rwx\]+ +\\d+ \\w+ (\\w+ )? *\\d+ \\w+ +\\d+ \\d+:\\d+ subdir
 
-Empty dir exists before delete: Bool.true
-Empty dir exists after delete: Bool.false
+Empty dir was deleted: Bool.true
 Size before delete_all: \\d+\\w*\\s*test_parent
 
-Parent dir exists after delete_all: Bool.false
+Parent dir no longer exists: Bool.true
 
 Testing Path.hard_link!:
 Hard link count before: 1
@@ -96,7 +90,13 @@ Files to clean up:
 -rw-r--r-- \\d+ \\w+ \\w+ \\d+ \\w+ +\\d+ \\d+:\\d+ test_path_rename_new\\.txt
 -rw-r--r-- \\d+ \\w+ \\w+ \\d+ \\w+ +\\d+ \\d+:\\d+ test_path_utf8\\.txt
 
-Files remaining after cleanup: Bool.false
+ls: cannot access .*
+ls: cannot access .*
+ls: cannot access .*
+ls: cannot access .*
+ls: cannot access .*
+ls: cannot access .*
+Files deleted successfully: Bool.true
 "]
 
 expect -re $expected_output {

crates/roc_command/src/lib.rs

@@ -1,4 +1,5 @@
 //! This crate provides common functionality for Roc to interface with `std::process::Command`
+
 use roc_std::{RocList, RocResult, RocStr};
 
 #[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord, Hash)]
@@ -61,29 +62,50 @@ impl From<&Command> for std::process::Command {
 
 #[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord, Hash)]
 #[repr(C)]
-pub struct OutputFromHost {
-    pub status: roc_std::RocResult<i32, roc_io_error::IOErr>,
-    pub stderr: roc_std::RocList<u8>,
-    pub stdout: roc_std::RocList<u8>,
+pub struct OutputFromHostSuccess {
+    pub stderr_bytes: roc_std::RocList<u8>,
+    pub stdout_bytes: roc_std::RocList<u8>,
+}
+
+#[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[repr(C)]
+pub struct OutputFromHostFailure {
+    pub stderr_bytes: roc_std::RocList<u8>,
+    pub stdout_bytes: roc_std::RocList<u8>,
+    pub exit_code: i32,
 }
 
-impl roc_std::RocRefcounted for OutputFromHost {
+impl roc_std::RocRefcounted for OutputFromHostSuccess {
     fn inc(&mut self) {
-        self.status.inc();
-        self.stderr.inc();
-        self.stdout.inc();
+        self.stdout_bytes.inc();
+        self.stderr_bytes.inc();
     }
     fn dec(&mut self) {
-        self.status.dec();
-        self.stderr.dec();
-        self.stdout.dec();
+        self.stdout_bytes.dec();
+        self.stderr_bytes.dec();
     }
     fn is_refcounted() -> bool {
         true
     }
 }
 
-pub fn command_status(roc_cmd: &Command) -> RocResult<i32, roc_io_error::IOErr> {
+impl roc_std::RocRefcounted for OutputFromHostFailure {
+    fn inc(&mut self) {
+        self.exit_code.inc();
+        self.stdout_bytes.inc();
+        self.stderr_bytes.inc();
+    }
+    fn dec(&mut self) {
+        self.exit_code.dec();
+        self.stdout_bytes.dec();
+        self.stderr_bytes.dec();
+    }
+    fn is_refcounted() -> bool {
+        true
+    }
+}
+
+pub fn command_exec_exit_code(roc_cmd: &Command) -> RocResult<i32, roc_io_error::IOErr> {
     match std::process::Command::from(roc_cmd).status() {
         Ok(status) => from_exit_status(status),
         Err(err) => RocResult::err(err.into()),
@@ -94,29 +116,44 @@ pub fn command_status(roc_cmd: &Command) -> RocResult<i32, roc_io_error::IOErr>
 fn from_exit_status(status: std::process::ExitStatus) -> RocResult<i32, roc_io_error::IOErr> {
     match status.code() {
         Some(code) => RocResult::ok(code),
-        None => killed_by_signal(),
+        None => RocResult::err(killed_by_signal_err()),
     }
 }
 
-// If no exit code is returned, the process was terminated by a signal.
-fn killed_by_signal() -> RocResult<i32, roc_io_error::IOErr> {
-    RocResult::err(roc_io_error::IOErr {
+fn killed_by_signal_err() -> roc_io_error::IOErr {
+    roc_io_error::IOErr {
         tag: roc_io_error::IOErrTag::Other,
-        msg: "Killed by signal".into(),
-    })
+        msg: "Process was killed by operating system signal.".into(),
+    }
 }
 
-pub fn command_output(roc_cmd: &Command) -> OutputFromHost {
+// TODO Can we make this return a tag union (with three variants) ?
+pub fn command_exec_output(roc_cmd: &Command) -> RocResult<OutputFromHostSuccess, RocResult<OutputFromHostFailure, roc_io_error::IOErr>> {
     match std::process::Command::from(roc_cmd).output() {
-        Ok(output) => OutputFromHost {
-            status: from_exit_status(output.status),
-            stdout: RocList::from(&output.stdout[..]),
-            stderr: RocList::from(&output.stderr[..]),
-        },
-        Err(err) => OutputFromHost {
-            status: RocResult::err(err.into()),
-            stdout: RocList::empty(),
-            stderr: RocList::empty(),
-        },
+        Ok(output) =>
+            match output.status.code() {
+                Some(status) => {
+
+                    let stdout_bytes = RocList::from(&output.stdout[..]);
+                    let stderr_bytes = RocList::from(&output.stderr[..]);
+
+                    if status == 0 {
+                        // Success case
+                        RocResult::ok(OutputFromHostSuccess {
+                            stderr_bytes,
+                            stdout_bytes,
+                        })
+                    } else {
+                        // Failure case
+                        RocResult::err(RocResult::ok(OutputFromHostFailure {
+                            stderr_bytes,
+                            stdout_bytes,
+                            exit_code: status,
+                        }))
+                    }
+                },
+                None => RocResult::err(RocResult::err(killed_by_signal_err()))
+            }
+        Err(err) => RocResult::err(RocResult::err(err.into()))
     }
 }

crates/roc_host/src/lib.rs

@@ -339,8 +339,8 @@ pub fn init() {
         roc_fx_tcp_read_exactly as _,
         roc_fx_tcp_read_until as _,
         roc_fx_tcp_write as _,
-        roc_fx_command_status as _,
-        roc_fx_command_output as _,
+        roc_fx_command_exec_exit_code as _,
+        roc_fx_command_exec_output as _,
         roc_fx_dir_create as _,
         roc_fx_dir_create_all as _,
         roc_fx_dir_delete_empty as _,
@@ -742,17 +742,17 @@ pub extern "C" fn roc_fx_tcp_write(stream: RocBox<()>, msg: &RocList<u8>) -> Roc
 }
 
 #[no_mangle]
-pub extern "C" fn roc_fx_command_status(
+pub extern "C" fn roc_fx_command_exec_exit_code(
     roc_cmd: &roc_command::Command,
 ) -> RocResult<i32, roc_io_error::IOErr> {
-    roc_command::command_status(roc_cmd)
+    roc_command::command_exec_exit_code(roc_cmd)
 }
 
 #[no_mangle]
-pub extern "C" fn roc_fx_command_output(
+pub extern "C" fn roc_fx_command_exec_output(
     roc_cmd: &roc_command::Command,
-) -> roc_command::OutputFromHost {
-    roc_command::command_output(roc_cmd)
+) -> RocResult<roc_command::OutputFromHostSuccess, RocResult<roc_command::OutputFromHostFailure, roc_io_error::IOErr>> {
+    roc_command::command_exec_output(roc_cmd)
 }
 
 #[no_mangle]