Reason support #58

rgrinberg · 2017-04-18T16:27:40Z

Attempt at #53

@diml I've left the renaming commit separate at the end so you can judge if you want it or not.

@jordwalke @IwanKaramazow some testing would be appreciated.

Add transparent support for reason sources. This requires adding a flag to Module.t to distinguish reason and and ocaml sources.

Module.t's ml_fname and mli_fname are no longer good names since Module.t can contain reason modules as well. impl_fname and intf_fname are better names

ghost · 2017-04-18T16:39:05Z

src/gen_rules.ml

+      let ml = Module.ocaml_of_reason re in
+      let refmt =
+        match Context.which ctx "refmt" with
+        | None -> die "refmt not found. Fix with $ opam install reason"


There is a function Utils.program_not_found to raise a standard error, could you use it instead? Additionally, to match other messages, it should print:

Hint: opam install reason

you can add a ?hint argument to Util.program_not_found to do that.

Another issue is that the current code will fail when setting up the rules instead of when evaluating them, which for instance makes external-lib-deps unusable. You can do this instead:

| None -> Build.Prog_spec.Dyn (fun _ -> <raise exception>) | Some refmt -> Dep refmt

ghost · 2017-04-18T16:45:51Z

src/gen_rules.ml

+  (* Generate rules for the reason modules in [modules] and return a list
+     a new list of modules with only OCaml sources *)
+  let reason_rules ~dir ~modules =
+    let rule re =


This should be called rules given that it returns several rules. Alternatively it could also be called setup_rules and call add_rule directly

ghost · 2017-04-18T16:49:42Z

src/gen_rules.ml

-          ; obj_name  = lib.name ^ suf
-          }
+        Some (Module.create ~name:(main_module_name ^ suf)
+                ~impl_fname:(lib.name ^ suf ^ ".ml-gen")


Won't Module.create choke on the .ml-gen extension? I think it would be simpler to just add an is_reason:bool argument to Module.create

I think at that point create becomes useless so I'll just remove it.

ghost · 2017-04-18T16:53:27Z

src/gen_rules.ml

+      | ".re" -> (ml, mli, f :: re, rei)
+      | ".rei" -> (ml, mli, rei, f :: rei)
+      | "" -> (ml, mli, re, rei)
+      | f -> invalid_arg ("Unexpected extension from: " ^ f))


guess_modules is called with all the files in the directory as argument, so it should simply ignore extensions it doesn't recognize.

BTW, the previous code used String.lsplit2 ~on:'.' rather than Filename.extension. This was intentional, it is to filter out filenames such as foo.cppo.ml

👍 I'll leave a comment about this fact too

ghost · 2017-04-18T17:02:04Z

src/gen_rules.ml

+        | Some _, Some _ -> assert false
+      )
+    else
+      die "The following modules have both reason and ocaml sources:@.%a"


I don't think this is going to work really well. For instance, you might want to write a .rei file for a generated lexer. Then you have a module with a reason interface and a ml implementation.

I think it's fine to just keep 2 lists as before, but add a flag to each element telling whether it is reason or ocaml. The existing checks will catch cases when there are both a .ml and .re files

For instance, you might want to write a .rei file for a generated lexer. Then you have a module with a reason interface and a ml implementation

So in this situation a per module reason flag doesn't even make sense anymore. Since we'd allow to mix and match OCaml/Reason interfaces/implemenations? If so, where would this flag exist? Seems like the type of source could be discovered from the intf_fname and impl_fname

Would let to get this and the comment below straight before I make the changes.

Indeed, we could just look at the extension, but I think it would be cleaner to keep separated flags in [impl_fname] and [intf_fname]. We could change the types in module.mli as follow:

module Syntax : sig type t = OCaml | Reason end module File : sig type t = { name : string ; syntax : Syntax.t } end type t = { name : string ; impl : File.t ; intf : File.t option ...

ghost · 2017-04-18T17:14:37Z

src/gen_rules.ml

    let preprocessor_deps = Dep_conf_interpret.dep_of_list ~dir preprocessor_deps in
    String_map.map modules ~f:(fun (m : Module.t) ->
+      assert (not m.reason);


Instead of doing two successive String_map.map, we could replace this line by:

let m = ocaml_of_reason m in

ghost · 2017-04-18T17:21:21Z

src/module.ml

+  if t.reason then
+    { t with
+      reason = false
+    ; impl_fname = Filename.chop_extension t.impl_fname ^ ".ml"


Actually we should just keep the two extensions, i.e. the filename would be: foo.re.ml. The reason for this is that otherwise the generation of the rules is not a fix point: initially guess_modules picks up only foo.re, but if run again it would pickup foo.re and foo.ml. It doesn't matter right now, but it might for the jenga bridge. It might also matter for the implementation of #35.

ghost · 2017-04-18T17:23:56Z

src/module.ml

+                    ~f:(fun mli -> Filename.chop_extension mli ^ ".mli")
+    }
+  else
+    invalid_arg "ocaml_of_reason only expect reason sources"


I tend to use code_errorf (defined in import.ml). The Code_error exception is caught in main.ml and a better error message is printed. It should also include the function name

ghost · 2017-04-18T17:25:01Z

Thanks. Renaming looks good, let's keep it

Note that this still lacks the ability to mix and match reason/ocaml sources

The tag denotes the syntax of the module

rgrinberg · 2017-04-19T17:16:55Z

ok @diml I've tried to address all of your points. Every new commit tries to address one of your points separately except for the last few which tie-in everything together.

I didn't quite get your suggestion as how to avoid the use of 2 successive String_map.map but I succeeded in getting rid of it anyway. Is that what you meant?

The new rules now also allow to mix & match reason/ocaml interfaces/implementations

ghost · 2017-04-19T17:45:18Z

src/module.ml

-  | Intf -> Option.map t.mli_fname ~f:(fun _ -> Path.relative dir (t.obj_name ^ ".cmti"))
+  | Intf -> Option.map t.intf ~f:(fun _ -> Path.relative dir (t.obj_name ^ ".cmti"))
+
+let ocaml_of_reason _t =


This is no longer needed

ghost · 2017-04-19T17:53:33Z

Yep, that's what I meant. Code looks good, appart from the leftover function it's ready to be merged

rgrinberg · 2017-04-19T17:55:05Z

Should we let jordan and co. test it out first?

ghost · 2017-04-19T17:56:02Z

src/module.mli

@@ -16,3 +30,5 @@ val file      : t -> dir:Path.t -> Ml_kind.t -> Path.t option
 val cm_source : t -> dir:Path.t -> Cm_kind.t -> Path.t option
 val cm_file   : t -> dir:Path.t -> Cm_kind.t -> Path.t
 val cmt_file  : t -> dir:Path.t -> Ml_kind.t -> Path.t option
+
+val ocaml_of_reason : t -> t


And this one as well

ghost · 2017-04-19T17:58:06Z

Sure, our we can test it ourselves. BTW I just though of an improvement, instead of the Context.which ctx "refmt", we could use Artifacts.binary "refmt". The difference is that it would allow to use jbuilder for reason itself, since jbuilder would then use the local refmt binary if it is part of the current workspace.

rgrinberg · 2017-04-19T18:00:50Z

Yup. I also tested this, but I don't really have any real reason projects to try it on. They can try this out at their own leisure I suppose.

I'll make that change to use Artifacts.

jordwalke

Wow, thank you @rgrinberg.

jordwalke · 2017-04-20T06:35:33Z

src/gen_rules.ml

+     a new module with only OCaml sources *)
+  let setup_reason_rules ~dir (m : Module.t) =
+    let refmt =
+      match Artifacts.binary "refmt" with


Just to confirm, will this work if refmt is a symlink? It very well might be. Nice touch with the hint to install via opam.

Yes. Basically it is the same as writing ${bin:refmt} in a user rule in a jbuild file:

if refmt is a tool that is installed by the project being compiled, then jbuilder will use resolve ${bin:refmt} to the local binary

otherwise it is looked up in the PATH

Basically this changes nothing for users of reason. What it changes is that if you wanted to use jbuilder to build reason itself, then you could build the tests/examples directly without having to install reason first.

ghost · 2017-04-20T15:40:43Z

Ok, well the patch looks ready to me and has been tested so I'm merging now

jordwalke · 2017-04-21T03:56:50Z

Thanks, everyone! Can't wait to try it out.

rgrinberg added 4 commits April 18, 2017 12:14

Option.both

1bfb796

guess_modules: detect and ignore reason modules for now

414480b

Reason rules

25bd290

Add transparent support for reason sources. This requires adding a flag to Module.t to distinguish reason and and ocaml sources.

Rename .ml{,i}_fname

d87906b

Module.t's ml_fname and mli_fname are no longer good names since Module.t can contain reason modules as well. impl_fname and intf_fname are better names

rgrinberg requested a review from a user April 18, 2017 16:27

ghost suggested changes Apr 18, 2017

View reviewed changes

ghost reviewed Apr 18, 2017

View reviewed changes

rgrinberg added 9 commits April 18, 2017 16:17

change rule to setup_rules

1a81322

add ?hint argument to program_not_found

bf324aa

Signal missing with refmt with Utils.program_not_found

de74be8

Don't fail on absent refmt when setting up build rules

3c35ee9

Avoid doing String_map.map when processing reason rules

31c5e7b

split_to_modules should use String.lsplit2 like it used to

fdee6af

Tag sources with reason/ocaml when guessing modules

24bbdc0

Note that this still lacks the ability to mix and match reason/ocaml sources

Remove Module.create

50aff9f

Change module intf/impl to be tagged files

5721612

The tag denotes the syntax of the module

Modify reason rules to use source tags

760f52e

The new rules now also allow to mix & match reason/ocaml interfaces/implementations

rgrinberg force-pushed the reason branch from 99017b1 to 760f52e Compare April 19, 2017 17:18

ghost suggested changes Apr 19, 2017

View reviewed changes

remove unused function

98b2247

rgrinberg force-pushed the reason branch from 6eea6a9 to 98b2247 Compare April 19, 2017 17:56

ghost approved these changes Apr 19, 2017

View reviewed changes

use Artifacts.binary

b062943

jordwalke reviewed Apr 20, 2017

View reviewed changes

ghost merged commit 66f973c into ocaml:master Apr 20, 2017

rgrinberg deleted the reason branch April 20, 2017 15:50

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reason support #58

Reason support #58

rgrinberg commented Apr 18, 2017 •

edited

Loading

ghost Apr 18, 2017

ghost Apr 18, 2017

ghost Apr 18, 2017

rgrinberg Apr 18, 2017

ghost Apr 18, 2017

rgrinberg Apr 18, 2017

ghost Apr 18, 2017

rgrinberg Apr 19, 2017

ghost Apr 19, 2017

ghost Apr 18, 2017

ghost Apr 18, 2017

ghost Apr 18, 2017

ghost commented Apr 18, 2017

rgrinberg commented Apr 19, 2017

ghost Apr 19, 2017

ghost commented Apr 19, 2017

rgrinberg commented Apr 19, 2017

ghost Apr 19, 2017

ghost commented Apr 19, 2017

rgrinberg commented Apr 19, 2017 •

edited

Loading

jordwalke left a comment

jordwalke Apr 20, 2017

ghost Apr 20, 2017

ghost commented Apr 20, 2017

jordwalke commented Apr 21, 2017

Reason support #58

Reason support #58

Conversation

rgrinberg commented Apr 18, 2017 • 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

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

ghost commented Apr 18, 2017

rgrinberg commented Apr 19, 2017

Choose a reason for hiding this comment

ghost commented Apr 19, 2017

rgrinberg commented Apr 19, 2017

Choose a reason for hiding this comment

ghost commented Apr 19, 2017

rgrinberg commented Apr 19, 2017 • edited Loading

jordwalke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 20, 2017

jordwalke commented Apr 21, 2017

rgrinberg commented Apr 18, 2017 •

edited

Loading

rgrinberg commented Apr 19, 2017 •

edited

Loading