From c3d0c8b52e6e61adf810d7958ca4672a7c714909 Mon Sep 17 00:00:00 2001 From: Artem Baguinski Date: Sat, 24 Aug 2024 11:23:23 +0000 Subject: [PATCH 1/6] Allow calling chmod in tests --- tests/test.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test.rs b/tests/test.rs index f1b73bf161..4428109349 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -1,4 +1,4 @@ -use {super::*, pretty_assertions::assert_eq}; +use {super::*, fs::Permissions, pretty_assertions::assert_eq}; macro_rules! test { { @@ -99,6 +99,11 @@ impl Test { self } + pub(crate) fn chmod(self, path: impl AsRef, perm: Permissions) -> Self { + fs::set_permissions(self.tempdir.path().join(path.as_ref()), perm).unwrap(); + self + } + pub(crate) fn current_dir(mut self, path: impl AsRef) -> Self { path.as_ref().clone_into(&mut self.current_dir); self From 251445455c2c67c1c6c5ab09199138aa350bc44b Mon Sep 17 00:00:00 2001 From: Artem Baguinski Date: Sat, 24 Aug 2024 10:31:59 +0000 Subject: [PATCH 2/6] Improve error message on bad working directory Handles two cases: - working directory doesn't exist. - working directory cannot be changed into due to on of the following: - The process lacks permissions to view the contents. - The path points at a non-directory file. Addresses: #2295 --- src/error.rs | 11 +++++++ src/recipe.rs | 9 ++++++ tests/working_directory.rs | 60 +++++++++++++++++++++++++++++++++++++- 3 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/error.rs b/src/error.rs index cd22311e7b..d05faf8cb3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -127,6 +127,11 @@ pub(crate) enum Error<'src> { MissingModuleFile { module: Name<'src>, }, + WorkingDirectoryIo { + recipe: &'src str, + working_directory: PathBuf, + io_error: io::Error, + }, NoChoosableRecipes, NoDefaultRecipe, NoRecipes, @@ -470,6 +475,12 @@ impl<'src> ColorDisplay for Error<'src> { UnstableFeature { unstable_feature } => { write!(f, "{unstable_feature} Invoke `just` with `--unstable`, set the `JUST_UNSTABLE` environment variable, or add `set unstable` to your `justfile` to enable unstable features.")?; } + WorkingDirectoryIo { recipe, working_directory, io_error } => { + match io_error.kind() { + io::ErrorKind::NotFound => write!(f, "Recipe `{recipe}` could not be run because just could not find working directory `{}`: {io_error}", working_directory.to_string_lossy()), + _ => write!(f, "Recipe `{recipe}` could not be run because just could not set working directory to `{}`: {io_error}", working_directory.to_string_lossy()), + }?; + }, WriteJustfile { justfile, io_error } => { let justfile = justfile.display(); write!(f, "Failed to write justfile to `{justfile}`: {io_error}")?; diff --git a/src/recipe.rs b/src/recipe.rs index 3976983aef..07a7cedd74 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -304,6 +304,15 @@ impl<'src, D> Recipe<'src, D> { } } Err(io_error) => { + if let Some(working_directory) = self.working_directory(context) { + if let Err(io_error) = fs::read_dir(working_directory.clone()) { + return Err(Error::WorkingDirectoryIo { + recipe: self.name(), + working_directory, + io_error, + }); + } + } return Err(Error::Io { recipe: self.name(), io_error, diff --git a/tests/working_directory.rs b/tests/working_directory.rs index 3396b73eb7..9efddeb926 100644 --- a/tests/working_directory.rs +++ b/tests/working_directory.rs @@ -1,4 +1,4 @@ -use super::*; +use {super::*, fs::Permissions, std::os::unix::fs::PermissionsExt}; const JUSTFILE: &str = r#" foo := `cat data` @@ -331,3 +331,61 @@ file := shell('cat file.txt') .stdout("FILE\n") .run(); } + +#[test] +fn missing_working_directory_produces_clear_message() { + Test::new() + .justfile( + " + set working-directory := 'missing' + default: + pwd + ", + ) + .status(1) + .stderr_regex( + ".*Recipe `default` could not be run because just could not find working directory `.*/missing`.*", + ) + .run(); +} + +#[test] +fn unusable_working_directory_produces_clear_message() { + Test::new() + .justfile( + " + set working-directory := 'unusable' + default: + pwd + ", + ) + .tree(tree! { + unusable: {} + }) + .chmod("unusable", Permissions::from_mode(0o000)) + .status(1) + .stderr_regex( + ".*Recipe `default` could not be run because just could not set working directory to `.*/unusable`:.*", + ) + .run(); +} + +#[test] +fn working_directory_is_not_a_directory_produces_clear_message() { + Test::new() + .justfile( + " + set working-directory := 'unusable' + default: + pwd + ", + ) + .tree(tree! { + unusable: "is not a directory" + }) + .status(1) + .stderr_regex( + ".*Recipe `default` could not be run because just could not set working directory to `.*/unusable`:.*", + ) + .run(); +} From 98b6e4bdc4dfaf0b0992d7bbd1cd2647d26b0fb2 Mon Sep 17 00:00:00 2001 From: Artem Baguinski Date: Sun, 25 Aug 2024 04:07:01 +0000 Subject: [PATCH 3/6] Implement the PR suggestions --- src/error.rs | 15 ++++++--------- src/recipe.rs | 2 +- tests/working_directory.rs | 6 ++++-- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/error.rs b/src/error.rs index d05faf8cb3..ebd05b2cc4 100644 --- a/src/error.rs +++ b/src/error.rs @@ -127,11 +127,6 @@ pub(crate) enum Error<'src> { MissingModuleFile { module: Name<'src>, }, - WorkingDirectoryIo { - recipe: &'src str, - working_directory: PathBuf, - io_error: io::Error, - }, NoChoosableRecipes, NoDefaultRecipe, NoRecipes, @@ -191,6 +186,11 @@ pub(crate) enum Error<'src> { justfile: PathBuf, io_error: io::Error, }, + WorkingDirectoryIo { + recipe: &'src str, + working_directory: PathBuf, + io_error: io::Error, + }, } impl<'src> Error<'src> { @@ -476,10 +476,7 @@ impl<'src> ColorDisplay for Error<'src> { write!(f, "{unstable_feature} Invoke `just` with `--unstable`, set the `JUST_UNSTABLE` environment variable, or add `set unstable` to your `justfile` to enable unstable features.")?; } WorkingDirectoryIo { recipe, working_directory, io_error } => { - match io_error.kind() { - io::ErrorKind::NotFound => write!(f, "Recipe `{recipe}` could not be run because just could not find working directory `{}`: {io_error}", working_directory.to_string_lossy()), - _ => write!(f, "Recipe `{recipe}` could not be run because just could not set working directory to `{}`: {io_error}", working_directory.to_string_lossy()), - }?; + write!(f, "Recipe `{recipe}` could not be run because just could not set working directory to `{}`: {io_error}", working_directory.display())?; }, WriteJustfile { justfile, io_error } => { let justfile = justfile.display(); diff --git a/src/recipe.rs b/src/recipe.rs index 07a7cedd74..e3645ae3fe 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -305,7 +305,7 @@ impl<'src, D> Recipe<'src, D> { } Err(io_error) => { if let Some(working_directory) = self.working_directory(context) { - if let Err(io_error) = fs::read_dir(working_directory.clone()) { + if let Err(io_error) = fs::read_dir(&working_directory) { return Err(Error::WorkingDirectoryIo { recipe: self.name(), working_directory, diff --git a/tests/working_directory.rs b/tests/working_directory.rs index 9efddeb926..a7691285e8 100644 --- a/tests/working_directory.rs +++ b/tests/working_directory.rs @@ -1,4 +1,4 @@ -use {super::*, fs::Permissions, std::os::unix::fs::PermissionsExt}; +use super::*; const JUSTFILE: &str = r#" foo := `cat data` @@ -344,13 +344,15 @@ fn missing_working_directory_produces_clear_message() { ) .status(1) .stderr_regex( - ".*Recipe `default` could not be run because just could not find working directory `.*/missing`.*", + ".*Recipe `default` could not be run because just could not set working directory to `.*/missing`.*", ) .run(); } #[test] +#[cfg(unix)] fn unusable_working_directory_produces_clear_message() { + use {fs::Permissions, std::os::unix::fs::PermissionsExt}; Test::new() .justfile( " From 3c03e88acdb3567877a50e40cdbe3900e77b753e Mon Sep 17 00:00:00 2001 From: Artem Baguinski Date: Sun, 1 Sep 2024 07:50:37 +0000 Subject: [PATCH 4/6] Improve io error message context - include the failed recipe, shell and working directory - include the original io error message from Command.status() - include the io error message from read_dir() check if it differs from the original On Unix the two error messages don't differ in the cases that we test, but we don't want to assume that is always the case. Additionally test error messages when both workdir and shell are unusable. --- src/error.rs | 26 +++++++------- src/recipe.rs | 23 ++++++++----- tests/shell.rs | 2 +- tests/working_directory.rs | 70 +++++++++++++++++++++++++++++++++----- 4 files changed, 89 insertions(+), 32 deletions(-) diff --git a/src/error.rs b/src/error.rs index ebd05b2cc4..abbe7ed66b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -116,6 +116,9 @@ pub(crate) enum Error<'src> { Io { recipe: &'src str, io_error: io::Error, + shell: String, + working_directory_io_error: Option, + working_directory: Option, }, Load { path: PathBuf, @@ -186,11 +189,6 @@ pub(crate) enum Error<'src> { justfile: PathBuf, io_error: io::Error, }, - WorkingDirectoryIo { - recipe: &'src str, - working_directory: PathBuf, - io_error: io::Error, - }, } impl<'src> Error<'src> { @@ -401,12 +399,15 @@ impl<'src> ColorDisplay for Error<'src> { write!(f, "Internal runtime error, this may indicate a bug in just: {message} \ consider filing an issue: https://github.com/casey/just/issues/new")?; } - Io { recipe, io_error } => { - match io_error.kind() { - io::ErrorKind::NotFound => write!(f, "Recipe `{recipe}` could not be run because just could not find the shell: {io_error}"), - io::ErrorKind::PermissionDenied => write!(f, "Recipe `{recipe}` could not be run because just could not run the shell: {io_error}"), - _ => write!(f, "Recipe `{recipe}` could not be run because of an IO error while launching the shell: {io_error}"), - }?; + Io { recipe, io_error, shell, working_directory_io_error, working_directory } => { + write!(f, "Failed to run recipe `{recipe}`:\n Failed to run shell `{shell}`:\n {io_error}", )?; + if let Some(working_directory_io_error) = working_directory_io_error { + let working_directory = working_directory.as_ref().expect("is Some when error is Some").display(); + write!(f, "\n Failed to set working directory to `{working_directory}`")?; + if working_directory_io_error.to_string() != io_error.to_string() { + write!(f, ":\n {working_directory_io_error}")?; + } + } } Load { io_error, path } => { write!(f, "Failed to read justfile at `{}`: {io_error}", path.display())?; @@ -475,9 +476,6 @@ impl<'src> ColorDisplay for Error<'src> { UnstableFeature { unstable_feature } => { write!(f, "{unstable_feature} Invoke `just` with `--unstable`, set the `JUST_UNSTABLE` environment variable, or add `set unstable` to your `justfile` to enable unstable features.")?; } - WorkingDirectoryIo { recipe, working_directory, io_error } => { - write!(f, "Recipe `{recipe}` could not be run because just could not set working directory to `{}`: {io_error}", working_directory.display())?; - }, WriteJustfile { justfile, io_error } => { let justfile = justfile.display(); write!(f, "Failed to write justfile to `{justfile}`: {io_error}")?; diff --git a/src/recipe.rs b/src/recipe.rs index e3645ae3fe..a78b1ec101 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -304,18 +304,23 @@ impl<'src, D> Recipe<'src, D> { } } Err(io_error) => { - if let Some(working_directory) = self.working_directory(context) { - if let Err(io_error) = fs::read_dir(&working_directory) { - return Err(Error::WorkingDirectoryIo { - recipe: self.name(), - working_directory, - io_error, - }); - } - } + let working_directory = self.working_directory(context); + let working_directory_io_error = match &working_directory { + Some(working_directory) => match fs::read_dir(&working_directory) { + Ok(_) => None, + Err(io_error) => Some(io_error), + }, + // no working directory = no attempt to change current working directory = no error + None => None, + }; + let (shell, _) = context.module.settings.shell(config); + return Err(Error::Io { recipe: self.name(), io_error, + shell: shell.into(), + working_directory_io_error, + working_directory, }); } }; diff --git a/tests/shell.rs b/tests/shell.rs index 7096f7ae19..ccae0d5272 100644 --- a/tests/shell.rs +++ b/tests/shell.rs @@ -164,7 +164,7 @@ fn recipe_shell_not_found_error_message() { .shell(false) .args(["--shell", "NOT_A_REAL_SHELL"]) .stderr_regex( - "error: Recipe `foo` could not be run because just could not find the shell: .*\n", + ".*Failed to run recipe `foo`:\n Failed to run shell `NOT_A_REAL_SHELL`:\n .*", ) .status(1) .run(); diff --git a/tests/working_directory.rs b/tests/working_directory.rs index a7691285e8..28472e38cb 100644 --- a/tests/working_directory.rs +++ b/tests/working_directory.rs @@ -343,9 +343,7 @@ fn missing_working_directory_produces_clear_message() { ", ) .status(1) - .stderr_regex( - ".*Recipe `default` could not be run because just could not set working directory to `.*/missing`.*", - ) + .stderr_regex(".*Failed to run recipe `default`:\n Failed to run shell `bash`:\n .*\n Failed to set working directory to `.*/missing`.*") .run(); } @@ -366,9 +364,7 @@ fn unusable_working_directory_produces_clear_message() { }) .chmod("unusable", Permissions::from_mode(0o000)) .status(1) - .stderr_regex( - ".*Recipe `default` could not be run because just could not set working directory to `.*/unusable`:.*", - ) + .stderr_regex(".*Failed to run recipe `default`:\n Failed to run shell `bash`:\n .*\n Failed to set working directory to `.*/unusable`.*") .run(); } @@ -386,8 +382,66 @@ fn working_directory_is_not_a_directory_produces_clear_message() { unusable: "is not a directory" }) .status(1) - .stderr_regex( - ".*Recipe `default` could not be run because just could not set working directory to `.*/unusable`:.*", + .stderr_regex(".*Failed to run recipe `default`:\n Failed to run shell `bash`:\n .*\n Failed to set working directory to `.*/unusable`.*") + .run(); +} + +#[test] +fn missing_working_directory_and_missing_shell_produces_clear_message() { + Test::new() + .justfile( + " + set working-directory := 'missing' + default: + pwd + ", + ) + .shell(false) + .args(["--shell", "NOT_A_REAL_SHELL"]) + .status(1) + .stderr_regex(".*Failed to run recipe `default`:\n Failed to run shell `NOT_A_REAL_SHELL`:\n .*\n Failed to set working directory to `.*/missing`.*") + .run(); +} + +#[test] +#[cfg(unix)] +fn unusable_working_directory_and_missing_shell_produces_clear_message() { + use {fs::Permissions, std::os::unix::fs::PermissionsExt}; + Test::new() + .justfile( + " + set working-directory := 'unusable' + default: + pwd + ", + ) + .tree(tree! { + unusable: {} + }) + .shell(false) + .args(["--shell", "NOT_A_REAL_SHELL"]) + .chmod("unusable", Permissions::from_mode(0o000)) + .status(1) + .stderr_regex(".*Failed to run recipe `default`:\n Failed to run shell `NOT_A_REAL_SHELL`:\n .*\n Failed to set working directory to `.*/unusable`.*") + .run(); +} + +#[test] +fn working_directory_is_not_a_directory_and_missing_shell_produces_clear_message() { + Test::new() + .justfile( + " + set working-directory := 'unusable' + default: + pwd + ", ) + .tree(tree! { + unusable: "is not a directory" + }) + .shell(false) + .args(["--shell", "NOT_A_REAL_SHELL"]) + .status(1) + .stderr_regex(".*Failed to run recipe `default`:\n Failed to run shell `NOT_A_REAL_SHELL`:\n .*\n Failed to set working directory to `.*/unusable`.*") .run(); } From fd14aecd21832880422d44eb5495daf31b121f28 Mon Sep 17 00:00:00 2001 From: Artem Baguinski Date: Tue, 3 Sep 2024 05:07:19 +0000 Subject: [PATCH 5/6] Fix a lint: unnecessary borrow --- src/recipe.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/recipe.rs b/src/recipe.rs index a78b1ec101..ef740871f9 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -306,7 +306,7 @@ impl<'src, D> Recipe<'src, D> { Err(io_error) => { let working_directory = self.working_directory(context); let working_directory_io_error = match &working_directory { - Some(working_directory) => match fs::read_dir(&working_directory) { + Some(working_directory) => match fs::read_dir(working_directory) { Ok(_) => None, Err(io_error) => Some(io_error), }, From 064a50533041127a594646a6aecac57151f5de58 Mon Sep 17 00:00:00 2001 From: Artem Baguinski Date: Tue, 3 Sep 2024 05:08:01 +0000 Subject: [PATCH 6/6] Only provide Test.chmod on unix, where it works --- tests/test.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test.rs b/tests/test.rs index 4428109349..d33feb5f2c 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -99,6 +99,7 @@ impl Test { self } + #[cfg(unix)] pub(crate) fn chmod(self, path: impl AsRef, perm: Permissions) -> Self { fs::set_permissions(self.tempdir.path().join(path.as_ref()), perm).unwrap(); self