feature: install dir

[rgrinberg]

This PR allows installing all the artifacts in a directory using the following syntax:

(install
 (dir foo/bar)
 (package pkg)
 (section baz))

This will build foo/bar and add its recursive listing to pkg.install.

This feature is particularly useful for installing things such as documentation, where there's a lot of overhead in listing the contents individually.

[rgrinberg]

This is ready now. It took a lot longer than I ever expected.

@staronj could you review you the minimal build system changes? It's just:

• Putting files and directories on an equal footing wrt to naming, digests
• Allowing non sandboxed actions to produce directory targets if the actions don't need to be sandboxed (e.g. symlinking)

@emillon could you review the rules side of things?

[snowleopard]

I think there is a bug in the current implementation, where we watch directories via their parents in Fs_memo.path_digest. It's tempting to treat files and directories using the same functions but I think we should resist the temptation and handle them separately.

We have a generic Fs_memo.path_stat but that's for a good reason: we use it to figure out if something is a file or a directory. Everything else in Fs_memo's API, I think, should have either file_ or dir_ prefix.

[rgrinberg]

I ended up reverting my file -> changes. They weren't even needed, and probably made things more confusing. I still kept the Fs_cache.Untracked.path_digest because I think it better reflects what we're installing in that cache. For better or for worse, we currently store file/directory in one table, so I think it's best if the name would reflect that.

I changed my mind and it made the code even more confusing.

It's not like our x in 2x is very large: it's like 2 or maybe 3 at a stretch? In some cases dir_ functions have no file_ counterpart, so we'd end up with some inconsistent API anyway.

I tried doing that, but it wasn't quite so easy. Some issues:

• The fs cache tables do not allow you to assert the type of path. In other words, Fs_cache.{read,evict} should accept a File | Directory argument or have separate functions for paths and files.
• There's lots of other places in the codebase where we assume things are files but we should accept directories as well (See Dep.Facts.t for example). So the number of functions that need to be changed is more than 2 or 3.

So I elected to just maintain the status quo and do a more complete and principled refactoring in a subsequent PR. In particular, I'd rather try merging the Cached_digest and Fs_cache tables first. So that we have a single place in the codebase where we can verify that whenever we ask for a {file,directory}, we actually get what we asked for.

[snowleopard]

@rgrinberg I agree that improving Fs_memo should be done separately from this PR. Please keep in mind that we use Fs_memo internally to implement Jenga rules on top of the Dune engine, so any breaking changes, particularly with respect to performance or semantics, will need a discussion. I'd rather we step carefully, since a lot of effort went into making Dune work for our whole monorepo. If that's not a big trouble for you, I'd prefer you split your improvements into small PRs that can be discussed and applied (as well as tested internally) in isolation.

[snowleopard]

Can this PR be split into smaller steps, perhaps? As I say elsewhere in a comment, I'm still super confused about what this is all about. Adding a few new conditionals into the heart of build_system.ml that deals with action execution doesn't seem great either: could some of that be factored out into a separate module/functions?

[rgrinberg]

It's possible to undo the changes in build_system if we sandbox the install rules. The install rules are all symlinking/copying, so this will not affect anything.

[rgrinberg]

It's possible to undo the changes in build_system if we sandbox the install rules. The install rules are all symlinking/copying, so this will not affect anything.

Actually, this turns out to be not possible. We get bit by the following check:

          match Action.is_useful_to_sandbox action.action with
          | Clearly_not ->
            if Sandbox_config.mem action.sandbox Sandbox_mode.none then
              Sandbox_mode.none
            else
              User_error.raise ~loc
                [ Pp.text
                    "Rule dependencies are configured to require sandboxing, \
                     but the rule has no actions that could potentially \
                     require sandboxing."
                ]

We are trying sandbox a rule that produces a directory to satisfy the sandboxing check for directory targets, but dune doesn't allow us to sandbox actions that don't benefit from sandboxing.

[snowleopard]

@rgrinberg Thanks for making this PR smaller! I think I'm mostly happy with it now but I left a couple of new questions.

[rgrinberg]

@snowleopard PR is officially ready. We are no longer introducing any changes to the internals of dune.

[snowleopard]

Looks good now, thanks!