Private Modules by rgrinberg · Pull Request #1241 · ocaml/dune

rgrinberg · 2018-09-07T21:27:08Z

This PR still needs some cleaning before it's ready for merging but it's already ready for feedback. One small issue that I think might be worth discussing is the auto mangling of private module names. The benefits will be large for users of the unwrapped mode, but it would also be quite useful for virtual libraries. As without this name mangling, it would technically be a breaking change to add a private module to a virtual library.

cc @Drup and @c-cube

emillon · 2018-09-10T08:15:21Z

src/virtual_rules.ml

          List.map ms ~f:Module.Name.to_string
          |> String.concat ~sep:"\n"
        in
+        if private_virtual_modules <> [] then begin


Since you're testing all 3 lists for emptiness, what do you think about refactoring into this:

let partitioned_modules = List.fold_left ... in match partitioned_modules with | _::_, _::_, _::_ -> {Implementation. ... } | [], _, _ -> (* error 1 *) | _, [], _ -> (* error 2 *) | _, _, [] -> (* error 3 *)

(bonus points if the pattern matching can be extracted into a check function a bit higher)

I think extracting the check into its own function but I'm not convinced the pattern matching is adding here. Now if we want to add another field, we'll have to update 3 other completely independent clauses. Why do we prefer the pattern matching here? I don't see any gains from exhaustivity checks here for example.

emillon · 2018-09-10T08:16:33Z

src/lib_rules.ml

        ~dir
        ~dir_kind
        ~obj_dir
+        ~private_obj_dir:private_obj_dir


this will soon be formatted away, but you can remove the duplication here

yeah, good call

emillon · 2018-09-10T08:17:29Z

src/module.ml

  type nonrec t = t Name.Map.t
 end
+
+let is_public t =


The pattern matching part should be in Visibility.is_public

ghost · 2018-09-10T11:54:21Z

In the end, are private modules only visible to the library they are part of? Regarding auto-mangling of names, that seems fine, however before doing that we need a clear idea of whether we want to support more fine-grained visibility in the future and how it will be presented to the user. Otherwise it might prove to be difficult to implement.

BTW, could you update the variants test to also include the private status?

rgrinberg · 2018-09-10T12:31:00Z

In the end, are private modules only visible to the library they are part of?

Yup. Did you have anything else in mind for them?

Regarding auto-mangling of names, that seems fine, however before doing that we need a clear idea of whether we want to support more fine-grained visibility in the future and how it will be presented to the user. Otherwise it might prove to be difficult to implement.

I see. So let's punt on the name mangling for now. What I had in mind is a Private alias module that would be automatically -open for the user and the private modules named as lib__private__module. But I'd like to see how this would interact with FS modules. Anyways, we can add this later without compatibility losses so it's not a blocker.

BTW, could you update the variants test to also include the private status?

Sure thing.

ghost · 2018-09-10T13:33:07Z

Yup. Did you have anything else in mind for them?

Nope, I just remember this came up while discussing private modules in the past.

test/blackbox-tests/test-cases/variants/run.t

ghost · 2018-09-11T09:10:21Z

src/module.ml

+let is_public t = Visibility.is_public t.visibility
+
+let set_private t =
+  { t with visibility = Visibility.Private }


We shouldn't need the module path here

What is the module path?

Visibility.

Ah, fixed. I thought you mean some path value of Module.t 😅

ghost · 2018-09-11T09:13:21Z

test/blackbox-tests/test-cases/private-modules/run.t

+  File "foo.ml", line 1, characters 0-5:
+  Error: Unbound module X
+
+  $ dune build --root excluded-from-install-file


I suggest to add | grep priv, so that the focus is on private elements

test/blackbox-tests/test-cases/private-modules/run.t

rgrinberg · 2018-09-11T10:02:13Z

I pushed a commit that disables the compatibility alias for private modules.

ghost · 2018-09-11T12:16:13Z

test/blackbox-tests/test-cases/private-modules/run.t

+    "_build/install/default/lib/lib/foo/priv2.cmt" {"foo/priv2.cmt"}
+    "_build/install/default/lib/lib/foo/priv2.cmx" {"foo/priv2.cmx"}
+    "_build/install/default/lib/lib/foo/priv2.ml" {"foo/priv2.ml"}
+    "_build/install/default/lib/lib/priv.ml" {"priv.ml"}


Why do we install the cmx/cmt for priv2 but not for priv?

The grep was a little too strict. The artifacts for priv appear as Priv since it's wrapped. I'fxed this.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Since it's getting quite large Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

@diml

CHANGES: - Ignore stderr output when trying to find out the number of jobs available (ocaml/dune#1118, fix ocaml/dune#1116, @diml) - Fix error message when the source directory of `copy_files` does not exist. (ocaml/dune#1120, fix ocaml/dune#1099, @emillon) - Highlight error locations in error messages (ocaml/dune#1121, @emillon) - Display actual stanza when package is ambiguous (ocaml/dune#1126, fix ocaml/dune#1123, @emillon) - Add `dune unstable-fmt` to format `dune` files. The interface and syntax are still subject to change, so use with caution. (ocaml/dune#1130, fix ocaml/dune#940, @emillon) - Improve error message for `dune utop` without a library name (ocaml/dune#1154, fix ocaml/dune#1149, @emillon) - Fix parsing `ocamllex` stanza in jbuild files (ocaml/dune#1150, @rgrinberg) - Highlight multi-line errors (ocaml/dune#1131, @anuragsoni) - Do no try to generate shared libraries when this is not supported by the OS (ocaml/dune#1165, fix ocaml/dune#1051, @diml) - Fix `Flags.write_{sexp,lines}` in configurator by avoiding the use of `Stdune.Path` (ocaml/dune#1175, fix ocaml/dune#1161, @rgrinberg) - Add support for `findlib.dynload`: when linking an executable using `findlib.dynload`, automatically record linked in libraries and findlib predicates (ocaml/dune#1172, @bobot) - Add support for promoting a selected list of files (ocaml/dune#1192, @diml) - Add an emacs mode providing helpers to promote correction files (ocaml/dune#1192, @diml) - Improve message suggesting to remove parentheses (ocaml/dune#1196, fix ocaml/dune#1173, @emillon) - Add `(wrapped (transition "..message.."))` as an option that will generate wrapped modules but keep unwrapped modules with a deprecation message to preserve compatibility. (ocaml/dune#1188, fix ocaml/dune#985, @rgrinberg) - Fix the flags passed to the ppx rewriter when using `staged_pps` (ocaml/dune#1218, @diml) - Add `(env var)` to add a dependency to an environment variable. (ocaml/dune#1186, @emillon) - Add a simple version of a polling mode: `dune build -w` keeps running and restarts the build when something change on the filesystem (ocaml/dune#1140, @kodek16) - Cleanup the way we detect the library search path. We no longer call `opam config var lib` in the default build context (ocaml/dune#1226, @diml) - Make test stanzas honor the -p flag. (ocaml/dune#1236, fix ocaml/dune#1231, @emillon) - Test stanzas take an optional (action) field to customize how they run (ocaml/dune#1248, ocaml/dune#1195, @emillon) - Add support for private modules via the `private_modules` field (ocaml/dune#1241, fix ocaml/dune#427, @rgrinberg) - Add support for passing arguments to the OCaml compiler via a response file when the list of arguments is too long (ocaml/dune#1256, @diml) - Do not print diffs by default when running inside dune (ocaml/dune#1260, @diml) - Interpret `$ dune build dir` as building the default alias in `dir`. (ocaml/dune#1259, @rgrinberg) - Make the `dynlink` library available without findlib installed (ocaml/dune#1270, fix ocaml/dune#1264, @rgrinberg)

rgrinberg requested review from a user and emillon September 7, 2018 21:27

rgrinberg force-pushed the private-modules-2 branch from 4872c29 to eddef5a Compare September 8, 2018 10:44

emillon reviewed Sep 10, 2018

View reviewed changes

ghost reviewed Sep 10, 2018

View reviewed changes

test/blackbox-tests/test-cases/variants/run.t Show resolved Hide resolved

rgrinberg force-pushed the private-modules-2 branch 3 times, most recently from dac04b6 to 23bdd80 Compare September 10, 2018 19:01

ghost suggested changes Sep 11, 2018

View reviewed changes

ghost reviewed Sep 11, 2018

View reviewed changes

rgrinberg force-pushed the private-modules-2 branch 2 times, most recently from da2b80b to b950fe5 Compare September 11, 2018 14:30

rgrinberg added 13 commits September 11, 2018 18:30

Add visibility to modules

81ef661

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add private modules field

e2d035b

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add simple private modules test

4b1bfcc

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Make obj_file respect visibility

f42fda4

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add private obj dir to compilation context

b8aadf4

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Read private modules

53287c8

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Some helpers for private module handling

a541714

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Make install rules respect private modules

2830a67

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Compile against private modules

b733a42

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add tests for private modules

f5ab8a0

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Make private_modules available only in 1.2

66f2715

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Move the .private sub dir to Utils

94b1da1

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Disable implementation from work virtual modules as private

e383928

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg added 8 commits September 11, 2018 18:30

Move pattern matching to Visibility module

60db49a

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Pun private_obj_dir field

4444096

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Move virtual modules verification to own field

c0aabcf

Since it's getting quite large Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Update variant tests to account for private modules

1ecb3c3

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add docs and changelog entry for private_modules

f005ee8

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Filter installed artifacts for private modules test

a5921f1

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Do not create compat aliases for private modules

95dd368

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Remove unnecessary module path

b950fe5

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

ghost approved these changes Sep 11, 2018

View reviewed changes

rgrinberg merged commit 7337a36 into ocaml:master Sep 11, 2018

rgrinberg deleted the private-modules-2 branch September 11, 2018 16:52

rgrinberg mentioned this pull request Sep 14, 2018

[new release] dune (1.2.0) ocaml/opam-repository#12622

Merged

hannesm mentioned this pull request Sep 17, 2018

Switch build system to Dune hannesm/logs-syslog#10

Closed

rgrinberg restored the private-modules-2 branch April 20, 2019 05:45

Conversation

rgrinberg commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 10, 2018

Uh oh!

rgrinberg commented Sep 10, 2018

Uh oh!

ghost commented Sep 10, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgrinberg commented Sep 11, 2018

Uh oh!

ghost Sep 11, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost Sep 11, 2018 •

edited by ghost

Loading