feature: install dir by rgrinberg · Pull Request #5097 · ocaml/dune

rgrinberg · 2021-11-04T17:50:38Z

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.

src/dune_engine/build_system.ml

test/blackbox-tests/test-cases/directory-targets/install-dir-target.t

rgrinberg · 2022-06-02T20:30:19Z

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?

src/dune_engine/build_system.ml

src/dune_engine/cached_digest.ml

src/dune_engine/fs_memo.ml

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 · 2022-08-04T18:33:33Z

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 · 2022-08-04T19:26:30Z

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

src/dune_engine/target_promotion.ml

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 · 2022-08-07T01:39:30Z

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 · 2022-08-08T23:10:32Z

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.

src/dune_engine/action_builder.ml

src/dune_engine/cached_digest.ml

snowleopard · 2022-08-16T23:27:44Z

@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 · 2022-08-16T23:33:25Z

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

snowleopard

Looks good now, thanks!

Signed-off-by: Rudi Grinberg <me@rgrinberg.com> ps-id: 175956A3-5AAE-466E-874C-12198E224F96

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0~alpha1) CHANGES: - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses findlib). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, fixes ocaml/dune#5650, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter)

@rgrinberg

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) CHANGES: - macOS: Handle unknown fsevents without crashing (ocaml/dune#6217, @rgrinberg) - Enable file watching on MacOS SDK < 10.13. (ocaml/dune#6218, @rgrinberg) - Sandbox running cinaps actions starting from cinaps 1.1 (ocaml/dune#6176, @rgrinberg) - Add a `runtime_deps` field in the `cinaps` stanza to specify runtime dependencies for running the cinaps preprocessing action (ocaml/dune#6175, @rgrinberg) - Shadow alias module `Foo__` when building a library `Foo` (ocaml/dune#6126, @rgrinberg) - Extend dune describe to include the root path of the workspace and the relative path to the build directory. (ocaml/dune#6136, @reubenrowe) - Allow dune describe workspace to accept directories as arguments. The provided directories restrict the worskpace description to those directories. (ocaml/dune#6107, fixes ocaml/dune#3893, @esope) - Add a terminal persistence mode that attempts to clear the terminal history. It is enabled by setting terminal persistence to `clear-on-rebuild-and-flush-history` (ocaml/dune#6065, @rgrinberg) - Disallow generating targets in sub direcories in inferred rules. The check to forbid this was accidentally done only for manually specified targets (ocaml/dune#6031, @rgrinberg) - Do not ignore rules marked `(promote (until-clean))` when `--ignore-promoted-rules` (or `-p`) is passed. (ocaml/dune#6010, fixes ocaml/dune#4401, @emillon) - Dune no longer considers .aux files as targets during Coq compilation. This means that .aux files are no longer cached. (ocaml/dune#6024, fixes ocaml/dune#6004, @Alizter) - Cinaps actions are now sandboxed by default (ocaml/dune#6062, @rgrinberg) - Allow rules producing directory targets to be not sandboxed (ocaml/dune#6056, @rgrinberg) - Introduce a `dirs` field in the `install` stanza to install entire directories (ocaml/dune#5097, fixes ocaml/dune#5059, @rgrinberg) - Menhir rules are now sandboxed by default (ocaml/dune#6076, @rgrinberg) - Allow rules producing directory targets to create symlinks (ocaml/dune#6077, fixes ocaml/dune#5945, @rgrinberg) - Inline tests are now sandboxed by default (ocaml/dune#6079, @rgrinberg) - Fix build-info version when used with flambda (ocaml/dune#6089, fixes ocaml/dune#6075, @jberdine) - Add an `(include <file>)` term to the `include_dirs` field for adding directories to the include paths sourced from a file. (ocaml/dune#6058, fixes ocaml/dune#3993, @gridbugs) - Support `(extra_objects ...)` field in `(executable ...)` and `(library ...)` stanzas (ocaml/dune#6084, fixes ocaml/dune#4129, @gridbugs) - Fix compilation of Dune under esy on Windows (ocaml/dune#6109, fixes ocaml/dune#6098, @nojb) - Improve error message when parsing several licenses in `(license)` (ocaml/dune#6114, fixes ocaml/dune#6103, @emillon) - odoc rules now about `ODOC_SYNTAX` and will rerun accordingly (ocaml/dune#6010, fixes ocaml/dune#1117, @emillon) - dune install: copy files in an atomic way (ocaml/dune#6150, @emillon) - Add `%{coq:...}` macro for accessing data about the configuration about Coq. For instance `%{coq:version}` (ocaml/dune#6049, @Alizter) - update vendored copy of cmdliner to 1.1.1. This improves the built-in documentation for command groups such as `dune ocaml`. (ocaml/dune#6038, @emillon, ocaml/dune#6169, @shonfeder) - The test suite for Coq now requires Coq >= 8.16 due to changes in the plugin loading mechanism upstream (which now uses `Findlib`). - Starting with Coq build language 0.6, theories can be built without importing Coq's standard library by including `(stdlib no)`. (ocaml/dune#6165 ocaml/dune#6164, fixes ocaml/dune#6163, @ejgallego @Alizter @LasseBlaauwbroek) - on macOS, sign executables produced by artifact substitution (ocaml/dune#6137, ocaml/dune#6231, fixes ocaml/dune#5650, fixes ocaml/dune#6226, @emillon) - Added an (aliases ...) field to the (rules ...) stanza which allows the specification of multiple aliases per rule (ocaml/dune#6194, @Alizter) - The `(coq.theory ...)` stanza will now ensure that for each declared `(plugin ...)`, the `META` file for it is built before calling `coqdep`. This enables the use of the new `Findlib`-based loading method in Coq 8.16; however as of Coq 8.16.0, Coq itself has some bugs preventing this to work yet. (ocaml/dune#6167 , workarounds ocaml/dune#5767, @ejgallego) - Allow include statement in install stanza (ocaml/dune#6139, fixes ocaml/dune#256, @gridbugs) - Handle CSI n K code in ANSI escape codes from commands. (ocaml/dune#6214, fixes ocaml/dune#5528, @emillon) - Add a new experimental feature `mode_specific_stubs` that allows the specification of different flags and sources for foreign stubs depending on the build mode (ocaml/dune#5649, @voodoos)

rgrinberg requested a review from snowleopard November 4, 2021 17:50

rgrinberg linked an issue Nov 4, 2021 that may be closed by this pull request

Install directories #5059

Closed

rgrinberg commented Nov 4, 2021

View reviewed changes

src/dune_engine/build_system.ml Outdated Show resolved Hide resolved

snowleopard reviewed Nov 4, 2021

View reviewed changes

test/blackbox-tests/test-cases/directory-targets/install-dir-target.t Outdated Show resolved Hide resolved

snowleopard reviewed Nov 4, 2021

View reviewed changes

test/blackbox-tests/test-cases/directory-targets/install-dir-target.t Show resolved Hide resolved

rgrinberg linked an issue May 2, 2022 that may be closed by this pull request

dune install fails to install directories #3752

Closed

rgrinberg force-pushed the ps/rr/feature__install_dir branch 2 times, most recently from 3b6bc23 to 715216e Compare June 2, 2022 20:27

rgrinberg requested review from emillon and staronj June 2, 2022 20:30

rgrinberg added this to the 3.3.0 milestone Jun 2, 2022

snowleopard reviewed Jun 3, 2022

View reviewed changes

src/dune_engine/build_system.ml Outdated Show resolved Hide resolved

snowleopard reviewed Jun 3, 2022

View reviewed changes

src/dune_engine/build_system.ml Outdated Show resolved Hide resolved

snowleopard reviewed Jun 3, 2022

View reviewed changes

src/dune_engine/cached_digest.ml Outdated Show resolved Hide resolved

snowleopard reviewed Jun 3, 2022

View reviewed changes

src/dune_engine/fs_memo.ml Show resolved Hide resolved

snowleopard requested changes Jun 3, 2022

View reviewed changes

rgrinberg modified the milestones: 3.3.0, 3.4.0 Jun 17, 2022

rgrinberg force-pushed the ps/rr/feature__install_dir branch from 715216e to 4215ff0 Compare June 21, 2022 00:19

emillon modified the milestones: 3.4.0, 3.5.0 Jul 20, 2022

rgrinberg force-pushed the ps/rr/feature__install_dir branch 3 times, most recently from 8226332 to 9eaaace Compare August 4, 2022 18:04

rgrinberg force-pushed the ps/rr/feature__install_dir branch from 9eaaace to 8bae704 Compare August 4, 2022 18:33

snowleopard reviewed Aug 6, 2022

View reviewed changes

src/dune_engine/target_promotion.ml Outdated Show resolved Hide resolved

snowleopard reviewed Aug 6, 2022

View reviewed changes

rgrinberg force-pushed the ps/rr/feature__install_dir branch 2 times, most recently from f743a22 to a3b5d0c Compare August 7, 2022 01:58

snowleopard reviewed Aug 16, 2022

View reviewed changes

src/dune_engine/action_builder.ml Outdated Show resolved Hide resolved

snowleopard reviewed Aug 16, 2022

View reviewed changes

src/dune_engine/cached_digest.ml Outdated Show resolved Hide resolved

rgrinberg force-pushed the ps/rr/feature__install_dir branch from a3b5d0c to fc7e35f Compare August 16, 2022 23:27

rgrinberg force-pushed the ps/rr/feature__install_dir branch from fc7e35f to fe08534 Compare August 16, 2022 23:30

snowleopard approved these changes Aug 16, 2022

View reviewed changes

rgrinberg force-pushed the ps/rr/feature__install_dir branch 2 times, most recently from 430aa5a to 2e7cc67 Compare August 17, 2022 00:24

feature: install dir

c35ad2a

Signed-off-by: Rudi Grinberg <me@rgrinberg.com> ps-id: 175956A3-5AAE-466E-874C-12198E224F96

rgrinberg force-pushed the ps/rr/feature__install_dir branch from 2e7cc67 to c35ad2a Compare August 17, 2022 00:28

rgrinberg merged commit 29920c5 into main Aug 17, 2022

kit-ty-kate mentioned this pull request Sep 7, 2022

The new dir install feature does not work on existing directories #6133

Open

emillon mentioned this pull request Oct 19, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.5.0) ocaml/opam-repository#22317

Merged

Alizter deleted the ps/rr/feature__install_dir branch November 12, 2022 15:16

ejgallego mentioned this pull request Jul 3, 2023

[RFC] Installable directories #1868

Closed

Conversation

rgrinberg commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowleopard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Aug 4, 2022

Uh oh!

snowleopard commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

snowleopard left a comment

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Aug 7, 2022

Uh oh!

rgrinberg commented Aug 8, 2022

Uh oh!

Uh oh!

Uh oh!

snowleopard commented Aug 16, 2022

Uh oh!

rgrinberg commented Aug 16, 2022

Uh oh!

snowleopard left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rgrinberg commented Nov 4, 2021 •

edited

Loading

rgrinberg commented Jun 2, 2022 •

edited

Loading

snowleopard left a comment •

edited

Loading

snowleopard commented Aug 4, 2022 •

edited

Loading