Skip to content

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

Merged
rgrinberg merged 1 commit intoocaml:mainfrom
gridbugs:install-glob
Oct 29, 2022
Merged

Add (glob_files <glob>) and (glob_files_rec <glob>) to the files field of the install stanza#6250
rgrinberg merged 1 commit intoocaml:mainfrom
gridbugs:install-glob

Conversation

@gridbugs
Copy link
Copy Markdown
Collaborator

@gridbugs gridbugs commented Oct 18, 2022

Closes #6018

@gridbugs gridbugs force-pushed the install-glob branch 2 times, most recently from 1b734c3 to e5f29eb Compare October 18, 2022 12:26
@emillon emillon added this to the 3.6.0 milestone Oct 18, 2022
christinerose
christinerose suggested changes Oct 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

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

rgrinberg
rgrinberg reviewed Oct 18, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 18, 2022
View reviewed changes
src/dune_rules/glob_files.mli
@@ -0,0 +1,17 @@
open! Import
Copy link
Copy Markdown
Member

@rgrinberg 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
rgrinberg reviewed Oct 18, 2022
View reviewed changes
src/dune_rules/dune_file.ml Outdated
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)
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Oct 18, 2022

Choose a reason for hiding this comment

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

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

rgrinberg
rgrinberg reviewed Oct 18, 2022
View reviewed changes
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Oct 18, 2022

Should be updated to require 3.6.0

Btw, does this compose with include?

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

gridbugs commented Oct 20, 2022

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

@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 20, 2022

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.

rgrinberg
rgrinberg reviewed Oct 20, 2022
View reviewed changes
@gridbugs gridbugs force-pushed the install-glob branch 2 times, most recently from 296392c to 8118f81 Compare October 21, 2022 05:46
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 21, 2022

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
Copy link
Copy Markdown
Member

rgrinberg commented Oct 21, 2022

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 gridbugs force-pushed the install-glob branch 2 times, most recently from 997a63d to 2d73dfa Compare October 24, 2022 06:48
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 24, 2022

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
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 24, 2022

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 gridbugs force-pushed the install-glob branch 3 times, most recently from c3f496b to e665cee Compare October 25, 2022 01:56
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 25, 2022

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
Copy link
Copy Markdown
Member

rgrinberg commented Oct 25, 2022

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

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Oct 25, 2022

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

@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 26, 2022

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.

rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
src/dune_rules/glob_files.ml Outdated
>>| List.map ~f:(replace_path_dir relative_dir))
>>| List.sort ~compare:String.compare

let expand_memo t ~f ~base_dir =
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Oct 26, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator Author

@gridbugs gridbugs Oct 26, 2022

Choose a reason for hiding this comment

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

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.

rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 26, 2022
View reviewed changes
@gridbugs gridbugs force-pushed the install-glob branch 2 times, most recently from b77396d to e954fbb Compare October 26, 2022 06:05
@gridbugs
Copy link
Copy Markdown
Collaborator Author

gridbugs commented Oct 26, 2022

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.

@gridbugs gridbugs force-pushed the install-glob branch 4 times, most recently from 5a81874 to 09a3ef3 Compare October 28, 2022 05:38
@gridbugs @rgrinberg
feature: glob_files to (install (files ...))
e7dead3 
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@rgrinberg rgrinberg merged commit b61bd6a into ocaml:main Oct 29, 2022
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Oct 29, 2022

Merged after a few cosmetic changes and simplifications. Thanks.

jchavarri added a commit to jchavarri/dune that referenced this pull request Oct 29, 2022
@jchavarri
Merge branch 'main' into melange/private-modules
4161117 
* main:
  feature: glob_files to (install (files ...)) (ocaml#6250)
@anmonteiro anmonteiro mentioned this pull request Nov 6, 2022
Open
@emillon emillon mentioned this pull request Nov 14, 2022
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Nov 14, 2022
@emillon
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
f207d3f 
…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 gridbugs deleted the install-glob branch October 11, 2023 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

+1 more reviewer

@christinerose christinerose christinerose approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.6.0

Development

Successfully merging this pull request may close these issues.

Allow globbing in the install stanza

4 participants

@gridbugs @rgrinberg @christinerose @emillon