Add basic support for directory targets

[snowleopard]

This PR implements a basic version of the RFC from #3316.

Basically, one can now write (target (dir output)) in a rule to specify a directory target. This functionality is experimental and is therefore guarded by the directory-targets extension. We need directory targets internally at Jane Street and will polish the feature as it gets some further use before making it officially supported in Dune.

See the test file for all the currently available features. For now, I only added a brief comment to the docs that this feature is now available for experimentation. Documentation will be expanded when we figure out the full story.

[cwong-ocaml]

This maybe doesn't need to block this PR, but are dynamic actions even used anywhere? A quick grep didn't find anything.

[snowleopard]

I don't think dynamic actions are currently used anywhere but in principle, it's a pretty cool feature. I'd be sad to delete the corresponding code even if it's currently unused.

[snowleopard]

Maybe @jeremiedimino can say something about the original motivation for dynamic actions -- are there any plans to start using this plugin at some point?

[rgrinberg]

I'd like to revive them soon personally. No plans before 3.0 though.

I'm just curious, what's the source of incompatibility? I can't quite see the issue.

[cwong-ocaml]

It may be too deeply-entrenched by now, but it seems friendlier to have directory targets imply this (with a corresponding error if it can't be sandboxed for some reason), rather than erroring out and forcing the user to write this explicitly.

[snowleopard]

I like this idea! I'll try to change the code to error out if the rule can't be sandboxed.

[snowleopard]

Implementing this turned out to be harder than it initially seemed. It's easy to make a directory target imply sandboxing but it's hard to later give a good error message about why there is no available sandboxing option. This requires to plumb information about the provenance of sandboxing restrictions, which seems like a lot of work. So, I'm leaving this as is for now.

[snowleopard]

I now think that in future it may be possible to support directory targets without sandboxing. I left a CR-someday.

[cwong-ocaml]

I think we should probably specify that output is a directory in this case somehow. The easy patch would be to say output/*, but maybe something like did you mean output (directory) or output_ (file) would be clearer? Not sure.

[snowleopard]

Good idea, I'll clarify that the hint refers to a target directory.

[snowleopard]

Addressing this led to a refactoring that seems better done separately. Leaving a CR-someday to follow up with another PR instead.

[rgrinberg]

IMO this convention isn't consistent with the rest of the dune language and I'd prefer if it was avoided. * is already used for globs, and this convention overloads that. Is there a reason why we can't use an explicit constructor like (dir foo) vs. foo?

[snowleopard]

I was also hesitating between (dir foo) and foo/*. We discussed this a bit with @jeremiedimino and one reason why foo/* seemed preferable is that it has good potential for generalisation, for example, to allow patterns like foo/*.txt or even *.txt i.e. a dynamic set of outputs produced in the very same directory. One could argue that all of these cases deserve separate dir-like subfields but then that's an extra thing to remember, whereas just using patterns seems quite natural.

[rgrinberg]

I see. Well we should discuss it more with other team members. @bobot what do you think?

I think your idea at the generalization exposes even more inconsistencies with this current syntax. We'll use foo/*.txt to define a dynamic set of outputs, but then write (glob_files foo/*.txt) to depend on it? That can't be right.

IMO, we should strive for symmetry and consistency between the following three things:

• Dependency specification
• Target specification
• Command line invocation

It's OK if we haven't figured out a completely scheme, but at that point we should just go for explicit (even if verbose) descriptions. If a better suggestion on how to generalize this all comes up, we'll still be able to implement it.

[snowleopard]

OK, I agree that the first iteration should use the simplest syntax, so I switched to (dir foo) for specifying targets. Note that this led to dropping support for printing rules because that got more complicated. We can do it later.

For dependency specification, the current implementation accepts just foo but I added a CR-someday to change this to (dir foo) in the next iteration.

For command line invocation, the current implementation accepts just foo and I'm not sure if we want to replace it with (dir foo) since that would be awkward.

If we want consistency, foo/* might work for all of the above but I leave it for future discussion.

[rgrinberg]

I'm excited to see try this feature out in 3.0. A couple of questions:

1. why did you make this feature experimental? Are there any aspects of the semantics you aren't confident about?

2. I didn't see a test for defining a rule inside directory targets. I assume this should be an error. Should we add a test case?

[snowleopard]

@rgrinberg Thanks for the review!

1. why did you make this feature experimental? Are there any aspects of the semantics you aren't confident about?

The main reason is that I'd like to push this PR through the review as soon as possible (we need this feature in Dune engine for some internal work) even though there are some design questions to discuss, such as using (dir output) vs output/* in the Dune language. By marking this feature as experimental, we are telling Dune users that this feature is still in development.

2. I didn't see a test for defining a rule inside directory targets. I assume this should be an error. Should we add a test case?

Good point, this should be an error. There is a CR-someday somewhere about detecting conflicts for directory targets. I can try addressing it in this PR but this can also be done separately.

[rgrinberg]

The main reason is that I'd like to push this PR through the review as soon as possible (we need this feature in Dune engine for some internal work) even though there are some design questions to discuss, such as using (dir output) vs output/* in the Dune language. By marking this feature as experimental, we are telling Dune users that this feature is still in development.

I see. Personally, I think that if we settle on the semantics then can make the feature as stable. If there's some bugs remaining or not everything is implemented, those can always be addressed later without breakage. I'm also itching to use this feature.

[snowleopard]

I see. Personally, I think that if we settle on the semantics then can make the feature as stable. If there's some bugs remaining or not everything is implemented, those can always be addressed later without breakage. I'm also itching to use this feature.

I switched to using (dir foo) for specifying directory targets and that resulted in a few more CR-somedays (see the comment above). I'd prefer not to delay merging this: we can resolve remaining design/implementation issues later and then drop the experimental status once we're happy. It's very easy to do that.

[rgrinberg]

Looks good.

I wouldn't worry too much about the generating the make rules. They haven't worked for a long time and directory targets in make have a different semantics anyway.

Are we going to have dependencies on directories that behave sensibly wrt to directory targets? E.g. I would expect this to work:

(rule
 (target (dir foo))
 ..)

(rule
 (deps foo)
 ..)

[snowleopard]

@rgrinberg Thanks for the review!

I wouldn't worry too much about the generating the make rules.

Generating Makefiles is easy. Its generating Dune files which is a bit tedious (at least I couldn't quickly figure out how to print out the set of targets in the right way).

Are we going to have dependencies on directories that behave sensibly wrt to directory targets? E.g. I would expect this to work:

(rule
 (target (dir foo))
 ..)

(rule
 (deps foo)
 ..)

This works exactly as you described -- see (deps output) in the test. You can also depend on individual files, e.g. there are also tests with dependencies like (deps output/subdir/b).

[snowleopard]

I managed to fix glob behaviour. Now %{deps} for (glob_files output/*) correctly expands to the contents of a directory target (previously it expanded just to the name of the directory which was just wrong).

Merging if CI looks OK. All tests pass for me locally.

[snowleopard]

./perf.sh seems to be happy:

             Current branch Main branch
             ============== ===========
           | real 5.684s    real 5.732s
Full build | user 29.892s   user 29.560s
           | sys  14.778s   sys  14.354s
                            
           | real 1.163s    real 1.113s
Zero build | user 1.059s    user 0.997s
           | sys  0.103s    sys  0.117s