Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start of support for languages and extensions [Part1] #874

Merged
4 commits merged into from Jun 12, 2018
Merged

Start of support for languages and extensions [Part1] #874

4 commits merged into from Jun 12, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jun 11, 2018

This PR is the first step for supporting (using ...) in dune-project files. Support is mostly there, however I haven't done the last part which is moving the parsing of menhir stanzas to menhir.ml and the necessary boilerplate that goes with it as this PR is starting to be big already.

Code changes

This PR does the following:

  • it moves the selection of what stanzas are available in dune files to dune_project.ml, since the language and extensions are defined in the dune-project file
  • it changes what stanzas are available in jbuild and dune files:
    • jbuild_version is no longer recognized in dune files
    • env is only available in dune files
  • it adds a mechanism for editing the dune-project file (see next) section
  • it implement the parsing of (using ...) stanzas
  • it update the tests to add missing dune-project files and remove old jbuild_version stanzas

Automatic edition of dune-project files

Since the language is now properly versioned, we need the user to write a dune-project file with:

  • the version of the dune language used: (lang dune 1.0)
  • the version of each extension used: (using menhir 1.0)

While this information is necessary, I think it's going to be annoying to have to manually write these lines over and over in every project. Moreover, if the user try to use a stanza from an extension, such as the menhir stanza, then the first step will always be to add (using menhir XX) to the dune-project file. So rather that having dune fail or suggest to do that, I think it's simpler to just do it.

After this PR, if a user adds a dune file to a jbuilder project or a new workspace, then dune will automatically create the dune-project file:

$ mkdir -p project/src
$ cd project
$ touch src/dune
$ dune build
Info: creating file dune-project with this contents: (lang dune 1.0)
$ cat dune-project
(lang dune 1.0)

and when the second part of this work is imlemented:

$ echo '(menhir ((modules (x)))' >> src/dune
$ dune build
Info: appending this line to dune-project: (using menhir 1.0)

@ghost ghost requested a review from rgrinberg June 11, 2018 16:35
@ghost
Copy link
Author

ghost commented Jun 11, 2018

I rebased this PR on top of #875 to minimize the diff.

@ejgallego
Copy link
Collaborator

That's great!

I will try to give more complete feedback soon, but I do believe that in order to add compilation of Coq files to Dune the only tricky part is that we would like to have an stanza which can affect whole subtrees.

Coq is organized a bit like Haskell in that sense, so you tend to have A/B/foo.v A/C/bar.v and you would ideally like to write a library stanza in A/dune with some (recursive true) flag such that the A library is including both A.B and A.C.

Just my 2cts.

@rgrinberg
Copy link
Member

@ejgallego some stanzas can already affect whole sub trees (env for example).

Btw, we're thinking of adding a similar feature to dune defined libraries themselves. A file system defined module hierarchy that dune will use module aliases to compile. E.g. module foo/bar.ml in lib baz would be Baz.Foo.Bar where the compilation unit would be Baz__Foo__Bar.

@rgrinberg
Copy link
Member

rgrinberg commented Jun 12, 2018

Why did you make stanza_parser mutable? You don't like using mutual recursion like this:

diff --git a/src/dune_project.ml b/src/dune_project.ml
index 1276e5e2..71683743 100644
--- a/src/dune_project.ml
+++ b/src/dune_project.ml
@@ -117,7 +117,7 @@ type t =
   ; root                  : Path.t
   ; version               : string option
   ; packages              : Package.t Package.Name.Map.t
-  ; mutable stanza_parser : Stanza.t list Sexp.Of_sexp.t
+  ; stanza_parser         : Stanza.t list Sexp.Of_sexp.t
   ; mutable project_file  : Path.t option
   }
 
@@ -196,19 +196,21 @@ end
 
 let filename = "dune-project"
 
-let anonymous = lazy(
-  let t =
+let anonymous =
+  let rec t = lazy (
     { kind          = Dune
     ; name          = Name.anonymous_root
     ; packages      = Package.Name.Map.empty
     ; root          = Path.root
     ; version       = None
-    ; stanza_parser = (fun _ -> assert false)
+    ; stanza_parser
     ; project_file  = None
     }
+  )
+  and stanza_parser sexp =
+    Sexp.Of_sexp.sum (snd (Lang.latest "dune") (Lazy.force t)) sexp
   in
-  t.stanza_parser <- Sexp.Of_sexp.sum (snd (Lang.latest "dune") t);
-  t)
+  t
 
 let default_name ~dir ~packages =
   match Package.Name.Map.choose packages with
@@ -245,18 +247,19 @@ let parse ~dir ~lang_stanzas ~packages ~file =
        (fun (loc, name) ver args_loc args ->
           (name, (loc, ver, Sexp.Ast.List (args_loc, args))))
      >>= fun extensions ->
-     let t =
+     let rec t =
        { kind = Dune
        ; name
        ; root = dir
        ; version
        ; packages
-       ; stanza_parser = (fun _ -> assert false)
+       ; stanza_parser
        ; project_file  = Some file
        }
+     and extensions_stanzas = lazy (Extension.parse t extensions)
+     and stanza_parser sexp =
+       Sexp.Of_sexp.sum (lang_stanzas t @ (Lazy.force extensions_stanzas)) sexp
      in
-     let extenstions_stanzas = Extension.parse t extensions in
-     t.stanza_parser <- Sexp.Of_sexp.sum (lang_stanzas t @ extenstions_stanzas);
      return t)
 
 let load_dune_project ~dir packages =
@@ -267,17 +270,17 @@ let load_dune_project ~dir packages =
     parse ~dir ~lang_stanzas ~packages ~file:fname sexp)
 
 let make_jbuilder_project ~dir packages =
-  let t =
+  let rec t =
     { kind = Jbuilder
     ; name = default_name ~dir ~packages
     ; root = dir
     ; version = None
     ; packages
-    ; stanza_parser = (fun _ -> assert false)
+    ; stanza_parser
     ; project_file = None
     }
-  in
-  t.stanza_parser <- Sexp.Of_sexp.sum (snd (Lang.latest "dune") t);
+  and stanza_parser sexp =
+    Sexp.Of_sexp.sum (snd (Lang.latest "dune") t) sexp in
   t
 
 let load ~dir ~files =
diff --git a/src/dune_project.mli b/src/dune_project.mli
index b413c268..1e57a0fd 100644
--- a/src/dune_project.mli
+++ b/src/dune_project.mli
@@ -36,7 +36,7 @@ type t =
   ; root                  : Path.t
   ; version               : string option
   ; packages              : Package.t Package.Name.Map.t
-  ; mutable stanza_parser : Stanza.t list Sexp.Of_sexp.t
+  ; stanza_parser         : Stanza.t list Sexp.Of_sexp.t
   ; mutable project_file  : Path.t option
   }

Everything else looks good to me, but I wish we were a bit more careful about writing stuff inside the user's source dir. For example, if a user could promote the project file we're suggesting to him, I think we'd be in a better position to implement more of these automatic assists.

@rgrinberg
Copy link
Member

Btw, the tests need to be updated.

Jeremie Dimino and others added 4 commits June 12, 2018 10:36
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
To ensure we can't mutate the mutable fields and that the value is
shared, which is important for the profile_file field for instance.

To make sure we don't confuse the root field for a path in the build
directory, change its type to Path.Local.t.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
There is no risk of confusion about the interpretation of the root
field anymore since it has type Path.Local.t.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Author

ghost commented Jun 12, 2018

Why did you make stanza_parser mutable? You don't like using mutual recursion like this:

This code would do the name lookup for every stanza, which is not optimal. It should be possible to make it work by making the stanza_parser field be lazy, but then we don't win much. I made Dune_project.t private, which gives us the guarantee that the fields can't be mutated from outside of the module.

While I was at at it I also changed the type of root to be Path.Local.t to be sure we don't confuse it for a path in the build directory. This allows to reuse the Dune_project.t value without having to change the root field.

@ghost
Copy link
Author

ghost commented Jun 12, 2018

Everything else looks good to me, but I wish we were a bit more careful about writing stuff inside the user's source dir. For example, if a user could promote the project file we're suggesting to him, I think we'd be in a better position to implement more of these automatic assists.

I thought about doing something like this, however the next step would always be to accept the correction. So we would just be adding a dummy step. It seems better to me to let the computer do it for us. We can always make this behavior configurable via the ~/.config/dune/config file.

@rgrinberg
Copy link
Member

I thought about doing something like this, however the next step would always be to accept the correction. So we would just be adding a dummy step. It seems better to me to let the computer do it for us. We can always make this behavior configurable via the ~/.config/dune/config file.

I can imagine a few cases where I wouldn't want to accept the correction. For example, I've cloned an external repo and just want to build an install it and it's annoying to $ git status polluted as a result. Anyways, this is still a minor objection.

@rgrinberg
Copy link
Member

This code would do the name lookup for every stanza, which is not optimal. It should be possible to make it work by making the stanza_parser field be lazy, but then we don't win much. I made Dune_project.t private, which gives us the guarantee that the fields can't be mutated from outside of the module.

I see. Let's leave things as is.

@rgrinberg
Copy link
Member

While I was at at it I also changed the type of root to be Path.Local.t to be sure we don't confuse it for a path in the build directory. This allows to reuse the Dune_project.t value without having to change the root field.

That's a good change. Why do we need the entire project in Jbuild.Library.t btw? All we use the is the name.

@ghost
Copy link
Author

ghost commented Jun 12, 2018

That's a good change. Why do we need the entire project in Jbuild.Library.t btw? All we use the is the name.

It just seems simpler to have the project at hand directly, otherwise if you want the project you need an additional lookup. While we had two instances of Dune_project.t values, it seemed dodgy to keep around the non-prefixed instance pointing to the source tree together with the prefixed one as it's easy to mix them up. But it's not a problem anymore.

@ghost ghost merged commit 86b1305 into ocaml:master Jun 12, 2018
@ghost ghost mentioned this pull request Jun 13, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants