Fix #985 by rgrinberg · Pull Request #1188 · ocaml/dune

rgrinberg · 2018-08-29T07:50:08Z

Add a transition mode to wrapped modules. This will add aliases to the unprefixed names with a deprecation message as in #985.

For some reason, I can't see the deprecation message in the test. @diml any idea why?

emillon · 2018-08-29T08:29:36Z

I'm not sure that it's a good idea to use dates here. Some library authors might want to use a version number, or just a generic "some day".

What do you think about making this a generic deprecation message, like (transition "message")?

ghost · 2018-08-29T08:49:45Z

The idea of the date was to remind the developer of the library using the transition mode to get rid of it after the given date. Though maybe that's not really the job of dune to do that. Taking the deprecation message as argument seems good.

ghost · 2018-08-29T08:51:33Z

For some reason, I can't see the deprecation message in the test. @diml any idea why?

Ah, that's annoying. Misplaced attributes are silently ignored by the compiler so I don't know if it's supported at all in this position. Need to look in the compiler code to see what we can do.

rgrinberg · 2018-08-29T10:20:25Z

The [@@@deprecated ...] works

ghost · 2018-08-29T10:25:26Z

src/dune_file.ml

+      | Yes_with_transition of string
+
+    let dparse =
+      if_list


We can use sum directly here:

sum ["true", return (Simple true) ...

ghost · 2018-08-29T10:30:23Z

src/lib_rules.ml

+    );
+    let dep_graphs =
+      Ocamldep.Dep_graphs.deprecated ~modules ~deprecated in
+    let cctx = Compilation_context.set_modules cctx deprecated in


We should use a dedicated compilation context here, i.e. with preprocessing, special flags, libraries, etc... Since these modules are under our control, we know exactly how they should be compiled. It should be similar to the one for the alias module

ghost · 2018-08-29T10:33:19Z

src/module.ml

+    intf = None
+  ; impl =
+      Some (
+        let impl = Option.value_exn t.impl in


What happens if we use (wrapped (transition "")) with (modules_without_implementation x)?

Good call. I fixed this and added a test case.

ghost · 2018-08-29T10:34:36Z

src/dir_contents.ml

    { modules          : Module.t Module.Name.Map.t
    ; alias_module     : Module.t option
    ; main_module_name : Module.Name.t
+    ; deprecated       : Module.t Module.Name.Map.t


I'm not sure deprecated is the right name. What about unwrapped_compat?

hhugo · 2018-08-29T18:09:40Z

I've tried this PR with https://github.com/ocsigen/js_of_ocaml/tree/transition and got the following error message:

Multiple rules generated for _build/default/lib/js_of_ocaml.cmo.js:
- <internal location>
- <internal location>

note that lib/js_of_ocaml.ml exists and js_of_ocaml is the name of the library.

rgrinberg · 2018-08-29T19:22:49Z

@hhugo could you try the latest commit?

hhugo · 2018-08-30T15:44:44Z

This doesn't work. I think it fail trying to build cmis for deprecated module.

ghost · 2018-08-31T10:25:48Z

src/module.mli

+
+val to_sexp : t Sexp.To_sexp.t
+
+val deprecate : t -> t


This should be renamed to wrapped_compat as well

rgrinberg · 2018-08-31T10:53:15Z

@hhugo I tried your branch and it works now

rgrinberg · 2018-08-31T11:04:18Z

@diml addressed your comment and added docs/change log entry.

rgrinberg · 2018-08-31T12:48:08Z

Fixed the tests. I think this is ready.

ghost · 2018-08-31T14:37:43Z

src/lib_rules.ml

         else
           acc)
     in
+     let deprecated_modules = Module.Name.Map.values wrapped_compat in


Just to be consistent, let's use the wrapped_compat terminology here as well. This will make it easier to understand this code in the future

That was just an omission. I fixed this.

ghost · 2018-08-31T14:39:15Z

src/lib_rules.ml

           acc)
     in
+     let deprecated_modules = Module.Name.Map.values wrapped_compat in
+     (* deprecated modules have implementations so we can just append them *)


This comments is not clear to me. The reason is: nothing depends on compatibility modules and compatibility modules depends only on real modules

Actually, my comment referred to the earlier line where we filter only the modules which have implementations. You're right that I should comment about the order as well.

ghost · 2018-08-31T14:48:16Z

src/module.ml

+        let (base, _) = Path.split_extension path in
+        { syntax = OCaml
+        ; path = Path.extend_basename base ~suffix:".ml-gen"
+        }


I think this code does the same thing:

{ syntax = OCaml ; path = Path.L.relative (dir t) [ ".wrapped_compat"; Name.to_string t.name ^ ".ml-gen" ] }

Indeed it is. Fixed.

ghost · 2018-08-31T14:51:46Z

BTW, I'm not sure Path.L.relative is the right name. We have Lib.L that operates on lists of Lib.t, while here Path.L.relative takes a single Path.t. Although I don't have a better suggestion

rgrinberg · 2018-08-31T14:54:41Z

Yeah, Path.L.relative is entirely satisfactory but it does the benefit that a name like relative_all, concat_strings, etc. won't be as memorable.

rgrinberg · 2018-08-31T15:00:26Z

Btw, I added 1.2 as the minimum version of the dune language for this feature

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

Somehow this removes the deprecation Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

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

deprecated modules don't really have interfaces Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

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

hhugo · 2018-08-31T17:12:28Z

Thanks

hhugo · 2018-09-01T08:21:45Z

When do you expect the next release of dune ?

rgrinberg · 2018-09-01T09:35:39Z

I expect to make a release once polling mode is merged.

@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 hhugo August 29, 2018 07:50

rgrinberg requested a review from emillon as a code owner August 29, 2018 07:50

ghost suggested changes Aug 29, 2018

View reviewed changes

rgrinberg force-pushed the wrapped-transition branch from 0417a69 to 74fe613 Compare August 29, 2018 17:21

ghost reviewed Aug 31, 2018

View reviewed changes

src/module.mli Outdated

val to_sexp : t Sexp.To_sexp.t

val deprecate : t -> t

Copy link
Copy Markdown

ghost Aug 31, 2018

Choose a reason for hiding this comment

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

This should be renamed to wrapped_compat as well

rgrinberg force-pushed the wrapped-transition branch from 5942ea3 to f8b2fbb Compare August 31, 2018 11:03

rgrinberg force-pushed the wrapped-transition branch from f8b2fbb to 96ef07a Compare August 31, 2018 11:05

ghost reviewed Aug 31, 2018

View reviewed changes

rgrinberg force-pushed the wrapped-transition branch 2 times, most recently from c82cbaf to 74264e7 Compare August 31, 2018 14:48

ghost suggested changes Aug 31, 2018

View reviewed changes

ghost approved these changes Aug 31, 2018

View reviewed changes

rgrinberg added 3 commits August 31, 2018 19:04

wrapped transition mode

d1d7672

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

Don't hardcode the transition message to be date based

492286b

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

Fix deprecation attribute for transition modules

f9ad608

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

rgrinberg added 14 commits August 31, 2018 19:04

Use proper sum type wrapped.t

d0b937c

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

Use dedicated compilation context for deprecated modules

8acca61

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

Add modules_without_implementation example to wrapped transition mode

f47834d

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

Change terminology to wrapped_compat

c8970ab

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

Handle case when module alias exists

dfd3b10

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

Add .mli to one of transitioned modules

366c102

Somehow this removes the deprecation Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Add Cm_kind.to_sexp conversion

161ca64

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

Set intf to None

ba1b3f8

deprecated modules don't really have interfaces Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Change deprecated modules in a separate directory

868ecbc

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

Add documentation and change log entry

96e3448

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

Fix tests

8449b46

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

Clarifying comment about appending wrapped compat modules

28b311c

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

Simplify code using Module.dir

18abaa7

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

Require dune lang 1.2 for transitioning wraps

3c89b6c

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

rgrinberg force-pushed the wrapped-transition branch from 08b0afb to 3c89b6c Compare August 31, 2018 16:05

rgrinberg merged commit 6da1f19 into ocaml:master Aug 31, 2018

rgrinberg deleted the wrapped-transition branch August 31, 2018 16:47

rgrinberg mentioned this pull request Sep 14, 2018

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

Merged

Conversation

rgrinberg commented Aug 29, 2018

Uh oh!

emillon commented Aug 29, 2018

Uh oh!

ghost commented Aug 29, 2018

Uh oh!

ghost commented Aug 29, 2018

Uh oh!

rgrinberg commented Aug 29, 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!

hhugo commented Aug 29, 2018

Uh oh!

rgrinberg commented Aug 29, 2018

Uh oh!

hhugo commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Aug 31, 2018

Uh oh!

rgrinberg commented Aug 31, 2018

Uh oh!

rgrinberg commented Aug 31, 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 Aug 31, 2018

Uh oh!

rgrinberg commented Aug 31, 2018

Uh oh!

rgrinberg commented Aug 31, 2018

Uh oh!

hhugo commented Aug 31, 2018

Uh oh!

hhugo commented Sep 1, 2018

Uh oh!

rgrinberg commented Sep 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants