diff --git a/.release-notes/216.md b/.release-notes/216.md new file mode 100644 index 00000000..6570c25f --- /dev/null +++ b/.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. + diff --git a/CHANGELOG.md b/CHANGELOG.md index 923cbf24..a2dd47cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to Corral will be documented in this file. This project adhe ### Fixed +- Improved error messages for `corral run` ([PR #216](https://github.com/ponylang/corral/pull/216)) ### Added diff --git a/corral/cmd/cmd_run.pony b/corral/cmd/cmd_run.pony index 365cabf7..0a0d17bd 100644 --- a/corral/cmd/cmd_run.pony +++ b/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 diff --git a/corral/test/_test.pony b/corral/test/_test.pony index f8899993..61df6846 100644 --- a/corral/test/_test.pony +++ b/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) diff --git a/corral/test/integration/test_run.pony b/corral/test/integration/test_run.pony index 5d9d11ae..c90baa75 100644 --- a/corral/test/integration/test_run.pony +++ b/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,79 @@ 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) + let path = Path.join("/path/to", "grmpf_i_do_not_exist_anywhere") + Execute(h, + recover [ + "run" + "--bundle_dir"; Data(h, "empty-deps")?.path + "--" + path + ] end, + {(h: TestHelper, ar: ActionResult) => + h.assert_ne[I32](0, ar.exit_code()) + h.assert_true(ar.stdout.lower().contains(path + " 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) + }) diff --git a/corral/test/util.pony b/corral/test/util.pony index ae3cb31c..cad9482f 100644 --- a/corral/test/util.pony +++ b/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,28 @@ 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 diff --git a/corral/util/action.pony b/corral/util/action.pony index 1d8ea7ff..d11033da 100644 --- a/corral/util/action.pony +++ b/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) =>