From 67c6de486458e6a9be53852674670fcfcdd1987f Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 11:48:10 +0300
Subject: [PATCH 01/12] Add test for #1102

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 .../no-name-field/public-name-invalid-name/c.opam      |  0
 .../no-name-field/public-name-invalid-name/dune        |  1 +
 .../public-name-invalid-name/dune-project              |  1 +
 test/blackbox-tests/test-cases/no-name-field/run.t     | 10 ++++++++++
 4 files changed, 12 insertions(+)
 create mode 100644 test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/c.opam
 create mode 100644 test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune
 create mode 100644 test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune-project

diff --git a/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/c.opam b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/c.opam
new file mode 100644
index 00000000..e69de29b
diff --git a/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune
new file mode 100644
index 00000000..af6d3200
--- /dev/null
+++ b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune
@@ -0,0 +1 @@
+(library (public_name c.find))
\ No newline at end of file
diff --git a/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune-project b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune-project
new file mode 100644
index 00000000..6687faf2
--- /dev/null
+++ b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-name/dune-project
@@ -0,0 +1 @@
+(lang dune 1.1)
\ No newline at end of file
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index e22bf93a..8723a7c6 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -17,3 +17,13 @@ executable(s) stanza works the same way
   File "dune", line 1, characters 0-36:
   Error: name field may not be omitted before dune version 1.1
   [1]
+
+there's only a public name but it's invalid as a name
+
+  $ dune build --root public-name-invalid-name
+  Entering directory 'public-name-invalid-name'
+      ocamlopt .c.find.objs/c.find.{cmx,o} (exit 2)
+  (cd _build/default && /Users/rgrinberg/.opam/4.06.1/bin/ocamlopt.opt -w @a-4-29-40-41-42-44-45-48-58-59-60-40 -strict-sequence -strict-formats -short-paths -keep-locs -w -49 -g -I .c.find.objs -intf-suffix .ml-gen -no-alias-deps -o .c.find.objs/c.find.cmx -c -impl c.find.ml-gen)
+  File "c.find.ml-gen", line 1:
+  Error: Could not find the .cmi file for interface c.find.ml-gen.
+  [1]

From 6260dad66bea3df5444838aa3f6dcadd55d34ca2 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 11:49:01 +0300
Subject: [PATCH 02/12] Remove comment that was comitted accidentally

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index 7d35c670..35aee96f 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -1107,13 +1107,7 @@ module Executables = struct
     else
       s ^ "s"
 
-  let common
-    (* :  (Loc.t * string) list option
-     * -> (Loc.t * string) list option
-     * -> multi:bool
-     * -> unit
-     * -> t * Install_conf.t option Sexp.Of_sexp.t *)
-    =
+  let common =
     let%map buildable = Buildable.t
     and (_ : bool) = field "link_executables" ~default:true
                        (Syntax.deleted_in Stanza.syntax (1, 0) >>> bool)

From 317388fd95993aff02b255e584303a60796699a1 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 12:07:16 +0300
Subject: [PATCH 03/12] Refactor code to to have a dedicated type for
 Library.name

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                 | 35 +++++++++++++------
 .../test-cases/no-name-field/run.t            |  2 +-
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index 35aee96f..855b4fc4 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -37,21 +37,36 @@ let module_name =
 
 let module_names = list module_name >>| String.Set.of_list
 
-let invalid_lib_name ~loc = of_sexp_errorf loc "invalid library name"
+module Lib_name : sig
+  type t
 
-let library_name =
-  plain_string (fun ~loc name ->
+  val to_string : t -> string
+
+  val of_string : loc:Loc.t -> string -> t
+
+  val t : t Sexp.Of_sexp.t
+end = struct
+  type t = string
+
+  let invalid ~loc = of_sexp_errorf loc "invalid library name"
+
+  let to_string s = s
+
+  let of_string ~loc name =
     match name with
-    | "" -> invalid_lib_name ~loc
+    | "" -> invalid ~loc
     | s ->
-      if s.[0] = '.' then invalid_lib_name ~loc
+      if s.[0] = '.' then invalid ~loc
       else
         try
           String.iter s ~f:(function
             | 'A'..'Z' | 'a'..'z' | '_' | '.' | '0'..'9' -> ()
             | _ -> raise_notrace Exit);
           s
-        with Exit -> invalid_lib_name ~loc)
+        with Exit -> invalid ~loc
+
+  let t = plain_string of_string
+end
 
 let file =
   plain_string (fun ~loc s ->
@@ -868,7 +883,7 @@ module Library = struct
     record
       (let%map buildable = Buildable.t
        and loc = loc
-       and name = field_o "name" library_name
+       and name = field_o "name" Lib_name.t
        and public = Public_lib.public_name_field
        and synopsis = field_o "synopsis" string
        and install_c_headers =
@@ -900,9 +915,9 @@ module Library = struct
        let name =
          match name, public with
          | Some n, _ -> n
-         | None, Some { name = (_loc, name) ; _ }  ->
+         | None, Some { name = (loc, name) ; _ }  ->
            if dune_version >= (1, 1) then
-             name
+             Lib_name.of_string ~loc name
            else
              of_sexp_error loc "name field cannot be omitted before version \
                                 1.1 of the dune language"
@@ -914,7 +929,7 @@ module Library = struct
                "name field is missing"
            )
        in
-       { name
+       { name = Lib_name.to_string name
        ; public
        ; synopsis
        ; install_c_headers
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index 8723a7c6..aecbd8de 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -4,7 +4,7 @@ the name field can be omitted for libraries when public_name is present
 
 this isn't possible for older syntax <= (1, 0)
   $ dune build --root no-name-lib-syntax-1-0
-  File "dune", line 1, characters 0-27:
+  File "dune", line 1, characters 22-25:
   Error: name field cannot be omitted before version 1.1 of the dune language
   [1]
 

From 131310d144d29a7bcd02e64460114eedb2c2f80c Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 12:08:22 +0300
Subject: [PATCH 04/12] Fix validation of library names

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                      | 2 +-
 test/blackbox-tests/test-cases/no-name-field/run.t | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index 855b4fc4..b7edadcd 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -60,7 +60,7 @@ end = struct
       else
         try
           String.iter s ~f:(function
-            | 'A'..'Z' | 'a'..'z' | '_' | '.' | '0'..'9' -> ()
+            | 'A'..'Z' | 'a'..'z' | '_' | '0'..'9' -> ()
             | _ -> raise_notrace Exit);
           s
         with Exit -> invalid ~loc
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index aecbd8de..4d01263b 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -21,9 +21,6 @@ executable(s) stanza works the same way
 there's only a public name but it's invalid as a name
 
   $ dune build --root public-name-invalid-name
-  Entering directory 'public-name-invalid-name'
-      ocamlopt .c.find.objs/c.find.{cmx,o} (exit 2)
-  (cd _build/default && /Users/rgrinberg/.opam/4.06.1/bin/ocamlopt.opt -w @a-4-29-40-41-42-44-45-48-58-59-60-40 -strict-sequence -strict-formats -short-paths -keep-locs -w -49 -g -I .c.find.objs -intf-suffix .ml-gen -no-alias-deps -o .c.find.objs/c.find.cmx -c -impl c.find.ml-gen)
-  File "c.find.ml-gen", line 1:
-  Error: Could not find the .cmi file for interface c.find.ml-gen.
+  File "dune", line 1, characters 22-28:
+  Error: invalid library name
   [1]

From 31e4f6f18ac4ff88a65de51a9eabe96d8cfc655b Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 13:02:17 +0300
Subject: [PATCH 05/12] Fix error messages for invalid lib names

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                 | 37 ++++++++++++++-----
 .../test-cases/no-name-field/run.t            |  4 +-
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index b7edadcd..3859512c 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -40,32 +40,43 @@ let module_names = list module_name >>| String.Set.of_list
 module Lib_name : sig
   type t
 
+  val error_message : string
+
   val to_string : t -> string
 
-  val of_string : loc:Loc.t -> string -> t
+  val of_string : string -> t option
 
   val t : t Sexp.Of_sexp.t
 end = struct
   type t = string
 
-  let invalid ~loc = of_sexp_errorf loc "invalid library name"
+  let error_message =
+    "Error: invalid library name\n\
+     Hint: library names must be non-empty and composed only of \
+     the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'"
+
+  let invalid ~loc = of_sexp_error loc error_message
 
   let to_string s = s
 
-  let of_string ~loc name =
+  let of_string name =
     match name with
-    | "" -> invalid ~loc
+    | "" -> None
     | s ->
-      if s.[0] = '.' then invalid ~loc
+      if s.[0] = '.' then
+        None
       else
         try
           String.iter s ~f:(function
             | 'A'..'Z' | 'a'..'z' | '_' | '0'..'9' -> ()
             | _ -> raise_notrace Exit);
-          s
-        with Exit -> invalid ~loc
+          Some s
+        with Exit -> None
 
-  let t = plain_string of_string
+  let t = plain_string (fun ~loc s ->
+    match of_string s with
+    | Some n -> n
+    | None -> invalid ~loc)
 end
 
 let file =
@@ -917,7 +928,15 @@ module Library = struct
          | Some n, _ -> n
          | None, Some { name = (loc, name) ; _ }  ->
            if dune_version >= (1, 1) then
-             Lib_name.of_string ~loc name
+             match Lib_name.of_string name with
+             | Some n -> n
+             | None ->
+               of_sexp_errorf loc
+                 "%s.\n\
+                  Public library names don't have this restriction. \
+                  You can either change this public name to be a valid \
+                  library name or add a `name` field with a valid library name."
+                 Lib_name.error_message
            else
              of_sexp_error loc "name field cannot be omitted before version \
                                 1.1 of the dune language"
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index 4d01263b..86b8a2d2 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -22,5 +22,7 @@ there's only a public name but it's invalid as a name
 
   $ dune build --root public-name-invalid-name
   File "dune", line 1, characters 22-28:
-  Error: invalid library name
+  Error: Error: invalid library name
+  Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
+  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a `name` field with a valid library name.
   [1]

From 272012ea5c96489d6b1fdc74c4abec4ef0092324 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 13:08:09 +0300
Subject: [PATCH 06/12] Update CHANGELOG

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 CHANGES.md | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/CHANGES.md b/CHANGES.md
index a28e1a7c..bdee80ca 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -9,8 +9,12 @@ next
 - Fix duplicate profile argument in suggested command when an external library
   is missing (#1109, #1106, @emillon)
 
-- Fix #1107. `-opaque` wasn't correctly being added to modules without
-  an interface. (#1108, fix #1107, @rgrinberg)
+- `-opaque` wasn't correctly being added to modules without an interface.
+  (#1108, fix #1107, @rgrinberg)
+
+- Fix validation of library `name` fields and make sure this validation also
+  applies when the `name` is derived from the `public_name`. (#1110, fix #1102,
+  @rgrinberg)
 
 1.1.0 (06/08/2018)
 ------------------

From 925bc8442702c42ddf18ecfe7f5bc35f3b4634df Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Tue, 7 Aug 2018 19:53:51 +0300
Subject: [PATCH 07/12] Fix error message

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                      | 6 +++---
 test/blackbox-tests/test-cases/no-name-field/run.t | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index 3859512c..bdbd5c13 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -51,7 +51,7 @@ end = struct
   type t = string
 
   let error_message =
-    "Error: invalid library name\n\
+    "invalid library name.\n\
      Hint: library names must be non-empty and composed only of \
      the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'"
 
@@ -934,8 +934,8 @@ module Library = struct
                of_sexp_errorf loc
                  "%s.\n\
                   Public library names don't have this restriction. \
-                  You can either change this public name to be a valid \
-                  library name or add a `name` field with a valid library name."
+                  You can either change this public name to be a valid library \
+                  name or add a \"name\" field with a valid library name."
                  Lib_name.error_message
            else
              of_sexp_error loc "name field cannot be omitted before version \
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index 86b8a2d2..563dbccd 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -22,7 +22,7 @@ there's only a public name but it's invalid as a name
 
   $ dune build --root public-name-invalid-name
   File "dune", line 1, characters 22-28:
-  Error: Error: invalid library name
+  Error: invalid library name.
   Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
-  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a `name` field with a valid library name.
+  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a "name" field with a valid library name.
   [1]

From c08cec1f4b0a65a8fdc1e8fafc911e8eea4ef70d Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Wed, 8 Aug 2018 10:59:12 +0300
Subject: [PATCH 08/12] Add test case for name when it doesn't matter

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 .../public-name-invalid-wrapped-false/dune            |  3 +++
 .../public-name-invalid-wrapped-false/foo.ml          |  0
 .../public-name-invalid-wrapped-false/foo.opam        |  0
 test/blackbox-tests/test-cases/no-name-field/run.t    | 11 +++++++++++
 4 files changed, 14 insertions(+)
 create mode 100644 test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/dune
 create mode 100644 test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/foo.ml
 create mode 100644 test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/foo.opam

diff --git a/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/dune b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/dune
new file mode 100644
index 00000000..9bc81433
--- /dev/null
+++ b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/dune
@@ -0,0 +1,3 @@
+(library
+ (wrapped false)
+ (public_name foo.bar))
\ No newline at end of file
diff --git a/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/foo.ml b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/foo.ml
new file mode 100644
index 00000000..e69de29b
diff --git a/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/foo.opam b/test/blackbox-tests/test-cases/no-name-field/public-name-invalid-wrapped-false/foo.opam
new file mode 100644
index 00000000..e69de29b
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index 563dbccd..a25f890b 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -26,3 +26,14 @@ there's only a public name but it's invalid as a name
   Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
   Public library names don't have this restriction. You can either change this public name to be a valid library name or add a "name" field with a valid library name.
   [1]
+
+there's only a public name which is invalid, but sine the library is unwrapped,
+it's just a warning
+
+  $ dune build --root public-name-invalid-wrapped-false
+  Info: creating file dune-project with this contents: (lang dune 1.1)
+  File "dune", line 3, characters 14-21:
+  Error: invalid library name.
+  Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
+  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a "name" field with a valid library name.
+  [1]

From 5706e4ee56a0f749bce7f3e449ae0910b4e97e7f Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Wed, 8 Aug 2018 11:06:14 +0300
Subject: [PATCH 09/12] Make invalid public name warn when (wrapped false)

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                  | 18 +++++++++++++-----
 .../test-cases/no-name-field/run.t             |  7 ++++---
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index bdbd5c13..269bb0b2 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -925,12 +925,20 @@ module Library = struct
        in
        let name =
          match name, public with
-         | Some n, _ -> n
+         | Some n, _ -> Lib_name.to_string n
          | None, Some { name = (loc, name) ; _ }  ->
            if dune_version >= (1, 1) then
-             match Lib_name.of_string name with
-             | Some n -> n
-             | None ->
+             match Lib_name.of_string name, wrapped with
+             | Some n, _ -> Lib_name.to_string n
+             | None, false ->
+               Loc.warn loc
+                 "%s.\n\
+                 This is temporary allowed for libraries with (wrapped false).\
+                  \nIt will not be supported in the future. \
+                  Please choose a valid name field."
+                 Lib_name.error_message;
+               name
+             | None, true ->
                of_sexp_errorf loc
                  "%s.\n\
                   Public library names don't have this restriction. \
@@ -948,7 +956,7 @@ module Library = struct
                "name field is missing"
            )
        in
-       { name = Lib_name.to_string name
+       { name
        ; public
        ; synopsis
        ; install_c_headers
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index a25f890b..e35fbc5d 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -33,7 +33,8 @@ it's just a warning
   $ dune build --root public-name-invalid-wrapped-false
   Info: creating file dune-project with this contents: (lang dune 1.1)
   File "dune", line 3, characters 14-21:
-  Error: invalid library name.
+  Warning: invalid library name.
   Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
-  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a "name" field with a valid library name.
-  [1]
+  This is temporary allowed for libraries with (wrapped false).
+  It will not be supported in the future. Please choose a valid name field.
+  Entering directory 'public-name-invalid-wrapped-false'

From ca7696f2c3fb224f643d4ead8a5d157fd10efda2 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Wed, 8 Aug 2018 13:21:03 +0300
Subject: [PATCH 10/12] Add test case for a library with an invalid name

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 test/blackbox-tests/dune.inc                           | 10 ++++++++++
 .../test-cases/name-field-validation/bar.ml            |  1 +
 .../test-cases/name-field-validation/dune              |  9 +++++++++
 .../test-cases/name-field-validation/dune-project      |  1 +
 .../test-cases/name-field-validation/foo.ml            |  1 +
 .../test-cases/name-field-validation/run.t             |  5 +++++
 6 files changed, 27 insertions(+)
 create mode 100644 test/blackbox-tests/test-cases/name-field-validation/bar.ml
 create mode 100644 test/blackbox-tests/test-cases/name-field-validation/dune
 create mode 100644 test/blackbox-tests/test-cases/name-field-validation/dune-project
 create mode 100644 test/blackbox-tests/test-cases/name-field-validation/foo.ml
 create mode 100644 test/blackbox-tests/test-cases/name-field-validation/run.t

diff --git a/test/blackbox-tests/dune.inc b/test/blackbox-tests/dune.inc
index b8c3cdae..936b55b0 100644
--- a/test/blackbox-tests/dune.inc
+++ b/test/blackbox-tests/dune.inc
@@ -490,6 +490,14 @@
     (run %{exe:cram.exe} -skip-versions 4.02.3 -test run.t)
     (diff? run.t run.t.corrected)))))
 
+(alias
+ (name name-field-validation)
+ (deps (package dune) (source_tree test-cases/name-field-validation))
+ (action
+  (chdir
+   test-cases/name-field-validation
+   (progn (run %{exe:cram.exe} -test run.t) (diff? run.t run.t.corrected)))))
+
 (alias
  (name no-installable-mode)
  (deps (package dune) (source_tree test-cases/no-installable-mode))
@@ -818,6 +826,7 @@
   (alias missing-loc-run)
   (alias multi-dir)
   (alias multiple-private-libs)
+  (alias name-field-validation)
   (alias no-installable-mode)
   (alias no-name-field)
   (alias null-dep)
@@ -908,6 +917,7 @@
   (alias misc)
   (alias missing-loc-run)
   (alias multi-dir)
+  (alias name-field-validation)
   (alias no-installable-mode)
   (alias no-name-field)
   (alias null-dep)
diff --git a/test/blackbox-tests/test-cases/name-field-validation/bar.ml b/test/blackbox-tests/test-cases/name-field-validation/bar.ml
new file mode 100644
index 00000000..c392d87c
--- /dev/null
+++ b/test/blackbox-tests/test-cases/name-field-validation/bar.ml
@@ -0,0 +1 @@
+Foo.run ();;
diff --git a/test/blackbox-tests/test-cases/name-field-validation/dune b/test/blackbox-tests/test-cases/name-field-validation/dune
new file mode 100644
index 00000000..dfa79dd3
--- /dev/null
+++ b/test/blackbox-tests/test-cases/name-field-validation/dune
@@ -0,0 +1,9 @@
+(library
+ (modules foo)
+ (name foo.bar)
+ (wrapped false))
+
+(executable
+ (modules bar)
+ (name bar)
+ (libraries foo))
\ No newline at end of file
diff --git a/test/blackbox-tests/test-cases/name-field-validation/dune-project b/test/blackbox-tests/test-cases/name-field-validation/dune-project
new file mode 100644
index 00000000..6687faf2
--- /dev/null
+++ b/test/blackbox-tests/test-cases/name-field-validation/dune-project
@@ -0,0 +1 @@
+(lang dune 1.1)
\ No newline at end of file
diff --git a/test/blackbox-tests/test-cases/name-field-validation/foo.ml b/test/blackbox-tests/test-cases/name-field-validation/foo.ml
new file mode 100644
index 00000000..66de3158
--- /dev/null
+++ b/test/blackbox-tests/test-cases/name-field-validation/foo.ml
@@ -0,0 +1 @@
+let run () = print_endline "foo"
diff --git a/test/blackbox-tests/test-cases/name-field-validation/run.t b/test/blackbox-tests/test-cases/name-field-validation/run.t
new file mode 100644
index 00000000..df486f6f
--- /dev/null
+++ b/test/blackbox-tests/test-cases/name-field-validation/run.t
@@ -0,0 +1,5 @@
+  $ dune exec ./bar.exe
+  File "dune", line 3, characters 7-14:
+  Error: invalid library name.
+  Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'
+  [1]

From d30361a1802bc973669e6c2c160b2c096ea83a43 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Wed, 8 Aug 2018 14:15:10 +0300
Subject: [PATCH 11/12] Fix overly strict validation of invalid names

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                 | 80 +++++++++++++------
 .../test-cases/name-field-validation/dune     |  2 +-
 .../test-cases/name-field-validation/run.t    |  8 +-
 3 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index 269bb0b2..e3911ec4 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -40,13 +40,20 @@ let module_names = list module_name >>| String.Set.of_list
 module Lib_name : sig
   type t
 
+  type result =
+    | Ok of t
+    | Warn of t
+    | Invalid
+
   val error_message : string
 
+  val warn_message : string
+
   val to_string : t -> string
 
-  val of_string : string -> t option
+  val of_string : string -> result
 
-  val t : t Sexp.Of_sexp.t
+  val t : (Loc.t * result) Sexp.Of_sexp.t
 end = struct
   type t = string
 
@@ -55,28 +62,48 @@ end = struct
      Hint: library names must be non-empty and composed only of \
      the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'"
 
-  let invalid ~loc = of_sexp_error loc error_message
+  let warn_message =
+    sprintf
+      "%s.\n\
+       This is temporary allowed for libraries with (wrapped false).\
+       \nIt will not be supported in the future. \
+       Please choose a valid name field."
+      error_message
+
+  type result =
+    | Ok of t
+    | Warn of t
+    | Invalid
+
+  let valid_char = function
+    | 'A'..'Z' | 'a'..'z' | '_' | '0'..'9' -> true
+    | _ -> false
 
   let to_string s = s
 
   let of_string name =
     match name with
-    | "" -> None
+    | "" -> Invalid
     | s ->
       if s.[0] = '.' then
-        None
+        Invalid
       else
-        try
-          String.iter s ~f:(function
-            | 'A'..'Z' | 'a'..'z' | '_' | '0'..'9' -> ()
-            | _ -> raise_notrace Exit);
-          Some s
-        with Exit -> None
+        let len = String.length s in
+        let rec loop warn i =
+          if i = len - 1 then
+            if warn then Warn s else Ok s
+          else
+            let c = String.unsafe_get s i in
+            if valid_char c then
+              loop warn (i + 1)
+            else if c = '.' then
+              loop true (i + 1)
+            else
+              Invalid
+        in
+        loop false 0
 
-  let t = plain_string (fun ~loc s ->
-    match of_string s with
-    | Some n -> n
-    | None -> invalid ~loc)
+  let t = plain_string (fun ~loc s -> (loc, of_string s))
 end
 
 let file =
@@ -925,20 +952,23 @@ module Library = struct
        in
        let name =
          match name, public with
-         | Some n, _ -> Lib_name.to_string n
+         | Some (loc, n), _ ->
+           begin match n, wrapped with
+           | Ok n, _ ->  n
+           | Warn _, true -> Loc.fail loc "%s" Lib_name.error_message
+           | Warn n, false -> Loc.warn loc "%s" Lib_name.warn_message; n
+           | Invalid, _ -> Loc.fail loc "%s" Lib_name.error_message
+           end
+           |> Lib_name.to_string
          | None, Some { name = (loc, name) ; _ }  ->
            if dune_version >= (1, 1) then
              match Lib_name.of_string name, wrapped with
-             | Some n, _ -> Lib_name.to_string n
-             | None, false ->
-               Loc.warn loc
-                 "%s.\n\
-                 This is temporary allowed for libraries with (wrapped false).\
-                  \nIt will not be supported in the future. \
-                  Please choose a valid name field."
-                 Lib_name.error_message;
+             | Ok n, _ -> Lib_name.to_string n
+             | Warn _, false ->
+               Loc.warn loc "%s" Lib_name.warn_message;
                name
-             | None, true ->
+             | Warn _, true
+             | Invalid, _ ->
                of_sexp_errorf loc
                  "%s.\n\
                   Public library names don't have this restriction. \
diff --git a/test/blackbox-tests/test-cases/name-field-validation/dune b/test/blackbox-tests/test-cases/name-field-validation/dune
index dfa79dd3..4e9e2b26 100644
--- a/test/blackbox-tests/test-cases/name-field-validation/dune
+++ b/test/blackbox-tests/test-cases/name-field-validation/dune
@@ -6,4 +6,4 @@
 (executable
  (modules bar)
  (name bar)
- (libraries foo))
\ No newline at end of file
+ (libraries foo.bar))
\ No newline at end of file
diff --git a/test/blackbox-tests/test-cases/name-field-validation/run.t b/test/blackbox-tests/test-cases/name-field-validation/run.t
index df486f6f..664c8467 100644
--- a/test/blackbox-tests/test-cases/name-field-validation/run.t
+++ b/test/blackbox-tests/test-cases/name-field-validation/run.t
@@ -1,5 +1,7 @@
   $ dune exec ./bar.exe
   File "dune", line 3, characters 7-14:
-  Error: invalid library name.
-  Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'
-  [1]
+  Warning: invalid library name.
+  Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
+  This is temporary allowed for libraries with (wrapped false).
+  It will not be supported in the future. Please choose a valid name field.
+  foo

From 9a132212639edc37b9e458c698f97fcebaad1bd2 Mon Sep 17 00:00:00 2001
From: Rudi Grinberg <rudi.grinberg@gmail.com>
Date: Wed, 8 Aug 2018 14:41:11 +0300
Subject: [PATCH 12/12] Clean up the error handling for lib names

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
---
 src/jbuild.ml                                 | 40 +++++++++----------
 .../test-cases/no-name-field/run.t            |  7 ++--
 2 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/src/jbuild.ml b/src/jbuild.ml
index e3911ec4..8c0eb18d 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -45,36 +45,43 @@ module Lib_name : sig
     | Warn of t
     | Invalid
 
-  val error_message : string
-
-  val warn_message : string
+  val invalid_message : string
 
   val to_string : t -> string
 
   val of_string : string -> result
 
+  val validate : (Loc.t * result) -> wrapped:bool -> t
+
   val t : (Loc.t * result) Sexp.Of_sexp.t
 end = struct
   type t = string
 
-  let error_message =
+  let invalid_message =
     "invalid library name.\n\
      Hint: library names must be non-empty and composed only of \
      the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'"
 
-  let warn_message =
+  let wrapped_message =
     sprintf
       "%s.\n\
        This is temporary allowed for libraries with (wrapped false).\
        \nIt will not be supported in the future. \
        Please choose a valid name field."
-      error_message
+      invalid_message
 
   type result =
     | Ok of t
     | Warn of t
     | Invalid
 
+  let validate (loc, res) ~wrapped =
+    match res, wrapped with
+    | Ok s, _ -> s
+    | Warn _, true -> Loc.fail loc "%s" wrapped_message
+    | Warn s, false -> Loc.warn loc "%s" wrapped_message; s
+    | Invalid, _ -> Loc.fail loc "%s" invalid_message
+
   let valid_char = function
     | 'A'..'Z' | 'a'..'z' | '_' | '0'..'9' -> true
     | _ -> false
@@ -952,29 +959,20 @@ module Library = struct
        in
        let name =
          match name, public with
-         | Some (loc, n), _ ->
-           begin match n, wrapped with
-           | Ok n, _ ->  n
-           | Warn _, true -> Loc.fail loc "%s" Lib_name.error_message
-           | Warn n, false -> Loc.warn loc "%s" Lib_name.warn_message; n
-           | Invalid, _ -> Loc.fail loc "%s" Lib_name.error_message
-           end
+         | Some n, _ ->
+           Lib_name.validate n ~wrapped
            |> Lib_name.to_string
          | None, Some { name = (loc, name) ; _ }  ->
            if dune_version >= (1, 1) then
-             match Lib_name.of_string name, wrapped with
-             | Ok n, _ -> Lib_name.to_string n
-             | Warn _, false ->
-               Loc.warn loc "%s" Lib_name.warn_message;
-               name
-             | Warn _, true
-             | Invalid, _ ->
+             match Lib_name.of_string name with
+             | Ok m -> Lib_name.to_string m
+             | Warn _ | Invalid ->
                of_sexp_errorf loc
                  "%s.\n\
                   Public library names don't have this restriction. \
                   You can either change this public name to be a valid library \
                   name or add a \"name\" field with a valid library name."
-                 Lib_name.error_message
+                 Lib_name.invalid_message
            else
              of_sexp_error loc "name field cannot be omitted before version \
                                 1.1 of the dune language"
diff --git a/test/blackbox-tests/test-cases/no-name-field/run.t b/test/blackbox-tests/test-cases/no-name-field/run.t
index e35fbc5d..a25f890b 100644
--- a/test/blackbox-tests/test-cases/no-name-field/run.t
+++ b/test/blackbox-tests/test-cases/no-name-field/run.t
@@ -33,8 +33,7 @@ it's just a warning
   $ dune build --root public-name-invalid-wrapped-false
   Info: creating file dune-project with this contents: (lang dune 1.1)
   File "dune", line 3, characters 14-21:
-  Warning: invalid library name.
+  Error: invalid library name.
   Hint: library names must be non-empty and composed only of the following characters: 'A'..'Z',  'a'..'z', '_'  or '0'..'9'.
-  This is temporary allowed for libraries with (wrapped false).
-  It will not be supported in the future. Please choose a valid name field.
-  Entering directory 'public-name-invalid-wrapped-false'
+  Public library names don't have this restriction. You can either change this public name to be a valid library name or add a "name" field with a valid library name.
+  [1]