Let "concat" or "split" be a quality of the variable (#336)

* Let variables say whether they are Concat or Split

To concatenate the contents of a split variable, put it in a string:
"${var} ".

Fixes #300

See also https://github.com/janestreet/jbuilder/issues/408

* Issue a deprecation warning for ${!...}

* Treat ${CC}, ${<}, ${^} and ${read-lines:...} as split vars

* Change ${!^} into ${^} for this project jbuild rules

doc/jbuild.rst

@@ -579,8 +579,9 @@ Jbuilder supports the following variables:
    the toplevel directory of your project and as long as you have at
    least one ``<package>.opam`` file there, ``SCOPE_ROOT`` is
    independant of the workspace configuration
--  ``CC`` is the C compiler command line being used in the current
+- ``CC`` is the C compiler command line (list made of the compiler
-   build context
+   name followed by its flags) that was used to compile OCaml in the
+   current build context
 -  ``CXX`` is the C++ compiler command line being used in the
    current build context
 -  ``ocaml_bin`` is the path where ``ocamlc`` lives
@@ -654,41 +655,55 @@ generated by an OCaml program via:
 
 #. Expansion of lists
 
-Forms that expands to list of items, such as ``${^}``, ``${@}`` or
+Forms that expands to list of items, such as ``${CC}``, ``${^}``,
-``${read-lines:...}`` will always expand to a single string where
+``${@}`` or ``${read-lines:...}``, are suitable to be used in, say,
-elements are separated by spaces. Inside ``(run <prog> <arguments>)``
+``(run <prog> <arguments>)``.  For instance in:
-forms you can however split the items as several arguments by
-prefixing the variable with ``!``. Such forms can only be used as a
-whole atom, i.e. they can't be used inside a quoted atom.
-
-For instance in:
 
 .. code:: scheme
 
     (run foo ${^})
 
-even if there are two dependencies ``a`` and ``b``, the produced
+if there are two dependencies ``a`` and ``b``, the produced command
-command will be equivalent to the shell command:
+will be equivalent to the shell command:
-
-.. code:: shell
-
-    $ foo "a b"
-
-However, if you replace ``${^}`` by ``${!^}`` in the previous example
-the command produced would be equivalent to this shell command:
 
 .. code:: shell
 
     $ foo "a" "b"
 
-You can also use ``${!^}`` as program name, for instance:
+If you want the two dependencies to be passed as a single argument,
+you have to quote the variable as in:
+
+.. code:: scheme
+
+    (run foo "${^} ")
+
+(for now the final space is necessary)
+which is equivalent to the following shell command:
+
+.. code:: shell
+
+    $ foo "a b "
+
+(the items of the list are concatenated with space).
+Note that, since ``${^}`` is a list of items, the first one may be
+used as a program name, for instance:
 
 .. code:: scheme
 
     (rule
      ((targets (result.txt))
       (deps    (foo.exe (glob_files *.txt)))
-      (action  (run ${!^}))))
+      (action  (run ${^}))))
+
+Here is another example:
+
+.. code:: scheme
+
+    (rule
+     ((targets (foo.exe))
+      (deps    (foo.c))
+      (action  (run ${CC} -o ${@} ${<} -lfoolib))))
+
 
 Library dependencies
 --------------------

src/action.ml

@@ -221,7 +221,7 @@ module Var_expansion = struct
   module Concat_or_split = struct
     type t =
       | Concat (* default *)
-      | Split  (* ${!...} *)
+      | Split  (* the variable is a "split" list of items *)
   end
 
   open Concat_or_split
@@ -244,15 +244,13 @@ module Var_expansion = struct
     | Paths   (l, Concat) -> [concat (List.map l ~f:(string_of_path ~dir))]
 
   let to_string ~dir = function
-    | Strings (_, Split) | Paths (_, Split) -> assert false
+    | Strings (l, _) -> concat l
-    | Strings (l, Concat) -> concat l
+    | Paths   (l, _) -> concat (List.map l ~f:(string_of_path ~dir))
-    | Paths   (l, Concat) -> concat (List.map l ~f:(string_of_path ~dir))
 
   let to_path ~dir = function
-    | Strings (_, Split) | Paths (_, Split) -> assert false
+    | Strings (l, _) -> path_of_string ~dir (concat l)
-    | Strings (l, Concat) -> path_of_string ~dir (concat l)
+    | Paths ([p], _) -> p
-    | Paths ([p], Concat) -> p
+    | Paths (l,   _) ->
-    | Paths (l,   Concat) ->
       path_of_string ~dir (concat (List.map l ~f:(string_of_path ~dir)))
 
   let to_prog_and_args ~dir exp : Unresolved.Program.t * string list =
@@ -388,13 +386,13 @@ module Unexpanded = struct
       | Remove_tree x ->
         Remove_tree (E.path ~dir ~f x)
       | Mkdir x -> begin
-        match x with
+          match x with
-        | Inl path -> Mkdir path
+          | Inl path -> Mkdir path
-        | Inr tmpl ->
+          | Inr tmpl ->
-          let path = E.path ~dir ~f x in
+            let path = E.path ~dir ~f x in
-          check_mkdir (SW.loc tmpl) path;
+            check_mkdir (SW.loc tmpl) path;
-          Mkdir path
+            Mkdir path
-      end
+        end
       | Digest_files x ->
         Digest_files (List.map x ~f:(E.path ~dir ~f))
   end

src/action.mli

@@ -4,7 +4,7 @@ module Var_expansion : sig
   module Concat_or_split : sig
     type t =
       | Concat (* default *)
-      | Split  (* ${!...} *)
+      | Split  (* the variable is a "split" list of items *)
   end
 
   type t =

src/jbuild.ml

@@ -853,7 +853,7 @@ module Menhir = struct
                        ; S.virt_var __POS__ ("path-no-dep:" ^ merge_into)
                        ]
                      ; t.flags
-                     ; [ S.virt_var __POS__ "!^" ]
+                     ; [ S.virt_var __POS__ "^" ]
                      ]))
        ; fallback = Not_possible
        ; locks = []

src/super_context.ml

@@ -52,7 +52,7 @@ type t =
   ; mutable known_targets_by_src_dir_so_far : String_set.t Path.Map.t
   ; libs_vfile                              : (module Vfile_kind.S with type t = Lib.t list)
   ; cxx_flags                               : string list
-  ; vars                                    : string String_map.t
+  ; vars                                    : Action.Var_expansion.t String_map.t
   ; ppx_dir                                 : Path.t
   ; ppx_drivers                             : (string, Path.t) Hashtbl.t
   ; external_dirs                           : (Path.t, External_dir.t) Hashtbl.t
@@ -83,7 +83,13 @@ let expand_vars t ~scope ~dir s =
   | "ROOT" -> Some (Path.reach ~from:dir t.context.build_dir)
   | "SCOPE_ROOT" ->
     Some (Path.reach ~from:dir (Path.append t.context.build_dir scope.Scope.root))
-  | var -> String_map.find var t.vars)
+  | var ->
+     let open Action.Var_expansion in
+     expand_var_no_root t var
+     |> Option.map ~f:(function
+            | Paths(p,_) -> let p = List.map p ~f:Path.to_string in
+                            String.concat ~sep:" " p
+            | Strings(s,_) -> String.concat ~sep:" " s))
 
 let resolve_program t ?hint bin =
   Artifacts.binary ?hint t.artifacts bin
@@ -163,26 +169,31 @@ let create
       | None -> Path.relative context.ocaml_bin "ocamlopt"
       | Some p -> p
     in
+    let open Action.Var_expansion in
+    let open Action.Var_expansion.Concat_or_split in
     let make =
       match Bin.make with
-      | None   -> "make"
+      | None   -> Strings (["make"], Split)
-      | Some p -> Path.to_string p
+      | Some p -> Paths ([p], Split)
     in
-    [ "-verbose"       , "" (*"-verbose";*)
+    let cflags = String.extract_blank_separated_words context.ocamlc_cflags in
-    ; "CPP"            , sprintf "%s %s -E" context.c_compiler context.ocamlc_cflags
+    [ "-verbose"       , Strings ([] (*"-verbose";*), Concat)
-    ; "PA_CPP"         , sprintf "%s %s -undef -traditional -x c -E" context.c_compiler
+    ; "CPP"            , Strings (context.c_compiler :: cflags @ ["-E"], Split)
-                           context.ocamlc_cflags
+    ; "PA_CPP"         , Strings (context.c_compiler :: cflags
-    ; "CC"             , sprintf "%s %s" context.c_compiler context.ocamlc_cflags
+                                  @ ["-undef"; "-traditional"; "-x"; "c"; "-E"],
-    ; "CXX"            , String.concat ~sep:" " (context.c_compiler :: cxx_flags)
+                                  Split)
-    ; "ocaml_bin"      , Path.to_string context.ocaml_bin
+    ; "CC"             , Strings (context.c_compiler :: cflags, Split)
-    ; "OCAML"          , Path.to_string context.ocaml
+    ; "CXX"            , Strings (context.c_compiler :: cxx_flags, Split)
-    ; "OCAMLC"         , Path.to_string context.ocamlc
+    ; "ocaml_bin"      , Paths ([context.ocaml_bin], Split)
-    ; "OCAMLOPT"       , Path.to_string ocamlopt
+    ; "OCAML"          , Paths ([context.ocaml], Split)
-    ; "ocaml_version"  , context.version
+    ; "OCAMLC"         , Paths ([context.ocamlc], Split)
-    ; "ocaml_where"    , Path.to_string context.stdlib_dir
+    ; "OCAMLOPT"       , Paths ([ocamlopt], Split)
-    ; "ARCH_SIXTYFOUR" , string_of_bool context.arch_sixtyfour
+    ; "ocaml_version"  , Strings ([context.version], Concat)
+    ; "ocaml_where"    , Paths ([context.stdlib_dir], Concat)
+    ; "ARCH_SIXTYFOUR" , Strings ([string_of_bool context.arch_sixtyfour],
+                                  Concat)
     ; "MAKE"           , make
-    ; "null"           , Path.to_string Config.dev_null
+    ; "null"           , Paths ([Config.dev_null], Concat)
     ]
     |> String_map.of_alist
     |> function
@@ -467,12 +478,12 @@ module Pkg_version = struct
     Build.vpath spec
 end
 
-let parse_bang var : Action.Var_expansion.Concat_or_split.t * string =
+let parse_bang var : bool * string =
   let len = String.length var in
   if len > 0 && var.[0] = '!' then
-    (Split, String.sub var ~pos:1 ~len:(len - 1))
+    (true, String.sub var ~pos:1 ~len:(len - 1))
   else
-    (Concat, var)
+    (false, var)
 
 module Action = struct
   open Build.O
@@ -533,7 +544,10 @@ module Action = struct
     let t =
       U.partial_expand t ~dir ~map_exe ~f:(fun loc key ->
         let open Action.Var_expansion in
-        let cos, var = parse_bang key in
+        let has_bang, var = parse_bang key in
+        if has_bang then
+          Loc.warn loc "The use of the variable prefix '!' is deprecated, \
+                        simply use '${%s}'@." var;
         match String.lsplit2 var ~on:':' with
         | Some ("path-no-dep", s) -> Some (path_exp (Path.relative dir s))
         | Some ("exe"     , s) ->
@@ -597,7 +611,7 @@ module Action = struct
             let path = Path.relative dir s in
             let data =
               Build.contents path
-              >>^ fun s -> Strings ([s], cos)
+              >>^ fun s -> Strings ([s], Concat)
             in
             add_ddep acc ~key data
           end
@@ -605,7 +619,7 @@ module Action = struct
             let path = Path.relative dir s in
             let data =
               Build.lines_of path
-              >>^ fun l -> Strings (l, cos)
+              >>^ fun l -> Strings (l, Split)
             in
             add_ddep acc ~key data
           end
@@ -613,7 +627,7 @@ module Action = struct
             let path = Path.relative dir s in
             let data =
               Build.strings path
-              >>^ fun l -> Strings (l, cos)
+              >>^ fun l -> Strings (l, Split)
             in
             add_ddep acc ~key data
           end
@@ -624,32 +638,30 @@ module Action = struct
           | "@" -> begin
               match targets_written_by_user with
               | Infer -> Loc.fail loc "You cannot use ${@} with inferred rules."
-              | Static l -> Some (Paths (l, cos))
+              | Static l -> Some (Paths (l, Split))
             end
-          | _ ->
+          | _ -> expand_var_no_root sctx var)
-            match expand_var_no_root sctx var with
-            | Some s -> Some (str_exp s)
-            | None -> None)
     in
     (t, acc)
 
   let expand_step2 ~dir ~dynamic_expansions ~deps_written_by_user ~map_exe t =
     let open Action.Var_expansion in
-    U.Partial.expand t ~dir ~map_exe ~f:(fun _loc key ->
+    U.Partial.expand t ~dir ~map_exe ~f:(fun loc key ->
       match String_map.find key dynamic_expansions with
       | Some _ as opt -> opt
       | None ->
-        let cos, var = parse_bang key in
+        let _, var = parse_bang key in
         match var with
         | "<" ->
           Some
             (match deps_written_by_user with
              | [] ->
-               (* CR-someday jdimino: this should be an error *)
+                Loc.warn loc "Variable '<' used with no explicit \
-               Strings ([""], cos)
+                              dependencies@.";
+                Strings ([""], Split)
              | dep :: _ ->
-               Paths ([dep], cos))
+               Paths ([dep], Split))
-        | "^" -> Some (Paths (deps_written_by_user, cos))
+        | "^" -> Some (Paths (deps_written_by_user, Split))
         | _ -> None)
 
   let run sctx t ~dir ~dep_kind ~targets:targets_written_by_user ~scope

test/blackbox-tests/test-cases/misc/jbuild

@@ -33,7 +33,7 @@
 (alias
  ((name   runtest)
   (deps   (jbuild jbuild-plop))
-  (action (run diff -u ${!^}))))
+  (action (run diff -u ${^}))))
 
 ;; For some tests in subdirs
 
@@ -56,4 +56,4 @@
 (alias
  ((name   runtest)
   (deps   (pnd-result pnd-expected))
-  (action (run diff -u ${!^}))))
+  (action (run diff -u ${^}))))