Multi directory libraries and executables #1034

ghost · 2018-07-19T14:39:16Z

This PR adds the necessary support for multi-directory libraries and executables.

Description

When writing (include_subdirs true) or simple (include_subdirs) in a dune file, dune will recurse in sub-directories to look for modules.

The recursion stops when encountering a sub-directory that:

is part of another project
contains (include_subdirs true) or (include_subdirs)
contains at least one module consumer stanza: library, executable(s), test(s)

Implementation

Dir_contents was adapted to recognize directories that are part of a group of directories. When Dir_contents.get is called on a directory that is part of a group, the whole group is scanned at once.

The rule producer in gen_rules.ml does exactly the same thing.

TODO (before this can be merged)

when looking for .c/.h files, include all directories that are part of the group

Future work

Add a mode (include_subdirs qualified)``so that sub-directories are qualifed. i.e. file a/b/x.mlwill be seen asA.B.xat the root, and asB.xina/`

rgrinberg · 2018-07-24T17:08:45Z

Add a mode (include_subdirs qualified)``so that sub-directories are qualifed. i.e. filea/b/x.mlwill be seen asA.B.xat the root, and asB.xina/`

Shouldn't this be the default? IMO we shouldn't encourage people to create arbitrary grouping of the modules that isn't reflected in the module structure.

Btw, I cherry picked the refactoring commit and rebased the rest so that it applies cleanly on master. It should make things easier to review.

rgrinberg · 2018-07-27T09:37:17Z

src/dir_contents.ml

+    | Standalone of File_tree.Dir.t
+                    * Super_context.Dir_with_jbuild.t
+    | Group_root of File_tree.Dir.t
+                    * Super_context.Dir_with_jbuild.t


Adding a brief doc for every constructor here would be hlpeful

rgrinberg · 2018-07-27T09:37:40Z

src/dir_contents.ml

+          Option.bind (Path.drop_build_context dir)
+            ~f:(File_tree.find_dir (Super_context.file_tree sctx))
+        with
+        | None -> Empty_standalone None


Is it intentional that this case doesn't populate the cache?

It does, line 520

rgrinberg · 2018-07-27T10:39:41Z

src/dir_contents.ml

+
+let cache = Hashtbl.create 32
+
+let rec get sctx ~dir =


Why not use Hashtbl.find_or_add for this function (and others like it)

I guess we could use it here. It's just unfortunate that we end up systematically creating a closure while it's only useful in the not found case.

For other functions, we modify the cache in the not found case through a recursive call. It's not clear what the behavior of Hashtbl.find_or_add is in this case should be, so it seems better to avoid it.

ghost · 2018-07-30T13:38:54Z

Shouldn't this be the default? IMO we shouldn't encourage people to create arbitrary grouping of the modules that isn't reflected in the module structure.

True, although allowing unqualified subdirs is not much work and I believe will help porting older projects to dune. What about:

(include_subdirs <false|true|unqualified>)

?

ghost · 2018-07-30T13:39:09Z

Btw, I cherry picked the refactoring commit and rebased the rest so that it applies cleanly on master. It should make things easier to review.

Thanks

rgrinberg · 2018-07-30T17:26:51Z

(include_subdirs <false|true|unqualified>)

What would (include_subdirs false) be useful for?

rgrinberg

I'm approving because the implementation looks good. @diml I'll let you pick what you think the default on qualified vs. unqualified should be.

ghost · 2018-07-31T07:37:26Z

Changing the default to (include_subdirs true) is a breaking change. For instance it breaks projects currently relying on copy_files to have multi-directory libraries. /cc @emillon as well in case he wants to chime in regarding the design (and/or code).

BTW, we still need to sort out the situation regarding .c/.h files before merging this. In the past there was a least one request to put .c files in a sub-directory. In the current state of this PR, .c/.h files in sub-directories are ignored, so we couldn't consider them in the future without introducing a breaking change. We can either make sure they are considered or simply forbid .c/.h files in sub-directories for now. I might do the latter for now since it's easier, in particular it's easier to setup the -I flags.

rgrinberg · 2018-07-31T07:40:07Z

Changing the default to (include_subdirs true) is a breaking change. For instance it breaks projects currently relying on copy_files to have multi-directory libraries.

Oh, I simply suggested that (include_subdirs) without an argument be equivalent to (include_subdirs qualified) rather than (include_subdirs unqualified). Btw, now I'm thinking if it's best that we shouldn't use booleans at all as the argument here. It will be confusing as we are introducing a new feature that users aren't really used to.

ghost · 2018-07-31T08:13:01Z

That's a good point. I'll change the syntax to the following then:

(include_subdirs <qualified|unqualified>)

ghost · 2018-07-31T08:14:06Z

And maybe we can leave out the short (include_subdirs) syntax for now, just to make things explicit.

ghost · 2018-07-31T16:56:08Z

I changed the syntax and also implemented the support for C files in sub-directories, since it wasn't that hard. @rgrinberg I let you review the latest changes.

emillon · 2018-08-01T07:32:53Z

doc/dune-files.rst

@@ -57,6 +57,11 @@ allowed to write an explicit ``Foo`` module, in which case this will
 be the interface of the library and you are free to expose only the
 modules you want.

+Note that by default libraries and other things that consume
+OCaml/Reason modules only consume modules from the directory where the
+stanza appear. In order to delcare a multi-directory library, you need


typo: declare

emillon · 2018-08-01T07:34:59Z

doc/dune-files.rst

+stop when encountering a sub-directory that:
+
+- is part of a different project (for instance when vendoring projects)
+- contains ``(include_subdirs true)`` or ``(include_subdirs)``


That should be (include_subdirs unqualified)

& the short syntax has been removed if I understand correctly.

Yep. Given that it's a new thing, it seems better to make things explicit

emillon · 2018-08-01T07:37:30Z

src/dir_contents.ml

+  | Standalone -> [t]
+  | Group_root (lazy l) -> t :: l
+  | Group_part t ->
+    match t.kind with


The compiler seems to be doing fine, but I think adding begin/end around the nested match will make this easier to read. An alternative is to use deep pattern matching instead of the nested match, and move the assert false to the outer match.

I used a deep pattern matching

emillon · 2018-08-01T07:40:41Z

src/jbuild.ml

+
+  let t =
+    enum
+      [ "unqualified", Unqualified


One thing that seems a bit weird is that there is no syntax for the default case. Would it be useful to have (include_subdirs no)?

I suppose. It especially makes sense if we plan to one day change the default to (include_subdirs qualified). I added no and reworked the documentation

rgrinberg

Looks good. Just some comments that need to be updated first.

rgrinberg · 2018-08-01T10:10:09Z

src/dir_contents.ml

+
+    | Is_component_of_a_group_but_not_the_root of
+        Super_context.Dir_with_jbuild.t option
+    (* Sub-directory of a directory with [(include_subdirs true)] *)


This commend needs to be updated as you've changed the argument for this stanza.

rgrinberg · 2018-08-01T10:10:20Z

src/dir_contents.ml

+
+    | Group_root of File_tree.Dir.t
+                    * Super_context.Dir_with_jbuild.t
+    (* Directory with [(include_subdirs true)] *)


This comment as well

rgrinberg · 2018-08-01T10:11:28Z

src/gen_rules.ml

+                Path.relative (Dir_contents.dir dc) fn :: acc
+              else
+                acc))
+        |> List.rev


Why do we need this reverse? I just see that we provide these as command line args, does the order matter there?

Indeed, it doesn't matter. Note that it's not even command line flags, it's just to specify dependencies

rgrinberg · 2018-08-01T10:20:12Z

src/gen_rules.ml

+           | Some _ ->
+             match Dir_contents.kind (Dir_contents.get sctx ~dir) with
+             | Group_part root ->
+               SC.load_dir sctx ~dir:(Dir_contents.dir root)


How come we only do this in the case when there are no stanzas in dir? Seems like we should let the paren that is part of multi dir lib regardless

It was done in the other gen_rules function. However, it's true that this code wasn't very clear and was starting to become spaghetti code, so I refactored it

Signed-off-by: Jeremie Dimino <[email protected]>

Signed-off-by: Rudi Grinberg <[email protected]>

Signed-off-by: Jeremie Dimino <[email protected]>

rgrinberg

Looks good. Just need a CHANGES update

Signed-off-by: Jeremie Dimino <[email protected]>

ghost · 2018-08-01T14:25:19Z

@bobot in the end we did add first class support for multi directory libraries

emillon · 2018-08-01T14:47:20Z

Looks good, thanks!

@rgrinberg

CHANGES: - Fix lookup of command line specified files when `--root` is given. Previously, passing in `--root` in conjunction with `--workspace` or `--config` would not work correctly (ocaml/dune#997, @rgrinberg) - Add support for customizing env nodes in workspace files. The `env` stanza is now allowed in toplevel position in the workspace file, or for individual contexts. This feature requires `(dune lang 1.1)` (ocaml/dune#1038, @rgrinberg) - Add `enabled_if` field for aliases and tests. This field controls whether the test will be ran using a boolean expression language. (ocaml/dune#819, @rgrinberg) - Make `name`, `names` fields optional when a `public_name`, `public_names` field is provided. (ocaml/dune#1041, fix ocaml/dune#1000, @rgrinberg) - Interpret `X` in `--libdir X` as relative to `PREFIX` when `X` is relative (ocaml/dune#1072, fix ocaml/dune#1070, @diml) - Add support for multi directory libraries by writing `(include_subdirs unqualified)` (ocaml/dune#1034, @diml) - Add `(staged_pps ...)` to support staged ppx rewriters such as ones using the OCaml typer like `ppx_import` (ocaml/dune#1080, fix ocaml/dune#193, @diml) - Use `-opaque` in the `dev` profile. This option trades off binary quality for compilation speed when compiling .cmx files. (ocaml/dune#1079, fix ocaml/dune#1058, @rgrinberg) - Fix placeholders in `dune subst` documentation (ocaml/dune#1090, @emillon, thanks @trefis for the bug report) - Add locations to errors when a missing binary in PATH comes from a dune file (ocaml/dune#1096, fixes ocaml/dune#1095, @rgrinberg)

ghost requested a review from rgrinberg July 19, 2018 14:39

rgrinberg reviewed Jul 27, 2018

View reviewed changes

rgrinberg approved these changes Jul 30, 2018

View reviewed changes

ghost requested a review from emillon July 31, 2018 17:01

emillon reviewed Aug 1, 2018

View reviewed changes

rgrinberg requested changes Aug 1, 2018

View reviewed changes

jeremiedimino and others added 13 commits August 1, 2018 11:43

Implement scanning of multi-dir groups

ba1e526

Signed-off-by: Jeremie Dimino <[email protected]>

Wire things in gen_rules

8a613ba

Signed-off-by: Jeremie Dimino <[email protected]>

Implement the syntax

f868aab

Signed-off-by: Jeremie Dimino <[email protected]>

Add tests

3f5e3df

Signed-off-by: Jeremie Dimino <[email protected]>

doc

7685047

Signed-off-by: Jeremie Dimino <[email protected]>

Make sure the include_subdirs stanza cannot appear twice

b3df8fb

Signed-off-by: Jeremie Dimino <[email protected]>

Move empty modules value to own definition

b9e4471

Signed-off-by: Rudi Grinberg <[email protected]>

Document Dir_status.t

b9982df

Signed-off-by: Jeremie Dimino <[email protected]>

_

b6ff3ce

Signed-off-by: Jeremie Dimino <[email protected]>

Change the syntax as discussed

1eb2b36

Signed-off-by: Jeremie Dimino <[email protected]>

Add a test for C stubs

1f4aa7c

Signed-off-by: Jeremie Dimino <[email protected]>

Support C files in sub-directories

0594008

Signed-off-by: Jeremie Dimino <[email protected]>

Make sure loading a group component loads the whole group

ab719ce

Signed-off-by: Jeremie Dimino <[email protected]>

jeremiedimino added 6 commits August 1, 2018 11:43

Fix doc

7b0861a

Signed-off-by: Jeremie Dimino <[email protected]>

tweak

8c1b2de

Signed-off-by: Jeremie Dimino <[email protected]>

Add a no mode

8d85e3a

Signed-off-by: Jeremie Dimino <[email protected]>

Update comment

0d89d42

Signed-off-by: Jeremie Dimino <[email protected]>

Remove useless List.rev

a5d3b3a

Signed-off-by: Jeremie Dimino <[email protected]>

Refactor gen_rules

74b335e

Signed-off-by: Jeremie Dimino <[email protected]>

rgrinberg approved these changes Aug 1, 2018

View reviewed changes

Update changelog

1943a8a

Signed-off-by: Jeremie Dimino <[email protected]>

ghost merged commit 282c0b3 into ocaml:master Aug 1, 2018

rgrinberg mentioned this pull request Aug 6, 2018

[new release] dune (1.1.0) ocaml/opam-repository#12442

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi directory libraries and executables #1034

Multi directory libraries and executables #1034

ghost commented Jul 19, 2018 •

edited by ghost

Loading

rgrinberg commented Jul 24, 2018 •

edited

Loading

rgrinberg Jul 27, 2018

rgrinberg Jul 27, 2018

ghost Jul 30, 2018

rgrinberg Jul 27, 2018

ghost Jul 30, 2018

ghost commented Jul 30, 2018

ghost commented Jul 30, 2018

rgrinberg commented Jul 30, 2018

rgrinberg left a comment

ghost commented Jul 31, 2018

rgrinberg commented Jul 31, 2018

ghost commented Jul 31, 2018

ghost commented Jul 31, 2018

ghost commented Jul 31, 2018

emillon Aug 1, 2018

emillon Aug 1, 2018

emillon Aug 1, 2018

ghost Aug 1, 2018

emillon Aug 1, 2018

ghost Aug 1, 2018

emillon Aug 1, 2018

ghost Aug 1, 2018

rgrinberg left a comment

rgrinberg Aug 1, 2018

rgrinberg Aug 1, 2018

rgrinberg Aug 1, 2018

ghost Aug 1, 2018

rgrinberg Aug 1, 2018

ghost Aug 1, 2018

rgrinberg left a comment

ghost commented Aug 1, 2018

emillon commented Aug 1, 2018

Multi directory libraries and executables #1034

Multi directory libraries and executables #1034

Conversation

ghost commented Jul 19, 2018 • edited by ghost Loading

Description

Implementation

TODO (before this can be merged)

Future work

rgrinberg commented Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 30, 2018

ghost commented Jul 30, 2018

rgrinberg commented Jul 30, 2018

rgrinberg left a comment

Choose a reason for hiding this comment

ghost commented Jul 31, 2018

rgrinberg commented Jul 31, 2018

ghost commented Jul 31, 2018

ghost commented Jul 31, 2018

ghost commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg left a comment

Choose a reason for hiding this comment

ghost commented Aug 1, 2018

emillon commented Aug 1, 2018

ghost commented Jul 19, 2018 •

edited by ghost

Loading

rgrinberg commented Jul 24, 2018 •

edited

Loading