Repository: casey/just
Pull request #1470 (2023-01-02 06:29:48)

Support including a justfile from another justfile
This PR adds support for including the contents of another justfile verbatim, using the syntax `!include <path-to-file>`. There is also some basic checking that justfiles cannot recursively include an already-processed justfile.

Should close https://github.com/casey/just/issues/1407



neunenak: This is ready for a review @casey 
neunenak: It looks like the test I added for this is failing in the macos-latest CI run, but not on my machine. With the debug print commit I added, I can see the output:

```

---- loader::tests::recursive_includes_fail stdout ----
Perform load on /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/justfile, seen_paths {}
Perform load on /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/subdir/justfile_b, seen_paths {"/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/justfile"}
Perform load on /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/justfile, seen_paths {"/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/subdir/justfile_b", "/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/justfile"}
thread 'loader::tests::recursive_includes_fail' panicked at 'assertion failed: (left ~= right)
  left: `IncludeRecursive { cur_path: "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/justfile", recursively_included_path: "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/temptree7vv8s5/subdir/justfile_b" }`
 right: `Error::IncludeRecursive { cur_path, recursively_included_path } if
cur_path == tmp.path().join("subdir").join("justfile_b") &&
    recursively_included_path == tmp.path().join("justfile")`', src/loader.rs:206:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    loader::tests::recursive_includes_fail

test result: FAILED. 461 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.17s
```

So there's some discrepency between `/var` and `/private/var` with the way that I'm creating tempfiles in the test. @casey any suggestions here?
casey: My guess is that it's because on macos, $TEMPDIR is a symlink, and if you canonicalize a path that includes a symlink, the path will change, so you might not get the output that you're expecting.

I thought of a potential issue here. Strings allow newlines, so it's possible for an include to be processed inside of a string. For example:

```
foo :="
!include bar
"
```

This is unlikely, but we should avoid it. We could only allow includes if there have been any non-blank, non-comment lines earlier in the file.
neunenak: > My guess is that it's because on macos, $TEMPDIR is a symlink, and if you canonicalize a path that includes a symlink, the path will change, so you might not get the output that you're expecting.

Ah, okay. So if I make sure to call `.canonicalize()` on both code paths, that should solve the OS X test issue I think.
 
> I thought of a potential issue here. Strings allow newlines, so it's possible for an include to be processed inside of a string.

Huh, that's a good point. The best solution is probably to actually do parsing on the justfile as written before processing includes, but that would require significant redesign. Maybe it's better to leave that for modules. For now I think disallowing includes after the first non-blank, non-comment line is fine.

casey: > Ah, okay. So if I make sure to call .canonicalize() on both code paths, that should solve the OS X test issue I think.

Not sure if it's possible, If you can avoid canonicalize, and just make the test more forgiving, maybe with a regex, that would probably be better. Canonicalized paths wind up being rather weird. I believe on windows especially, where I think it converts them to UNC paths, which strike most users as odd. 

> Huh, that's a good point. The best solution is probably to actually do parsing on the justfile as written before processing includes, but that would require significant redesign.

Definitely. I started working on a version that did just that, but it was tricky so I never finished it.
neunenak: > Not sure if it's possible, If you can avoid canonicalize, and just make the test more forgiving, maybe with a regex, that would probably be better. Canonicalized paths wind up being rather weird. I believe on windows especially, where I think it converts them to UNC paths, which strike most users as odd.

The reason I wanted to call `canonicalize()` is because otherwise it would be possible to include the same justfile using two different paths to identify it, which would wind up being different `PathBuf`s in the hashmap and wouldn't test positive for overlap. E.g. if we have:

```
justfile_a -> !include ./subdir/justfile_b
subdir/justfile_b -> !include ./subdir2/justfile_c
subdir/subdir2/justfile_c -> !include ../../justfile_a
```

we'd have an infinite circular dependency but all of the actual paths involved are different. 

casey: You could use the `Path::lexiclean` function as an alternative to `canonicalize`. It cleans paths lexically, i.e., only by looking at their contents, and doesn't look at symlinks on disk. So `assert_eq!(Path::new("subdir/subdir2/../../justfile_a").lexiclean(), Path::new("justfile_a"))`.
neunenak: > You could use the `Path::lexiclean` function as an alternative to `canonicalize`. It cleans paths lexically, i.e., only by looking at their contents, and doesn't look at symlinks on disk. For example:
> 
> ```
> Path::new("subdir/subdir2/../../justfile_a").lexiclean() == Path::new("justfile_a")
> ```

That's a handy crate to know that I wasn't previously aware of. Modified this PR to use that! @casey 
kbknapp: A quick note, I saw this comment in #1420 talking about this PR:

>  It's loosely based off of this one, but I had to make the code a bit more complicated to handle recursive justfile inclusion.

In our private fork of Just that added this include feature we found recursive inclusions to be very error prone and confusing for the users due to how recipe overrides work (and the lack of variable overrides). Which is why we ultimately opted for non-recursive includes which helped a ton.

Just my 2 cents.
casey: @kbknapp Thanks for the color! Can you elaborate a little bit about what was confusing about recursive includes? I'm not sure I understand.
kbknapp: (note the issues we experienced could have been helped if we'd also implemented specific error messages, etc. but we didn't do that for expediency. Also keep in mind we were/are using Just in a large monorepo which means the _users_ of the included recipes may not have been the original authors).

The two issues we ran into frequently were "variable already declared error" (since we didn't implement overriding variables), or finding which version of an overridden recipe was actually being run.

In either case (recursive justfiles or non-recursive) we found you'd have to manually look at (or `cat` all the justfiles into a single large file) to find out in what order things were included and then manually/mentally walk through the file to figure out where the variable was declared, or recipe was being overridden.

With recursive justfiles this lead us to have small building block style include files, so we ended up with tons of small justfiles that would be combined into intermediate justfiles that were effectively _only_ includes built-up of all these building block style files. The final "leaf" justfile that people actually ran would then also include many of these intermediate include justfiles along with some additional building block justfiles and override any recipes necessary. 

While this was a very satisfying outcome, and if you knew what was happening it was pretty magical, it also confused anyone who didn't really dig into the justfiles. Or anyone who only needed to add a new recipe for their team without really worrying about all the included files. So that debugging process I described above was a ton more work and many users got frustrated because they missed an include somewhere, or because imagine you have `#include "variables.just"` so the user didn't actually look at `variables.just` because they're dealing with a recipe override, but it turned out `variables.just` contained `#include "recipes.just"`.

It was also easy to end up in an include cycle (mostly due to how we these small building blocks set up, but in our defense recursive strategies beg for that style of set up 😅)

Of course these issues could be attributed to a combination of poor naming, or bad practices but it's still things we saw regularly. 

Once we went with non-recursive justfiles those issues mostly disappeared. If you saw `#include "foo.just"` could be sure it wasn't itself including anything else you'd have to trace down.

This meant we had to do away with the building-block style justfiles, but in the end the system is much easier to grok. If you're debugging a justfile and you don't see `#include "foo.just"` then you can be sure `foo.just` isn't involved. Cycles are also impossible.

Again, just our anecdote which is a relatively small sample size all things considered.
neunenak: > (note the issues we experienced could have been helped if we'd also implemented specific error messages, etc. but we didn't do that for expediency. Also keep in mind we were/are using Just in a large monorepo which means the _users_ of the included recipes may not have been the original authors).
> 
> The two issues we ran into frequently were "variable already declared error" (since we didn't implement overriding variables), or finding which version of an overridden recipe was actually being run.
> 
> In either case (recursive justfiles or non-recursive) we found you'd have to manually look at (or `cat` all the justfiles into a single large file) to find out in what order things were included and then manually/mentally walk through the file to figure out where the variable was declared, or recipe was being overridden.
> 
> With recursive justfiles this lead us to have small building block style include files, so we ended up with tons of small justfiles that would be combined into intermediate justfiles that were effectively _only_ includes built-up of all these building block style files. The final "leaf" justfile that people actually ran would then also include many of these intermediate include justfiles along with some additional building block justfiles and override any recipes necessary.
> 
> While this was a very satisfying outcome, and if you knew what was happening it was pretty magical, it also confused anyone who didn't really dig into the justfiles. Or anyone who only needed to add a new recipe for their team without really worrying about all the included files. So that debugging process I described above was a ton more work and many users got frustrated because they missed an include somewhere, or because imagine you have `#include "variables.just"` so the user didn't actually look at `variables.just` because they're dealing with a recipe override, but it turned out `variables.just` contained `#include "recipes.just"`.
> 
> It was also easy to end up in an include cycle (mostly due to how we these small building blocks set up, but in our defense recursive strategies beg for that style of set up sweat_smile)
> 
> Of course these issues could be attributed to a combination of poor naming, or bad practices but it's still things we saw regularly.
> 
> Once we went with non-recursive justfiles those issues mostly disappeared. If you saw `#include "foo.just"` could be sure it wasn't itself including anything else you'd have to trace down.
> 
> This meant we had to do away with the building-block style justfiles, but in the end the system is much easier to grok. If you're debugging a justfile and you don't see `#include "foo.just"` then you can be sure `foo.just` isn't involved. Cycles are also impossible.
> 
> Again, just our anecdote which is a relatively small sample size all things considered.

So what would happen if justfile_a includes justfile_b where justfile_b has its own include statements? Would they just be completely ignored by justfile_a? 
casey: @kbknapp Gotcha, thanks for all the detail! 

I can think of a few ways to address this issues. One is cycle detection, so if you recursively include the same justfile multiple times, you'll get an error.

Another is to add the ability to dump the final justfile, which would include the paths of what was included.

You might have:

justfile:
```just
!include bar.just

foo:
  echo foo
```

bar.just:
```just
bar:
  echo bar
```

And when you do `just --dump` you would get:

```just
# <bar.just>

bar:
  echo bar

# </bar.just>

foo:
  echo foo
```

Do you think that that would be enough to make dealing with recursive justfiles tractable?
neunenak: Since it seems like there's still some discussion to be had about exactly how `!include` ought to work, I've added a commit that gates it behind an unstable option, so the exact behavior can be tweaked if necessary.
kbknapp: > Do you think that that would be enough to make dealing with recursive justfiles tractable?

Those would definitely help a ton and I'd be happy to use them! I'd still think making includes recursive opens the complexity door a little too wide too early due to these question. Making the errors reflect any includes that are happening, and adding variable overrides would also help in these scenarios too.

Additionally, you can add recursion to includes in a backwards compatible manner, but you can't remove it in a backwards compatible manner.

Either way though, I'd be happy for the feature 😄 
casey: > Additionally, you can add recursion to includes in a backwards compatible manner, but you can't remove it in a backwards compatible manner.

We can always stabilize single-level includes, but leave recursive includes unstable.

@neunenak Let me know if this is ready to review.
neunenak: @casey ready for review but I am baffled as to why the generate-completions script is failing in the linter CI, it works on my machine fine.
neunenak: > When you run `just generate-completions` locally, does it produce a diff? That CI job complains if generate-completions generates a diff.

Ah, yes, it does. So okay that's the problem, I guess this is because I added a new option. Not sure what the right way to fix this is immediately.
neunenak: Or do I just need to commit the diff?
neunenak: Okay that was the problem. Ready for review @casey 
casey: Actually, thinking more about it, I no longer think using a regex is a good idea.

Instead I think we should do something like this:

```
for line in file.lines() {
  if line.starts_with('!') {
    // process directive
  } else if line.starts_with('#') {
    // comment_line
  } else if line.trim().is_empty { // or equivalent
    // blank
  } else {
    // stop processing lines
    break;
  }
}
```

Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index e0d8adea..4a746192 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -30,9 +30,6 @@ jobs:
 
     - uses: Swatinem/rust-cache@v1
 
-    - name: Check Lockfile
-      run: cargo update --locked --package just
-
     - name: Clippy
       run: cargo clippy --all --all-targets
 
diff --git a/README.md b/README.md
index e99aff00..18bae44f 100644
--- a/README.md
+++ b/README.md
@@ -2175,6 +2175,47 @@ But they must match:
 $ just foo/a bar/b
 error: Conflicting path arguments: `foo/` and `bar/`
 ```
+### Include Directives
+
+The `!include` directive, currently unstable, can be used to include the
+verbatim text of another file.
+
+If you have the following `justfile`:
+
+```mf
+!include foo/bar.just
+
+a: b
+  @echo A
+
+```
+
+And the following text in `foo/bar.just`:
+
+```mf
+b:
+  @echo B
+```
+
+`foo/bar.just` will be included in `justfile` and recipe `b` will be defined:
+
+```sh
+$ just --unstable b
+B
+$ just --unstable a
+B
+A
+```
+
+The `!include` directive path can be absolute or relative to the location of
+the justfile containing it. `!include` directives must appear at the beginning
+of a line. line.
+
+`!include` directives are only processed before the first non-blank,
+non-comment line.
+
+Included files can themselves contain `!include` directives, which are
+processed recursively.
 
 ### Hiding `justfile`s
 
diff --git a/src/error.rs b/src/error.rs
index 15e42b94..54bebc8c 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -31,6 +31,10 @@ pub(crate) enum Error<'src> {
     chooser: OsString,
     io_error: io::Error,
   },
+  CircularInclude {
+    current: PathBuf,
+    include: PathBuf,
+  },
   Code {
     recipe: &'src str,
     line_number: Option<usize>,
@@ -84,12 +88,19 @@ pub(crate) enum Error<'src> {
     function: Name<'src>,
     message: String,
   },
+  IncludeMissingPath {
+    file: PathBuf,
+    line: usize,
+  },
   InitExists {
     justfile: PathBuf,
   },
   Internal {
     message: String,
   },
+  InvalidDirective {
+    line: String,
+  },
   Io {
     recipe: &'src str,
     io_error: io::Error,
@@ -330,6 +341,12 @@ impl<'src> ColorDisplay for Error<'src> {
           io_error
         )?;
       }
+      CircularInclude { current, include } => {
+        write!(
+          f,
+          "Include `{}` in `{}` is a circular include", include.display(), current.display()
+        )?;
+      },
       Code {
         recipe,
         line_number,
@@ -482,6 +499,18 @@ impl<'src> ColorDisplay for Error<'src> {
           message
         )?;
       }
+      IncludeMissingPath {
+        file: justfile, line
+      } => {
+
+        write!(
+          f,
+          "!include directive on line {} of `{}` has no argument",
+          line.ordinal(),
+          justfile.display(),
+        )?;
+
+      },
       InitExists { justfile } => {
         write!(f, "Justfile `{}` already exists", justfile.display())?;
       }
@@ -493,6 +522,9 @@ impl<'src> ColorDisplay for Error<'src> {
           message
         )?;
       }
+      InvalidDirective { line } => {
+        write!(f, "Invalid directive: {line}")?;
+      }
       Io { recipe, io_error } => {
         match io_error.kind() {
           io::ErrorKind::NotFound => write!(
diff --git a/src/loader.rs b/src/loader.rs
index 95fa7c34..7ea5612e 100644
--- a/src/loader.rs
+++ b/src/loader.rs
@@ -1,21 +1,216 @@
 use super::*;
+use std::collections::HashSet;
+
+struct LinesWithEndings<'a> {
+  input: &'a str,
+}
+
+impl<'a> LinesWithEndings<'a> {
+  fn new(input: &'a str) -> Self {
+    Self { input }
+  }
+}
+
+impl<'a> Iterator for LinesWithEndings<'a> {
+  type Item = &'a str;
+
+  fn next(&mut self) -> Option<&'a str> {
+    if self.input.is_empty() {
+      return None;
+    }
+    let split = self.input.find('\n').map_or(self.input.len(), |i| i + 1);
+    let (line, rest) = self.input.split_at(split);
+    self.input = rest;
+    Some(line)
+  }
+}
 
 pub(crate) struct Loader {
   arena: Arena<String>,
+  unstable: bool,
 }
 
 impl Loader {
-  pub(crate) fn new() -> Self {
+  pub(crate) fn new(unstable: bool) -> Self {
     Loader {
       arena: Arena::new(),
+      unstable,
     }
   }
 
   pub(crate) fn load<'src>(&'src self, path: &Path) -> RunResult<&'src str> {
-    let src = fs::read_to_string(path).map_err(|io_error| Error::Load {
+    let src = self.load_recursive(path, HashSet::new())?;
+    Ok(self.arena.alloc(src))
+  }
+
+  fn load_file<'a>(path: &Path) -> RunResult<'a, String> {
+    fs::read_to_string(path).map_err(|io_error| Error::Load {
       path: path.to_owned(),
       io_error,
-    })?;
-    Ok(self.arena.alloc(src))
+    })
+  }
+
+  fn load_recursive(&self, file: &Path, seen: HashSet<PathBuf>) -> RunResult<String> {
+    let src = Self::load_file(file)?;
+
+    let mut output = String::new();
+
+    let mut seen_content = false;
+
+    for (i, line) in LinesWithEndings::new(&src).enumerate() {
+      if !seen_content && line.starts_with('!') {
+        let include = line
+          .strip_prefix("!include")
+          .ok_or_else(|| Error::InvalidDirective { line: line.into() })?;
+
+        if !self.unstable {
+          return Err(Error::Unstable {
+            message: "The !include directive is currently unstable.".into(),
+          });
+        }
+
+        let argument = include.trim();
+
+        if argument.is_empty() {
+          return Err(Error::IncludeMissingPath {
+            file: file.to_owned(),
+            line: i,
+          });
+        }
+
+        let contents = self.process_include(file, Path::new(argument), &seen)?;
+
+        output.push_str(&contents);
+      } else {
+        if !(line.trim().is_empty() || line.trim().starts_with('#')) {
+          seen_content = true;
+        }
+        output.push_str(line);
+      }
+    }
+
+    Ok(output)
+  }
+
+  fn process_include(
+    &self,
+    file: &Path,
+    include: &Path,
+    seen: &HashSet<PathBuf>,
+  ) -> RunResult<String> {
+    let canonical_path = if include.is_relative() {
+      let current_dir = file.parent().ok_or(Error::Internal {
+        message: format!(
+          "Justfile path `{}` has no parent directory",
+          include.display()
+        ),
+      })?;
+      current_dir.join(include)
+    } else {
+      include.to_owned()
+    };
+
+    let canonical_path = canonical_path.lexiclean();
+
+    if seen.contains(&canonical_path) {
+      return Err(Error::CircularInclude {
+        current: file.to_owned(),
+        include: canonical_path,
+      });
+    }
+
+    let mut seen_paths = seen.clone();
+    seen_paths.insert(file.lexiclean());
+
+    self.load_recursive(&canonical_path, seen_paths)
+  }
+}
+
+#[cfg(test)]
+mod tests {
+  use super::{Error, Lexiclean, Loader};
+  use temptree::temptree;
+
+  #[test]
+  fn include_justfile() {
+    let justfile_a = r#"
+# A comment at the top of the file
+!include ./justfile_b
+
+some_recipe: recipe_b
+    echo "some recipe"
+"#;
+
+    let justfile_b = r#"!include ./subdir/justfile_c
+
+recipe_b: recipe_c
+    echo "recipe b"
+"#;
+
+    let justfile_c = r#"recipe_c:
+    echo "recipe c"
+"#;
+
+    let tmp = temptree! {
+        justfile: justfile_a,
+        justfile_b: justfile_b,
+        subdir: {
+            justfile_c: justfile_c
+        }
+    };
+
+    let full_concatenated_output = r#"
+# A comment at the top of the file
+recipe_c:
+    echo "recipe c"
+
+recipe_b: recipe_c
+    echo "recipe b"
+
+some_recipe: recipe_b
+    echo "some recipe"
+"#;
+
+    let loader = Loader::new(true);
+
+    let justfile_a_path = tmp.path().join("justfile");
+    let loader_output = loader.load(&justfile_a_path).unwrap();
+
+    assert_eq!(loader_output, full_concatenated_output);
+  }
+
+  #[test]
+  fn recursive_includes_fail() {
+    let justfile_a = r#"
+# A comment at the top of the file
+!include ./subdir/justfile_b
+
+some_recipe: recipe_b
+    echo "some recipe"
+
+"#;
+
+    let justfile_b = r#"
+!include ../justfile
+
+recipe_b:
+    echo "recipe b"
+"#;
+    let tmp = temptree! {
+        justfile: justfile_a,
+        subdir: {
+            justfile_b: justfile_b
+        }
+    };
+
+    let loader = Loader::new(true);
+
+    let justfile_a_path = tmp.path().join("justfile");
+    let loader_output = loader.load(&justfile_a_path).unwrap_err();
+
+    assert_matches!(loader_output, Error::CircularInclude { current, include }
+        if current == tmp.path().join("subdir").join("justfile_b").lexiclean() &&
+        include == tmp.path().join("justfile").lexiclean()
+    );
   }
 }
diff --git a/src/run.rs b/src/run.rs
index 950fa0bd..a1a18c24 100644
--- a/src/run.rs
+++ b/src/run.rs
@@ -17,14 +17,14 @@ pub fn run() -> Result<(), i32> {
   info!("Parsing command line arguments…");
   let matches = app.get_matches();
 
-  let loader = Loader::new();
-
   let config = Config::from_matches(&matches).map_err(Error::from);
 
-  let (color, verbosity) = config
+  let (color, verbosity, unstable) = config
     .as_ref()
-    .map(|config| (config.color, config.verbosity))
-    .unwrap_or((Color::auto(), Verbosity::default()));
+    .map(|config| (config.color, config.verbosity, config.unstable))
+    .unwrap_or((Color::auto(), Verbosity::default(), false));
+
+  let loader = Loader::new(unstable);
 
   config
     .and_then(|config| config.run(&loader))


Test patch:
diff --git a/tests/choose.rs b/tests/choose.rs
index d89c31bc..b7e9f442 100644
--- a/tests/choose.rs
+++ b/tests/choose.rs
@@ -125,7 +125,7 @@ fn invoke_error_function() {
           echo bar
       ",
     )
-    .stderr_regex("error: Chooser `/ -cu fzf` invocation failed: .*")
+    .stderr_regex("error: Chooser `/ -cu fzf` invocation failed: .*\n")
     .status(EXIT_FAILURE)
     .shell(false)
     .args(["--shell", "/", "--choose"])
diff --git a/tests/error_messages.rs b/tests/error_messages.rs
index 8e5fb36a..e17a4afd 100644
--- a/tests/error_messages.rs
+++ b/tests/error_messages.rs
@@ -26,11 +26,11 @@ test! {
 
 test! {
   name: unexpected_character,
-  justfile: "!~",
+  justfile: "&~",
   stderr: "
-    error: Expected character `=`
+    error: Expected character `&`
       |
-    1 | !~
+    1 | &~
       |  ^
   ",
   status: EXIT_FAILURE,
diff --git a/tests/includes.rs b/tests/includes.rs
new file mode 100644
index 00000000..8850b9b6
--- /dev/null
+++ b/tests/includes.rs
@@ -0,0 +1,91 @@
+use super::*;
+
+#[test]
+fn include_fails_without_unstable() {
+  Test::new()
+    .justfile("!include ./include.justfile")
+    .status(EXIT_FAILURE)
+    .stderr("error: The !include directive is currently unstable. Invoke `just` with the `--unstable` flag to enable unstable features.\n")
+    .run();
+}
+
+#[test]
+fn include_succeeds_with_unstable() {
+  Test::new()
+    .tree(tree! {
+      "include.justfile": "
+        b:
+          @echo B
+      ",
+    })
+    .justfile(
+      "
+        !include ./include.justfile
+
+        a: b
+          @echo A
+      ",
+    )
+    .arg("--unstable")
+    .test_round_trip(false)
+    .arg("a")
+    .stdout("B\nA\n")
+    .run();
+}
+
+#[test]
+fn trailing_spaces_after_include_are_ignored() {
+  Test::new()
+    .tree(tree! {
+      "include.justfile": "
+        a:
+          @echo A
+      ",
+    })
+    .justfile("!include ./include.justfile\x20")
+    .arg("--unstable")
+    .test_round_trip(false)
+    .stdout("A\n")
+    .run();
+}
+
+#[test]
+fn include_directive_with_no_path() {
+  Test::new()
+    .justfile("!include")
+    .arg("--unstable")
+    .status(EXIT_FAILURE)
+    .stderr_regex("error: !include directive on line 1 of `.*` has no argument\n")
+    .run();
+}
+
+#[test]
+fn trailing_include() {
+  Test::new()
+    .justfile(
+      "
+      b:
+      !include ./include.justfile
+      ",
+    )
+    .arg("--unstable")
+    .status(EXIT_FAILURE)
+    .stderr("error: Expected character `=`\n  |\n2 | !include ./include.justfile\n  |  ^\n")
+    .run();
+}
+
+#[test]
+fn circular_include() {
+  Test::new()
+    .justfile("!include a")
+    .tree(tree! {
+      a: "!include b",
+      b: "!include a",
+    })
+    .arg("--unstable")
+    .status(EXIT_FAILURE)
+    .stderr_regex(path_for_regex(
+      "error: Include `.*/a` in `.*/b` is a circular include\n",
+    ))
+    .run();
+}
diff --git a/tests/lib.rs b/tests/lib.rs
index 29ddf2cc..6dbc733e 100644
--- a/tests/lib.rs
+++ b/tests/lib.rs
@@ -51,6 +51,7 @@ mod fallback;
 mod fmt;
 mod functions;
 mod ignore_comments;
+mod includes;
 mod init;
 #[cfg(unix)]
 mod interrupts;
@@ -93,3 +94,11 @@ fn path(s: &str) -> String {
     s.into()
   }
 }
+
+fn path_for_regex(s: &str) -> String {
+  if cfg!(windows) {
+    s.replace('/', "\\\\")
+  } else {
+    s.into()
+  }
+}
diff --git a/tests/test.rs b/tests/test.rs
index 62bc0895..8271074f 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -47,6 +47,7 @@ pub(crate) struct Test {
   pub(crate) stdout: String,
   pub(crate) stdout_regex: Option<Regex>,
   pub(crate) tempdir: TempDir,
+  pub(crate) test_round_trip: bool,
   pub(crate) unindent_stdout: bool,
 }
 
@@ -69,6 +70,7 @@ impl Test {
       stdout: String::new(),
       stdout_regex: None,
       tempdir,
+      test_round_trip: true,
       unindent_stdout: true,
     }
   }
@@ -125,7 +127,7 @@ impl Test {
   }
 
   pub(crate) fn stderr_regex(mut self, stderr_regex: impl AsRef<str>) -> Self {
-    self.stderr_regex = Some(Regex::new(&format!("(?m)^{}$", stderr_regex.as_ref())).unwrap());
+    self.stderr_regex = Some(Regex::new(&format!("^{}$", stderr_regex.as_ref())).unwrap());
     self
   }
 
@@ -140,7 +142,12 @@ impl Test {
   }
 
   pub(crate) fn stdout_regex(mut self, stdout_regex: impl AsRef<str>) -> Self {
-    self.stdout_regex = Some(Regex::new(&format!("(?m)^{}$", stdout_regex.as_ref())).unwrap());
+    self.stdout_regex = Some(Regex::new(&format!("^{}$", stdout_regex.as_ref())).unwrap());
+    self
+  }
+
+  pub(crate) fn test_round_trip(mut self, test_round_trip: bool) -> Self {
+    self.test_round_trip = test_round_trip;
     self
   }
 
@@ -245,7 +252,7 @@ impl Test {
       panic!("Output mismatch.");
     }
 
-    if self.status == EXIT_SUCCESS {
+    if self.test_round_trip && self.status == EXIT_SUCCESS {
       test_round_trip(self.tempdir.path());
     }