Support including a justfile from another justfile

[neunenak]

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 #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 PathBufs 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.

We could maybe try to be less anal about detecting circular includes, but I think this is something where we should try particularly hard to have a good error message, because the "default" behavior is recursively including the same files until just crashes, which is a bad user experience.

[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. For example:

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:

!include bar.just

foo:
  echo foo

bar.just:

bar:
  echo bar

And when you do just --dump you would get:

# <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.

[casey]

When you run just generate-completions locally, does it produce a diff? That CI job complains if generate-completions generates a diff.

[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.

[MadBomber]

congratulations I will begin the process of deprecating the Justprep stopgap reprocessor.