Fix resolving relative paths with corral run

.release-notes/216.md

@@ -0,0 +1,11 @@
+## Fix resolving relative paths for `corral run`
+
+Corral was failing when running commands with a relative path to the binary. E.g. `../../build/debug/ponyc`.
+This change switches relative binary resolution from using [`FilePath.from`](https://stdlib.ponylang.io/files-FilePath/#from) to [`Path.join`](https://stdlib.ponylang.io/files-Path/#join), in order to not fail if a path points to a parent directory, which is an error condition for [`FilePath.from`](https://stdlib.ponylang.io/files-FilePath/#from).
+
+`corral run` now also resolves relative paths against the current working directory first, before checking the `$PATH` environment variable.
+
+## Improved Error messages for `corral run`
+
+`corral run` will now print more detailed error messages when it is not able to run the given command.
+

corral/cmd/cmd_run.pony

@@ -42,24 +42,34 @@ class CmdRun is CmdType
     end
     ctx.log(Info) and ctx.log.log("run ponypath: " + ponypath)
 
-    try
-      let prog = Program(ctx.env, args(0)?)?
-      let vars = if ponypath.size() > 0 then
-          recover val [as String: "PONYPATH=" + ponypath] .> append(ctx.env.vars) end
-        else
-          ctx.env.vars
-        end
-      let a = Action(prog, recover args.slice(1) end, vars)
-      if not ctx.nothing then
-        Runner.run(a, {(result: ActionResult) =>
-          result.print_to(ctx.env.out)
-          if not result.successful() then
-            ctx.env.exitcode(result.exit_code())
-          end
-        })
+    let binary =
+      try
+        args(0)?
+      else
+        ctx.uout(Error) and ctx.uout.log("run: no run command provided")
+        ctx.env.exitcode(1)
+        return
+      end
+    let prog =
+      try
+        Program(ctx.env, binary)?
+      else
+        ctx.uout(Error) and ctx.uout.log("run: unable to find binary \"" + binary + "\" either in current directory or on $PATH.")
+        ctx.env.exitcode(1)
+        return
       end
-    else
-      ctx.uout(Error) and ctx.uout.log("run: " + "couldn't run program: " + " ".join(args.values()))
-      ctx.env.exitcode(1)
-      return
+
+    let vars = if ponypath.size() > 0 then
+        recover val [as String: "PONYPATH=" + ponypath] .> append(ctx.env.vars) end
+      else
+        ctx.env.vars
+      end
+    let a = Action(prog, recover args.slice(1) end, vars)
+    if not ctx.nothing then
+      Runner.run(a, {(result: ActionResult) =>
+        result.print_to(ctx.env.out)
+        if not result.successful() then
+          ctx.env.exitcode(result.exit_code())
+        end
+      })
     end

corral/test/_test.pony

@@ -26,6 +26,10 @@ actor \nodoc\ Main is TestList
     test(integration.TestUpdateSelfReferential)
     test(integration.TestRun)
     test(integration.TestRunWithoutBundle)
+    test(integration.TestRunNoArgs)
+    test(integration.TestRunBinaryNotFound)
+    test(integration.TestRunBinaryNotFoundAbsolute)
+    test(integration.TestRunBinaryInParentFolder)
     test(integration.TestClean)
 
     test(integration.TestUpdateScripts)

corral/test/integration/test_run.pony

@@ -3,7 +3,7 @@ use "ponytest"
 use ".."
 use "../../util"
 
-class  \nodoc\ TestRun is UnitTest
+class \nodoc\ TestRun is UnitTest
   fun name(): String => "integration/run"
   fun apply(h: TestHelper) ? =>
     h.long_test(30_000_000_000)
@@ -20,7 +20,7 @@ class  \nodoc\ TestRun is UnitTest
         h.complete(ar.exit_code() == 0)
       })
 
-class  \nodoc\ TestRunWithoutBundle is UnitTest
+class \nodoc\ TestRunWithoutBundle is UnitTest
   fun name(): String => "integration/run/without-bundle"
   fun apply(h: TestHelper) =>
     h.long_test(30_000_000_000)
@@ -36,3 +36,78 @@ class  \nodoc\ TestRunWithoutBundle is UnitTest
         h.assert_true(ar.stdout.lower().contains("compiled with: "))
         h.complete(ar.exit_code() == 0)
       })
+class \nodoc\ TestRunNoArgs is UnitTest
+  fun name(): String => "integration/run/no-args"
+  fun apply(h: TestHelper) ? =>
+    h.long_test(30_000_000_000)
+    Execute(h,
+      recover [
+        "run"
+        "--bundle_dir"; Data(h, "empty-deps")?.path
+        "--"
+      ] end,
+      {(h: TestHelper, ar: ActionResult) =>
+        h.assert_eq[I32](1, ar.exit_code())
+        h.assert_true(ar.stdout.lower().contains("no run command provided"))
+        h.complete(ar.exit_code() == 1)
+      })
+
+class \nodoc\ TestRunBinaryNotFound is UnitTest
+  fun name(): String => "integration/run/binary-not-found"
+  fun apply(h: TestHelper) ? =>
+    h.long_test(30_000_000_000)
+    Execute(h,
+      recover [
+        "run"
+        "--bundle_dir"; Data(h, "empty-deps")?.path
+        "--"
+        "grmpf_i_do_not_exist_anywhere_and_i_am_not_absolute"
+      ] end,
+      {(h: TestHelper, ar: ActionResult) =>
+        h.assert_eq[I32](1, ar.exit_code())
+        h.assert_true(ar.stdout.lower().contains("unable to find binary \"grmpf_i_do_not_exist_anywhere_and_i_am_not_absolute\""))
+        h.complete(ar.exit_code() == 1)
+      })
+
+class \nodoc\ TestRunBinaryNotFoundAbsolute is UnitTest
+  fun name(): String => "integration/run/binary-not-found-absolute"
+  fun apply(h: TestHelper) ? =>
+    h.long_test(30_000_000_000)
+    Execute(h,
+      recover [
+        "run"
+        "--bundle_dir"; Data(h, "empty-deps")?.path
+        "--"
+        "/path/to/grmpf_i_do_not_exist_anywhere"
+      ] end,
+      {(h: TestHelper, ar: ActionResult) =>
+        ar.print_to(h.env.out)
+        h.assert_ne[I32](0, ar.exit_code())
+        h.assert_true(ar.stdout.lower().contains("/path/to/grmpf_i_do_not_exist_anywhere does not exist or is a directory"))
+        h.complete(ar.exit_code() != 0)
+      })
+
+class \nodoc\ TestRunBinaryInParentFolder is UnitTest
+  fun name(): String => "integration/run/binary-in-parent-folder"
+  fun apply(h: TestHelper) ? =>
+    let cwd = Path.cwd()
+    let ponyc_path = try
+      RelativePathToPonyc(h)?
+    else
+      h.log("Unable to determine relative path to ponyc, skipping.")
+      return
+    end
+    h.log(ponyc_path)
+    h.long_test(30_000_000_000)
+    Execute(h,
+      recover [
+        "run"
+        "--bundle_dir"; Data(h, "empty-deps")?.path
+        "--"
+        "ponyc"; "-version"
+      ] end,
+      {(h: TestHelper, ar: ActionResult) =>
+        h.assert_eq[I32](0, ar.exit_code())
+        h.assert_true(ar.stdout.lower().contains("compiled with: "))
+        h.complete(ar.exit_code() == 0)
+      })

corral/test/util.pony

@@ -1,9 +1,11 @@
 use "cli"
 use "files"
+use "strings"
 use "ponytest"
 use "../util"
 
-interface  \nodoc\ val Checker
+
+interface \nodoc\ val Checker
   fun apply(h: TestHelper, ar: ActionResult)
 
 
@@ -75,3 +77,32 @@ class \nodoc\ val DataNone
   fun cleanup(h: TestHelper) => None
   fun dir(): String => ""
   fun dir_path(subdir: String): FilePath ? => error
+
+
+primitive RelativePathToPonyc
+  fun apply(h: TestHelper): String ? =>
+    var cwd = Path.cwd()
+    var ponyc_rel_path: String trn = recover trn String.create() end
+    let env = EnvVars(h.env.vars)
+    let path =
+      ifdef windows then
+        env("path")?
+      else
+        env("PATH")?
+      end
+    for bindir in Path.split_list(path).values() do
+      let ponyc_path = FilePath(h.env.root, Path.join(bindir, "ponyc"))
+      if ponyc_path.exists() then
+        let prefix: String val = CommonPrefix([cwd; ponyc_path.path])
+        while cwd.size() > prefix.size() do
+          cwd = Path.dir(cwd)
+          ponyc_rel_path = ponyc_rel_path + ".." + Path.sep()
+        end
+        ponyc_rel_path = ponyc_rel_path + ponyc_path.path.substring(prefix.size().isize())
+        return (consume ponyc_rel_path)
+      end
+    end
+    error
+
+
+

corral/util/action.pony

@@ -17,37 +17,41 @@ class val Program
     path = if Path.is_abs(name) then
       FilePath(auth, name)
     else
-      (let evars, let pathkey) =
-        ifdef windows then
-          // environment variables are case-insensitive on Windows
-          (EnvVars(env.vars, "", true), "path")
-        else
-          (EnvVars(env.vars), "PATH")
-        end
-      first_existing(auth, evars.get_or_else(pathkey, ""), name)?
+      let cwd = Path.cwd()
+      // first try to resolve the binary with the current directory as base
+      try
+        first_existing(auth, cwd, name)?
+      else
+        // then try with $PATH entries
+        (let evars, let pathkey) =
+          ifdef windows then
+            // environment variables are case-insensitive on Windows
+            (EnvVars(env.vars, "", true), "path")
+          else
+            (EnvVars(env.vars), "PATH")
+          end
+        first_existing(auth, evars.get_or_else(pathkey, ""), name)?
+      end
     end
 
   fun tag first_existing(auth': AmbientAuth, binpath: String, name: String)
     : FilePath ?
   =>
     for bindir in Path.split_list(binpath).values() do
-      try
-        let bd = FilePath(auth', bindir)
-        ifdef windows then
-          let bin_bare = FilePath.from(bd, name)?
-          if bin_bare.exists() then return bin_bare end
-          let bin_bat = FilePath.from(bd, name + ".bat")?
-          if bin_bat.exists() then return bin_bat end
-          let bin_ps1 = FilePath.from(bd, name + ".ps1")?
-          if bin_ps1.exists() then return bin_ps1 end
-          let bin_exe = FilePath.from(bd, name + ".exe")?
-          if bin_exe.exists() then return bin_exe end
-        else
-          let bin = FilePath.from(bd, name)?
-          if bin.exists() then
-            // TODO: should also stat for executable. FileInfo(bin)
-            return bin
-          end
+      ifdef windows then
+        let bin_bare = FilePath.create(auth', Path.join(bindir, name))
+        if bin_bare.exists() then return bin_bare end
+        let bin_bat = FilePath.create(auth', Path.join(bindir, name + ".bat"))
+        if bin_bat.exists() then return bin_bat end
+        let bin_ps1 = FilePath.create(auth', Path.join(bindir, name + ".ps1"))
+        if bin_ps1.exists() then return bin_ps1 end
+        let bin_exe = FilePath.create(auth', Path.join(bindir, name + ".exe"))
+        if bin_exe.exists() then return bin_exe end
+      else
+        let bin = FilePath.create(auth', Path.join(bindir, name))
+        if bin.exists() then
+          // TODO: should also stat for executable. FileInfo(bin)
+          return bin
         end
       end
     end
@@ -82,18 +86,23 @@ class val ActionResult
   let exit_status: ProcessExitStatus
   let stdout: String
   let stderr: String
-  let errmsg: String
+  let errmsg: (String | None)
 
   new val ok(exit_status': ProcessExitStatus, stdout': String, stderr': String) =>
     exit_status = exit_status'
     stdout = stdout'
     stderr = stderr'
-    errmsg = ""
-
-  new val fail(errmsg': String, exit_status': ProcessExitStatus = Exited(-1)) =>
+    errmsg = None
+
+  new val fail(
+    errmsg': String,
+    exit_status': ProcessExitStatus = Exited(-1),
+    stdout': String = "",
+    stderr': String = ""
+  ) =>
     exit_status = exit_status'
-    stdout = ""
-    stderr = ""
+    stdout = stdout'
+    stderr = stderr'
     errmsg = errmsg'
 
   fun val exit_code(): I32 =>
@@ -105,19 +114,24 @@ class val ActionResult
     end
 
   fun val print_to(out: OutStream) =>
-    out.print("  exit: " + exit_status.string())
+    match errmsg
+    | None =>
+      out.print("  exit: " + exit_status.string())
 
-    ifdef windows then
-      let stdout' = stdout.clone()
-      stdout'.replace("\r", "")
-      out.print("  out:\n" + (consume stdout'))
+      ifdef windows then
+        let stdout' = stdout.clone()
+        stdout'.replace("\r", "")
+        out.print("  out:\n" + (consume stdout'))
 
-      let stderr' = stderr.clone()
-      stderr'.replace("\r", "")
-      out.print("  err:\n" + (consume stderr'))
-    else
-      out.print("  out: " + stdout)
-      out.print("  err: " + stderr)
+        let stderr' = stderr.clone()
+        stderr'.replace("\r", "")
+        out.print("  err:\n" + (consume stderr'))
+      else
+        out.print("  out: " + stdout)
+        out.print("  err: " + stderr)
+      end
+    | let err: String =>
+      out.print("  failed: " + err)
     end
 
   fun successful(): Bool =>
@@ -179,7 +193,12 @@ class _Collector is ProcessNotify
     _stderr.append(consume data)
 
   fun ref failed(process: ProcessMonitor ref, err: ProcessError) =>
-    let cr = ActionResult.fail(err.string())
+    let cr = ActionResult.fail(
+      err.string()
+      where
+        stdout' = recover val _stdout.clone() end,
+        stderr' = recover val _stderr.clone() end
+    )
     _result(cr)
 
   fun ref dispose(process: ProcessMonitor ref, child_exit_status: ProcessExitStatus) =>