From 60ad83c52246b735a4808f581d697d7545259d6c Mon Sep 17 00:00:00 2001
From: Jeremie Dimino <jdimino@janestreet.com>
Date: Tue, 16 May 2017 14:47:52 +0100
Subject: [PATCH] Improve opam parsing errors & use opam-file-format to extract
 the version

---
 bootstrap.ml       |  8 ++++++--
 src/import.ml      | 16 ++++++++--------
 src/jbuild_load.ml | 22 ++++++++++------------
 src/opam_file.ml   | 22 ++++++++++++++++++++++
 src/opam_file.mli  | 12 ++++++++++++
 src/watermarks.ml  | 10 ++--------
 6 files changed, 60 insertions(+), 30 deletions(-)
 create mode 100644 src/opam_file.ml
 create mode 100644 src/opam_file.mli

diff --git a/bootstrap.ml b/bootstrap.ml
index c40f54d1..778518f0 100644
--- a/bootstrap.ml
+++ b/bootstrap.ml
@@ -281,14 +281,18 @@ module Jbuilder_opam_file_format = struct
       ; file_name     : string
       }
   end
-  module OpamParser = struct
+  module OpamBaseParser = struct
     open OpamParserTypes
-    let file fn =
+    let main _lex _lexbuf fn =
       assert (fn = "jbuilder.opam");
       { file_contents = []
       ; file_name     = fn
       }
   end
+  module OpamLexer = struct
+    exception Error of string
+    let token _ = assert false
+  end
 end
 
 module Glob_lexer = struct
diff --git a/src/import.ml b/src/import.ml
index 9bd3c74a..8a566d7d 100644
--- a/src/import.ml
+++ b/src/import.ml
@@ -407,14 +407,14 @@ let with_file_out ?(binary=true)fn ~f =
 
 let with_lexbuf_from_file fn ~f =
   with_file_in fn ~f:(fun ic ->
-      let lb = Lexing.from_channel ic in
-      lb.lex_curr_p <-
-        { pos_fname = fn
-        ; pos_lnum  = 1
-        ; pos_bol   = 0
-        ; pos_cnum  = 0
-        };
-      f lb)
+    let lb = Lexing.from_channel ic in
+    lb.lex_curr_p <-
+      { pos_fname = fn
+      ; pos_lnum  = 1
+      ; pos_bol   = 0
+      ; pos_cnum  = 0
+      };
+    f lb)
 
 let input_lines =
   let rec loop ic acc =
diff --git a/src/jbuild_load.ml b/src/jbuild_load.ml
index 06d7515a..ca76913d 100644
--- a/src/jbuild_load.ml
+++ b/src/jbuild_load.ml
@@ -149,18 +149,16 @@ let load ?(extra_ignored_subtrees=Path.Set.empty) () =
           match Filename.split_extension fn with
           | (pkg, ".opam") when pkg <> "" ->
             let version_from_opam_file =
-              let lines = lines_of_file (Path.relative path fn |> Path.to_string) in
-              List.find_map lines ~f:(fun s ->
-                try
-                Scanf.sscanf s "version: %S" (fun x -> Some x)
-              with _ ->
-                None)
-          in
-          (pkg,
-           { Package. name = pkg
-           ; path
-           ; version_from_opam_file
-           }) :: acc
+              let opam = Opam_file.load (Path.relative path fn |> Path.to_string) in
+              match Opam_file.get_field opam "version" with
+              | Some (String (_, s)) -> Some s
+              | _ -> None
+            in
+            (pkg,
+             { Package. name = pkg
+             ; path
+             ; version_from_opam_file
+             }) :: acc
           | _ -> acc)
       in
       if String_set.mem "jbuild-ignore" files then
diff --git a/src/opam_file.ml b/src/opam_file.ml
new file mode 100644
index 00000000..87907e7c
--- /dev/null
+++ b/src/opam_file.ml
@@ -0,0 +1,22 @@
+open Import
+open Jbuilder_opam_file_format
+open OpamParserTypes
+
+type t = opamfile
+
+let load fn =
+  with_lexbuf_from_file fn ~f:(fun lb ->
+    try
+      OpamBaseParser.main OpamLexer.token lb fn
+    with
+    | OpamLexer.Error msg ->
+      Loc.fail_lex lb "%s" msg
+    | Parsing.Parse_error ->
+      Loc.fail_lex lb "Parse error")
+
+let get_field t name =
+  List.find_map t.file_contents
+    ~f:(function
+      | Variable (_, var, value) when name = var ->
+        Some value
+      | _ -> None)
diff --git a/src/opam_file.mli b/src/opam_file.mli
new file mode 100644
index 00000000..764c9360
--- /dev/null
+++ b/src/opam_file.mli
@@ -0,0 +1,12 @@
+(** Parsing and interpretation of opam files *)
+
+open Jbuilder_opam_file_format.OpamParserTypes
+
+(** Type of opam files *)
+type t = opamfile
+
+(** Load a file *)
+val load : string -> t
+
+(** Extracts a field *)
+val get_field : t -> string -> value option
diff --git a/src/watermarks.ml b/src/watermarks.ml
index e614a8f9..d356022c 100644
--- a/src/watermarks.ml
+++ b/src/watermarks.ml
@@ -21,7 +21,7 @@ let is_a_source_file fn =
   | _ -> true
 
 let make_watermark_map ~name ~version ~commit =
-  let opam_file = OpamParser.file (name ^ ".opam") in
+  let opam_file = Opam_file.load (name ^ ".opam") in
   let version_num =
     if String.is_prefix version ~prefix:"v" then
       String.sub version ~pos:1 ~len:(String.length version - 1)
@@ -29,13 +29,7 @@ let make_watermark_map ~name ~version ~commit =
       version
   in
   let opam_var name sep =
-    match
-      List.find_map opam_file.file_contents
-        ~f:(function
-          | Variable (_, var, value) when name = var ->
-            Some value
-          | _ -> None)
-    with
+    match Opam_file.get_field opam_file name with
     | None -> Error (sprintf "variable %S not found in opam file" name)
     | Some value ->
       let err = Error (sprintf "invalid value for variable %S in opam file" name) in