allow tempdir configuration (#1300) #1369

dmatos2012 · 2022-10-17T18:00:57Z

Hi,

This PR addresses issue #1300 to allow configuring tempdir as an additional setting.

I am fairly new at Rust, and wanted to tackle this issue as it seemed like a good challenge. Please let me know if something is just completely wrong, and I will gladly try to change it to get it right. But as far as I could see, it does what it intends on doing. Most of the changes are just to make the other tests happy again.

Looking forward to it! And thanks for this awesome tool!

casey · 2022-10-18T02:03:05Z

Thanks for taking a crack at this! This should have tests. One suggestion is that the script could get the name of the script, extract the directory, write a file to that directory, and then after the tests assert that the file was created in the right location. The justfile for such a test will look something like this:

set tempdir = "."

foo:
  #!/usr/bin/env bash
  touch `dirname $0`/foo

dmatos2012 · 2022-10-18T20:49:31Z

I took your suggestion and I believe the two tests in tests/misc.rs should address what you have suggested. I was not sure exactly where to put the tests, so I can move them if you find them a more appropriate place, or maybe even its own temp_dir.rs test file.

Also not sure if these tests should have specific linux #[cfg] directives since I am not sure those will pass on windows(given use of linux only commands), but cant test that myself.

Let me know your thoughts about the new two tests. Thanks again for your time !

casey

Bunch of comments! Check 'em out. I think tempdir (no dash) is better than temp_dir.

casey · 2022-10-20T00:42:14Z

src/analyzer.rs

+        Setting::Tempdir(path) => {
+          settings.temp_dir = Some(path);


Suggested change

Setting::Tempdir(path) => {

settings.temp_dir = Some(path);

Setting::Tempdir(tempdir) => {

settings.tempdir = Some(tempdir);

casey · 2022-10-20T00:42:41Z

src/parser.rs

@@ -786,13 +786,24 @@ impl<'tokens, 'src> Parser<'tokens, 'src> {
        value: Setting::WindowsShell(self.parse_shell()?),
        name,
      })
+    } else if name.lexeme() == Keyword::Tempdir.lexeme() {
+      Ok(Set {
+        value: Setting::Tempdir(self.parse_path()?),


I think this can just be inlined, also the value of Seting::Tempdir can be a String:

Suggested change

value: Setting::Tempdir(self.parse_path()?),

value: Setting::Tempdir(self.parse_string_literal()?.cooked),

casey · 2022-10-20T00:44:43Z

src/parser.rs

+  // Parse tempdir setting value
+  fn parse_path(&mut self) -> CompileResult<'src, StringLiteral<'src>> {
+    let path = self.parse_string_literal()?;
+    Ok(path)
+  }
+


Suggested change

// Parse tempdir setting value

fn parse_path(&mut self) -> CompileResult<'src, StringLiteral<'src>> {

let path = self.parse_string_literal()?;

Ok(path)

}

casey · 2022-10-20T00:48:36Z

src/recipe.rs

+    let tmp = if let Some(temp_dir) = &context.settings.temp_dir {
+      tempfile::Builder::new()
+        .tempdir_in(temp_dir.raw)
+        .map_err(|error| Error::TmpdirIo {
+          recipe: self.name(),
+          io_error: error,
+        })?
+    } else {
+      tempfile::Builder::new()
+        .prefix("just")
+        .tempdir()
+        .map_err(|error| Error::TmpdirIo {
+          recipe: self.name(),
+          io_error: error,
+        })?
+    };


Maybe something like this to deduplicate a little bit, and preserve the temporary directory prefix:

Suggested change

let tmp = if let Some(temp_dir) = &context.settings.temp_dir {

tempfile::Builder::new()

.tempdir_in(temp_dir.raw)

.map_err(|error| Error::TmpdirIo {

recipe: self.name(),

io_error: error,

})?

} else {

tempfile::Builder::new()

.prefix("just")

.tempdir()

.map_err(|error| Error::TmpdirIo {

recipe: self.name(),

io_error: error,

})?

};

let mut tempdir_builder = tempfile::Builder::new();

tempdir_builder.prefix("just");

let tempdir = match &context.settings.tempdir {

Some(tempdir) => tempdir_builder.tempdir_in(tempdir),

None => tempdir_builder.tempdir(),

}.map_err(|error| Error::TmpdirIo {

recipe: self.name(),

io_error: error,

})?;

casey · 2022-10-20T00:48:46Z

src/setting.rs

@@ -8,6 +8,7 @@ pub(crate) enum Setting<'src> {
  Export(bool),
  PositionalArguments(bool),
  Shell(Shell<'src>),
+  Tempdir(StringLiteral<'src>),


Suggested change

Tempdir(StringLiteral<'src>),

Tempdir(String),

casey · 2022-10-20T00:49:08Z

src/setting.rs

@@ -22,6 +23,7 @@ impl<'src> Display for Setting<'src> {
      | Setting::PositionalArguments(value)
      | Setting::WindowsPowerShell(value) => write!(f, "{}", value),
      Setting::Shell(shell) | Setting::WindowsShell(shell) => write!(f, "{}", shell),
+      Setting::Tempdir(path) => write!(f, "{}", path),


Suggested change

Setting::Tempdir(path) => write!(f, "{}", path),

Setting::Tempdir(tempdir) => write!(f, "{}", tempdir),

casey · 2022-10-20T00:49:18Z

src/settings.rs

@@ -13,6 +13,7 @@ pub(crate) struct Settings<'src> {
  pub(crate) ignore_comments: bool,
  pub(crate) positional_arguments: bool,
  pub(crate) shell: Option<Shell<'src>>,
+  pub(crate) temp_dir: Option<StringLiteral<'src>>,


Suggested change

pub(crate) temp_dir: Option<StringLiteral<'src>>,

pub(crate) tempdir: Option<String>,

casey · 2022-10-20T00:49:28Z

tests/json.rs

@@ -44,6 +44,7 @@ fn alias() {
        "export": false,
        "positional_arguments": false,
        "shell": null,
+        "temp_dir" : null,


Suggested change

"temp_dir" : null,

"tempdir" : null,

casey · 2022-10-20T00:57:35Z

tests/misc.rs

+test! {
+    name: custom_tempdir,
+    justfile: r#"
+    set tempdir := "."
+    parent_tempdir := "."
+    foo:
+        #!/usr/bin/env bash
+        script_tempdir=$(basename "$(dirname $0)")
+        script_foo=`realpath {{parent_tempdir}}`/$script_tempdir/foo
+        [[ -f $script_foo ]] && echo "foo_exists" || echo "foo_missing"
+    "#,
+    stdout: "foo_exists\n",
+}
+
+test! {
+    name: standard_tempdir,
+    justfile: r#"
+    standard_parent_tempdir := "/tmp"
+    foo:
+        #!/usr/bin/env bash
+        script_tempdir=$(basename "$(dirname $0)")
+        script_foo={{standard_parent_tempdir}}/$script_tempdir/foo
+        [[ -f $script_foo ]] && echo "foo_exists" || echo "foo_missing"
+    "#,
+    stdout: "foo_exists\n",
+}
+


A few comments:

There are still a bunch of macro-based tests, but I've been moving away from them. For new tests it's best to use the Test struct. Check out some of the other tests.

These tests can go in their own file, perhaps tests/tempdir.rs.

realpath isn't available on MacOS.

The test can be simplified with something like this:

This will only succeed if the temporary directory is the same directory as the current directory, which will only be true if the tempdir has been set:

set tempdir := "." foo: touch $(basename "$(dirname $0)")/bar cat bar

I think the standard_tempdir test can be removed.

casey · 2022-10-20T00:58:51Z

Regarding running these tests on windows, we should avoid using any unix-specific commands so they can run anywhere.

dmatos2012 · 2022-10-25T19:35:29Z

Thanks for the feedback on all the files. I have changed them accordingly and added the tests which now should address what you have mentioned, and used the Test struct rather than the json.

Thanks again for your time!

casey · 2022-10-25T23:59:00Z

Nice merged! I added a small change, which is to interpret the tempdir path relative to the working directory, not the invocation directory.

dmatos2012 · 2022-10-26T11:19:43Z

Thanks so much for the guidance and adding the small tweaks. Hope to keep contributing in the near future to this repo :)

casey · 2022-10-26T19:21:42Z

You are most welcome! Thank you for the PR!

dmatos2012 force-pushed the add-tempdir-setting branch from fb7e058 to b87e0a8 Compare October 18, 2022 20:43

casey requested changes Oct 20, 2022

View reviewed changes

dmatos2012 force-pushed the add-tempdir-setting branch from b87e0a8 to 95f67f7 Compare October 25, 2022 19:31

allow tempdir configuration (casey#1300)

08af5dd

dmatos2012 force-pushed the add-tempdir-setting branch from 95f67f7 to 08af5dd Compare October 25, 2022 19:32

casey added 6 commits October 25, 2022 16:27

tweak

6a0e4e4

tweak

a8b130a

Merge branch 'master' into add-tempdir-setting

328f130

Add tmpdir to simple json

c33e580

Add to readme and grammar

c434ed7

Interpret tempdir relative to the working directory

15b1cc8

casey merged commit bac3133 into casey:master Oct 25, 2022

casey pushed a commit that referenced this pull request Oct 25, 2022

Add tempdir setting (#1369)

beeaa6c

casey mentioned this pull request Oct 25, 2022

Allow configuring tempdir #1300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow tempdir configuration (#1300) #1369

allow tempdir configuration (#1300) #1369

dmatos2012 commented Oct 17, 2022

casey commented Oct 18, 2022

dmatos2012 commented Oct 18, 2022 •

edited

Loading

casey left a comment

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey Oct 20, 2022

casey commented Oct 20, 2022

dmatos2012 commented Oct 25, 2022

casey commented Oct 25, 2022

dmatos2012 commented Oct 26, 2022

casey commented Oct 26, 2022

	value: Setting::Tempdir(self.parse_path()?),
	value: Setting::Tempdir(self.parse_string_literal()?.cooked),

	Setting::Tempdir(path) => write!(f, "{}", path),
	Setting::Tempdir(tempdir) => write!(f, "{}", tempdir),

	pub(crate) temp_dir: Option<StringLiteral<'src>>,
	pub(crate) tempdir: Option<String>,

allow tempdir configuration (#1300) #1369

allow tempdir configuration (#1300) #1369

Conversation

dmatos2012 commented Oct 17, 2022

casey commented Oct 18, 2022

dmatos2012 commented Oct 18, 2022 • edited Loading

casey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casey commented Oct 20, 2022

dmatos2012 commented Oct 25, 2022

casey commented Oct 25, 2022

dmatos2012 commented Oct 26, 2022

casey commented Oct 26, 2022

dmatos2012 commented Oct 18, 2022 •

edited

Loading