Allow to split all variables & proper error when using them in strings #309

Chris00 · 2017-11-05T09:08:46Z

Fix issue #300

dra27 · 2017-11-05T09:47:30Z

I haven't looked closely, but what happens, for example, if $CC is C:\Program Files\Microsoft Visual Studio\...\cl.exe -WX?

Chris00 · 2017-11-05T10:22:57Z

@dra27 Yes, I forgot to mention that I'm not convinced that String.extract_blank_separated_words is the right function to use to split the command. Is there already a splitting function that understand the command as the shell does? BTW, what is the value of bytecomp_c_compiler (in _build/log) on Windows?

rgrinberg · 2017-11-07T13:06:08Z

@Chris00 do you have a CLA submitted to JST? You might want to get that done while we're reviewing this PR.

Chris00 · 2017-11-07T16:54:58Z

On 2017-11-07, Rudi Grinberg wrote: @Chris00 do you have a CLA submitted to JST? You might want to get that done while we're reviewing this PR.

I don’t. Where do I find it?

rgrinberg · 2017-11-07T17:06:04Z

here

rgrinberg · 2017-11-08T10:50:38Z

BTW, what is the value of bytecomp_c_compiler (in _build/log) on Windows?

If you have a windows box ready, then it's just bytecomp_c_compiler from ocamlc -config.

Is there already a splitting function that understand the command as the shell does?

I don't think so

Chris00 · 2017-11-08T10:54:28Z

@rgrinberg I do not have a Windows box, that's why I was asking (I could run AppVeyor but I do not have the time to write the script right now).

dra27 · 2017-11-08T11:55:06Z

It doesn't include by default include a hard-coded directory. Here's a 4.06 msvc64 example: cl -nologo -O2 -Gy- -MD -D_CRT_SECURE_NO_DEPRECATE.

This should work - I'm just wondering whether we should use this opportunity to do some better for specifying compound "commands" like this. I'm for example having to use hacks to get jbuilder to run the compiler when it's a bytecode image with no header (so has to be invoked via ocamlrun).

It's tempting to allow this now, but if we then improve the handling later we'd probably want to remove this behaviour again?

Chris00 · 2017-11-08T18:07:06Z

@dra27 Could you precise what you have in mind? I wonder if variables are considered as lists (${CC} but also ${^}) should not be by default split because that's what the user expects. on can always use, say "${^}", if one wants to space-concatenate the list as a single argument (instead of raising an error as of now, it would automatically convert the list to a string).

dra27 · 2017-11-08T21:11:27Z

What concerns me slightly is that the present ${!^} and ${!@} do not split strings up, rather they do not concatenate lists in the first place (i.e. if any of the paths involved in the dependencies contain spaces, they will not generate extra entries).

I'm wondering if jbuilder should be doing something (potentially hideous) along the lines of how CreateProcess works - so ${CC} should just be automatically split when used in the "head" context of a (run.

This isn't entirely hypothetical - I know for example that LexiFi use absolute paths to C compilers in order to differentiate 32 and 64-bit Microsoft C compilers, so while I think what you're suggesting here works now, I can see a problem lurking shortly round the corner...

dra27 · 2017-11-08T21:12:27Z

I also have a small fear that we're heading towards ${GNU-make-function CC} if we're not careful!

Chris00 · 2017-11-08T22:03:32Z

I'm wondering if jbuilder should be doing something (potentially hideous) along the lines of how CreateProcess works - so ${CC} should just be automatically split when used in the "head" context of a (run.

I think it should be the other way around: ${CC} should be a split variable by default and "${CC}" turns it into a single argument — as it would be with a shell variable.

dra27 · 2017-11-08T22:09:03Z

Yes, but what therefore does "split" mean - because just splitting it on spaces is not semantically correct when the first "component" should itself include some spaces.

The shell's not the greatest inspiration for this, as it tends to be a nightmare there too!

Chris00 · 2017-11-08T22:39:44Z

The variable CC is (was) constructed by jbuilder as a string. However, the C compiler comes from a separate variable context.c_compiler which is no longer concatenated in my patch. So spaces in the compiler path are handled correctly. The problem is that the context.ocamlc_cflags must be split in order to be interpreted correctly by run — that split must be the same as for a Unix shell (maybe even on Windows since the compilation is done with Cygwin, but you know better).

dra27

Sorry for taking a while to review this properly. Minor changes - it basically looks good to me. My previous concern about splitting program names with spaces incorrectly was, as you politely pointed out, erroneous as it'll only be as bad as the output of ocamlc -config (i.e. it's good in 4.06.0+ where the compiler and flags are separate items and potentially bad in older versions)

dra27 · 2017-11-15T21:18:09Z

src/action.ml

@@ -200,7 +200,8 @@ 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 (_, Split) | Paths (_, Split) ->
+       failwith "Action.Var_expansion.to_string"


Given that this wants to be matched on later, I think it would be better to define an exception, rather than use Failure

dra27 · 2017-11-15T21:22:25Z

src/action.ml

+           try Some (VE.to_string ~dir e)
+           with Failure _ ->
+             let msg = sprintf "Split variable %S in string does not \
+                                make sense" var in


Given the message, I'd match an explicit exception just to guard against some future additional failure mode.

I know there's another message which refers to things not making sense, but I think something more descriptive of what doesn't make sense would be better, e.g. "${!%S} is a list and so cannot be used in a string (did you mean just ${%S}"

It's also tempting just to say that "${!@}" = "${@}"

It's also tempting just to say that "${!@}" = "${@}"

Indeed! (My take on this is that splitable variables should come be default as such — so that, say, (run ${CC} always does what is expected — and are concatenated with spaces if used in a string — e.g. (run "${CC}" to have the old behavior) .

dra27 · 2017-11-15T21:24:23Z

src/super_context.ml

-let expand_var_no_root t var = String_map.find var t.vars
+let expand_var_no_root t var cos =
+  match String_map.find var t.vars with
+  | Some v -> Some(v cos)


Nit: space before the (

dra27 · 2017-11-15T21:29:48Z

src/super_context.ml

+        let p = List.map p ~f:Path.to_string in
+        Some (String.concat ~sep:" " p)
+     | Some(Strings(s,_)) -> Some (String.concat ~sep:" " s)
+     | None -> None)


Please could this be refactored as a function and a call to Option.map?

dra27 · 2017-11-15T21:34:37Z

src/super_context.ml

+    [ "-verbose"       , str_exp [] (*"-verbose";*)
+    ; "CPP"            , str_exp (context.c_compiler :: cflags @ ["-E"])
+    ; "PA_CPP"         , str_exp (context.c_compiler :: cflags
+                                  @ ["-undef -traditional"; "-x"; "c"; "-E"])


Shouldn't "-undef -traditional" be "-undef"; "-traditional"?

Chris00 · 2017-11-15T22:00:13Z

I'll try to make the requested changes tomorrow.

dra27 · 2017-11-15T22:01:42Z

Thank you - I shall endeavour not to take so long to review them this time!

Fixes ocaml#300

Before this change, using, say ${!@}, in a string was making an assertion fail.

Chris00 · 2017-11-23T17:42:39Z

To show what I meant by "splitable variables should come be default as such", see #336

ghost · 2018-01-18T11:30:31Z

I suppose that #336 replaced this PR

rgrinberg assigned dra27 Nov 8, 2017

dra27 closed this Nov 15, 2017

dra27 reopened this Nov 15, 2017

dra27 requested changes Nov 15, 2017

View reviewed changes

Chris00 added 5 commits November 23, 2017 17:20

Allow the use of "!" for all variables

c50cc5d

Fixes ocaml#300

Display a proper error when using split variables in strings

2a4e233

Before this change, using, say ${!@}, in a string was making an assertion fail.

Specific exception for split var in string

e340978

Better error message for split vars in strings

ea36015

Small code improvements

34cb4c0

Chris00 force-pushed the master branch from 42465b4 to 34cb4c0 Compare November 23, 2017 16:20

ghost closed this Jan 18, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to split all variables & proper error when using them in strings #309

Allow to split all variables & proper error when using them in strings #309

Chris00 commented Nov 5, 2017

dra27 commented Nov 5, 2017

Chris00 commented Nov 5, 2017

rgrinberg commented Nov 7, 2017

Chris00 commented Nov 7, 2017 via email

rgrinberg commented Nov 7, 2017

rgrinberg commented Nov 8, 2017

Chris00 commented Nov 8, 2017

dra27 commented Nov 8, 2017 •

edited

Loading

Chris00 commented Nov 8, 2017 •

edited

Loading

dra27 commented Nov 8, 2017

dra27 commented Nov 8, 2017

Chris00 commented Nov 8, 2017

dra27 commented Nov 8, 2017

Chris00 commented Nov 8, 2017

dra27 left a comment

dra27 Nov 15, 2017

dra27 Nov 15, 2017

Chris00 Nov 15, 2017

dra27 Nov 15, 2017

dra27 Nov 15, 2017

dra27 Nov 15, 2017

Chris00 Nov 15, 2017

Chris00 commented Nov 15, 2017

dra27 commented Nov 15, 2017

Chris00 commented Nov 23, 2017

ghost commented Jan 18, 2018

Allow to split all variables & proper error when using them in strings #309

Allow to split all variables & proper error when using them in strings #309

Conversation

Chris00 commented Nov 5, 2017

dra27 commented Nov 5, 2017

Chris00 commented Nov 5, 2017

rgrinberg commented Nov 7, 2017

Chris00 commented Nov 7, 2017 via email

rgrinberg commented Nov 7, 2017

rgrinberg commented Nov 8, 2017

Chris00 commented Nov 8, 2017

dra27 commented Nov 8, 2017 • edited Loading

Chris00 commented Nov 8, 2017 • edited Loading

dra27 commented Nov 8, 2017

dra27 commented Nov 8, 2017

Chris00 commented Nov 8, 2017

dra27 commented Nov 8, 2017

Chris00 commented Nov 8, 2017

dra27 left a comment

Choose a reason for hiding this comment

dra27 Nov 15, 2017

Choose a reason for hiding this comment

dra27 Nov 15, 2017

Choose a reason for hiding this comment

Chris00 Nov 15, 2017

Choose a reason for hiding this comment

dra27 Nov 15, 2017

Choose a reason for hiding this comment

dra27 Nov 15, 2017

Choose a reason for hiding this comment

dra27 Nov 15, 2017

Choose a reason for hiding this comment

Chris00 Nov 15, 2017

Choose a reason for hiding this comment

Chris00 commented Nov 15, 2017

dra27 commented Nov 15, 2017

Chris00 commented Nov 23, 2017

ghost commented Jan 18, 2018

dra27 commented Nov 8, 2017 •

edited

Loading

Chris00 commented Nov 8, 2017 •

edited

Loading