Add `(glob_files <glob>)` and `(glob_files_rec <glob>)` to the `files` field of the `install` stanza #6250

gridbugs · 2022-10-18T12:24:09Z

Closes #6018

christinerose

A few minor grammar / formatting suggestions as well as a question or two.

src/dune_rules/glob_files.ml

test/blackbox-tests/test-cases/install-glob/install-glob-files-only.t

test/blackbox-tests/test-cases/install-glob/install-glob-recursive.t

src/dune_rules/dep_conf.ml

rgrinberg · 2022-10-18T22:07:46Z

src/dune_rules/glob_files.mli

@@ -0,0 +1,17 @@
+open! Import


Add a small description of the module please.

rgrinberg · 2022-10-18T22:11:13Z

src/dune_rules/dune_file.ml

+      let to_file_bindings_expanded t ~expand_str ~dir =
+        let open Memo.O in
+        to_file_bindings_unexpanded t ~expand_str
+        >>= Memo.List.map ~f:(File_binding.Unexpanded.expand ~dir ~f:expand_str)


It's a matter of style, but you can just do Memo.bind and remove the let open uses only once.

src/dune_rules/dep_conf.ml

rgrinberg · 2022-10-18T22:12:53Z

Should be updated to require 3.6.0

Btw, does this compose with include?

gridbugs · 2022-10-20T12:16:10Z

Updated to require 3.6. Good question about include. It should, but I'll add a test tomorrow to confirm.

gridbugs · 2022-10-20T12:19:05Z

I added a test which used globs in a dune file inside a subdirectory and that highlighted a problem with my implementation. I've updated this so that now the Glob_files.expand_* functions take an additional base_dir argument, and the globs are expanded relative to this argument. This involved reimplementing the Glob_files module. Apologies for the noise.

test/blackbox-tests/test-cases/glob_files_rec.t

gridbugs · 2022-10-21T05:47:50Z

Updated to require 3.6. Good question about include. It should, but I'll add a test tomorrow to confirm.

Added test composing glob_files and include

rgrinberg · 2022-10-21T14:41:32Z

The failure in CI looks a little suspicious:

#=== ERROR while compiling menhirLib.20220210 =================================#
# context     2.1.3 | linux/x86_64 | ocaml-base-compiler.4.14.0 | git+https://github.com/ocaml/opam-repository.git
# path        ~/work/dune/dune/_opam/.opam-switch/build/menhirLib.20220210
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p menhirLib -j 2
# exit-code   1
# env-file    ~/.opam/log/menhirLib-5720-7a711d.env
# output-file ~/.opam/log/menhirLib-5720-7a711d.out
### output ###
# Error: path outside the workspace: ../*.{ml,mli} from .
# -> required by _build/default/lib/pack/menhirLib.ml
# -> required by _build/install/default/lib/menhirLib/menhirLib.ml
# -> required by _build/default/menhirLib.install
# -> required by alias install

Do you think it's related to some your changes?

gridbugs · 2022-10-24T06:48:36Z

The failure in CI looks a little suspicious:

#=== ERROR while compiling menhirLib.20220210 =================================#
# context     2.1.3 | linux/x86_64 | ocaml-base-compiler.4.14.0 | git+https://github.com/ocaml/opam-repository.git
# path        ~/work/dune/dune/_opam/.opam-switch/build/menhirLib.20220210
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p menhirLib -j 2
# exit-code   1
# env-file    ~/.opam/log/menhirLib-5720-7a711d.env
# output-file ~/.opam/log/menhirLib-5720-7a711d.out
### output ###
# Error: path outside the workspace: ../*.{ml,mli} from .
# -> required by _build/default/lib/pack/menhirLib.ml
# -> required by _build/install/default/lib/menhirLib/menhirLib.ml
# -> required by _build/default/menhirLib.install
# -> required by alias install

Do you think it's related to some your changes?

Interesting. I'll investigate this tomorrow.

gridbugs · 2022-10-24T06:54:19Z

I found yet another case which this didn't handle. Namely, it would crash if the glob contained an absolute path. Note that relative globs containing absolute paths previously resulted in an unhandled error. I've added a User_error for this case instead. I still need to add a test for the new error, but I tried earlier today and the loc underlining was too wide. Need to investigate that further. I would have caught this case sooner (we do have a test for absolute globs) but when I run the test suite locally (tried on nixos and a very vanilla ubuntu docker image) many of the tests fail and I'm not sure why. Will investigate this tomorrow too.

gridbugs · 2022-10-25T01:58:11Z

I found two cases which would previously crash dune (prior to this PR that is):

absolute paths to files or directories in the install stanza
absolute paths in (glob_files_rec ...) (in (rule (deps ...)))

I've made these cases result in user errors instead and updated the docs accordingly.

rgrinberg · 2022-10-25T15:33:49Z

Why aren't absolute paths allowed in install stanzas? They're allowed in dependency specifications.

rgrinberg · 2022-10-25T15:34:14Z

It would also be good to separate all the absolute paths business into a separate PR.

gridbugs · 2022-10-26T01:41:35Z

Why aren't absolute paths allowed in install stanzas? They're allowed in dependency specifications.

Just because currently they cause dune to crash. File_binding.Unexpanded.expand assumes that its input is a relative path. I think this is a bug and that they should be supported but I didn't want to fix that bug as part of this work.

src/dune_rules/dune_file.ml

src/dune_rules/glob_files.ml

rgrinberg · 2022-10-26T03:00:39Z

src/dune_rules/glob_files.ml

+          >>| List.map ~f:(replace_path_dir relative_dir))
+  >>| List.sort ~compare:String.compare
+
+let expand_memo t ~f ~base_dir =


This function looks awfully similar, can we only keep one if possible?

I've had a go at combining them with a functor, and added a Glob_file.Expand signature to avoid repetition in the interface. Can you take a look and tell me if there's too much indirection for it to be readable? It's non-trivial because it needs to generalize over the Memo.t and Action_builder.t type constructors. Is there are more ergonomic way of doing this than what I've come up with? I tried to simplify it with first-class modules but ran into an issue because I couldn't access internal parameterized types of first-class modules in function signatures.

src/dune_rules/glob_files.mli

test/blackbox-tests/test-cases/glob-deps-outside-directory.t

test/blackbox-tests/test-cases/glob_files_rec.t

gridbugs · 2022-10-26T06:17:22Z

It would also be good to separate all the absolute paths business into a separate PR.

Agreed. I've opened #6331 and rebased this PR on top.

Signed-off-by: Stephen Sherratt <[email protected]>

rgrinberg · 2022-10-29T17:43:16Z

Merged after a few cosmetic changes and simplifications. Thanks.

* main: feature: glob_files to (install (files ...)) (ocaml#6250)

@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.6.0) CHANGES: - Forbid multiple instances of dune running concurrently in the same workspace. (ocaml/dune#6360, fixes ocaml/dune#236, @rgrinberg) - Allow promoting into source directories specified by `subdir` (ocaml/dune#6404, fixes ocaml/dune#3502, @rgrinberg) - Make dune describe workspace return the correct root path (ocaml/dune#6380, fixes ocaml/dune#6379, @esope) - Introduce a `$ dune ocaml top-module` subcommand to load modules directly without sealing them behind the signature. (ocaml/dune#5940, @rgrinberg) - [ctypes] do not mangle user written names in the ctypes stanza (ocaml/dune#6374, fixes ocaml/dune#5561, @rgrinberg) - Support `CLICOLOR` and `CLICOLOR_FORCE` to enable/disable/force ANSI colors. (ocaml/dune#6340, fixes ocaml/dune#6323, @MisterDA). - Forbid private libraries with `(package ..)` set from depending on private libraries that don't belong to a package (ocaml/dune#6385, fixes ocaml/dune#6153, @rgrinberg) - Allow `Byte_complete` binaries to be installable (ocaml/dune#4873, @AltGr, @rgrinberg) - Revive `$ dune external-lib-deps` under `$ dune describe external-lib-deps`. (ocaml/dune#6045, @moyodiallo) - Fix running inline tests in bytecode mode (ocaml/dune#5622, fixes ocaml/dune#5515, @dariusf) - [ctypes] always re-run `pkg-config` because we aren't tracking its external dependencies (ocaml/dune#6052, @rgrinberg) - [ctypes] remove dependency on configurator in the generated rules (ocaml/dune#6052, @rgrinberg) - Build progress status now shows number of failed jobs (ocaml/dune#6242, @Alizter) - Allow absolute build directories to find public executables. For example, those specified with `(deps %{bin:...})` (ocaml/dune#6326, @anmonteiro) - Create a fake socket file `_build/.rpc/dune` on windows to allow rpc clients to connect using the build directory. (ocaml/dune#6329, @rgrinberg) - Prevent crash if absolute paths are used in the install stanza and in recursive globs. These cases now result in a user error. (ocaml/dune#6331, @gridbugs) - Add `(glob_files <glob>)` and `(glob_files_rec <glob>)` terms to the `files` field of the `install` stanza (ocaml/dune#6250, closes ocaml/dune#6018, @gridbugs) - Allow `:standard` in the `(modules)` field of the `coq.pp` stanza (ocaml/dune#6229, fixes ocaml/dune#2414, @Alizter) - Fix passing of flags to dune coq top (ocaml/dune#6369, fixes ocaml/dune#6366, @Alizter) - Extend the promotion CLI to a `dune promotion` group: `dune promote` is moved to `dune promotion apply` (the former still works) and the new `dune promotion diff` command can be used to just display the promotion without applying it. (ocaml/dune#6160, fixes ocaml/dune#5368, @emillon)

gridbugs requested a review from christinerose as a code owner October 18, 2022 12:24

gridbugs force-pushed the install-glob branch 2 times, most recently from 1b734c3 to e5f29eb Compare October 18, 2022 12:26

emillon added this to the 3.6.0 milestone Oct 18, 2022

christinerose suggested changes Oct 18, 2022

View reviewed changes

rgrinberg reviewed Oct 18, 2022

View reviewed changes

src/dune_rules/dep_conf.ml Outdated Show resolved Hide resolved

rgrinberg reviewed Oct 18, 2022

View reviewed changes

src/dune_rules/glob_files.mli

@@ -0,0 +1,17 @@

open! Import

Copy link

Member

rgrinberg Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a small description of the module please.

rgrinberg reviewed Oct 18, 2022

View reviewed changes

src/dune_rules/dep_conf.ml Outdated Show resolved Hide resolved

gridbugs force-pushed the install-glob branch 2 times, most recently from 7c09323 to 071b100 Compare October 20, 2022 12:15

gridbugs force-pushed the install-glob branch from 071b100 to e84bd95 Compare October 20, 2022 12:16

rgrinberg reviewed Oct 20, 2022

View reviewed changes

test/blackbox-tests/test-cases/glob_files_rec.t Outdated Show resolved Hide resolved

christinerose approved these changes Oct 20, 2022

View reviewed changes

gridbugs force-pushed the install-glob branch 2 times, most recently from 296392c to 8118f81 Compare October 21, 2022 05:46

gridbugs force-pushed the install-glob branch 2 times, most recently from 997a63d to 2d73dfa Compare October 24, 2022 06:48

gridbugs force-pushed the install-glob branch 3 times, most recently from c3f496b to e665cee Compare October 25, 2022 01:56

gridbugs force-pushed the install-glob branch from e665cee to 824f2a4 Compare October 25, 2022 06:04