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

Chris00 · 2017-11-23T17:41:27Z

No description provided.

Chris00 · 2017-11-23T17:46:09Z

One could imagine that split variables say what is their separator so, say, ${ocaml_version} could be by default split as 3 components and "${ocaml_version}" would concat them on "." — but this is maybe going too far (it is not part of this PR)!

rgrinberg · 2017-11-26T09:09:50Z

From a first glance, the implementation here looks good. Though I'm not yet sure why is this feature necessary and where it can be useful.

A couple of minor nits:

Use ocp-indent to format your source. There are some indentation issues here
The jbuilder source uses 90 columns for wrapping rather than 80. I have no idea and it's not my preference but it's best to maintain this. Or at least not introduce spurious changes to this.

Chris00 · 2017-11-26T10:09:43Z

I'm not yet sure why is this feature necessary and where it can be useful.

This is an alternative solution to #300
Wit this (run ${CC} ...) will behave as expected without the need for !.

Chris00 · 2017-11-26T10:23:21Z

Use ocp-indent to format your source.

I did but it changed lines I did not modify. I made it a separate commit so it is easily revertible.

Chris00 · 2017-11-26T10:24:28Z

not introduce spurious changes to this.

I reverted the single line affected by this (and merged it with the original commit).

Chris00 · 2017-11-26T10:26:50Z

I'm not yet sure why is this feature necessary and where it can be useful.

Actually, the logical sequel to this change is to put a warning when using ! and adapting the manual.

ghost · 2017-12-20T10:46:18Z

I had a look at this PR. I just like to say that I'm a bit ashamed of the !... it is a bit nasty indeed and only exists for historical reasons. This PR goes in the right direction, whether a variable is a string or a list is indeed a property of the variable.

One reason I didn't do this earlier is that I'm a bit nervous about this:

let cflags = String.extract_blank_separated_words context.ocamlc_cflags in

ocamlc_cflags comes from the output of ocamlc -config, and all we know about it is that it is a string that can be passed unquoted to Sys.command. In particular, splitting it might not be enough, we might need to unquote as well and interpret quotes and double quotes. And of course, this parsing is dependent on the OS...

In the end, I would still like to see this PR merged. It's unlikely that such ocamlc_cflags will contain complicated stuff, and we could always fallback to ["/bin/sh"; "-c"; "<cc>"; "<cflags>"] when it's too complicated to parse.

So before we merge it, I'd like to know two things:

that someone is happy to deal with such quoting problems when they arise
what is the impact on existing packages

The only case of breakage I can think of is the following:

(run foo.exe -cc ${CC})

The fix is easy: just write "${CC} ". But we should make sure that this is done before we merge this. We can look at the jbuilder-universe to find packages that are affected. If no packages are affected, I'm ok doing the change at once. If several packages are affected, we should do a first relaese that prints a warning on such cases, fix all packages and then merge this.

Chris00 · 2017-12-20T19:30:10Z

A quick grep on the jbuilder-universe shows that very few packages use ${CC}:

async_ssl v0.9.1 (stubgen) and in bindings;
base v0.10.0;
dlm 0.3.0;
vpnkit 0.1.1 as a patch to base v0.9.3 for win32! — @Samoth could you comment on the reason?

None of these uses is problematic with the new behavior.

Chris00 · 2017-12-20T21:08:27Z

that someone is happy to deal with such quoting problems when they arise

I can provided nobody is in a hurry — I may not be able to react within a week. I may also need help to test the Windows side (I so not have a readily available Windows machine).

ghost · 2017-12-21T08:20:38Z

src/super_context.ml

    let make =
      match Bin.make with
-      | None   -> "make"
-      | Some p -> Path.to_string p
+      | None   -> Paths ([Path.of_string "make"], Split)


This needs to be Strings, otherwise jbuilder is going to think it's the file "make" in the current dir and expand it to ./make

ghost · 2017-12-21T08:23:39Z

Ok, thanks. After the discussion on slack I'm less worried about this anyway.

Could you update the doc as well?

Chris00 · 2017-12-25T12:23:04Z

I updated the code following your remarks — and more :-)

This requires ${CC} to be a split variable, see ocaml/dune#336

ghost · 2018-01-09T10:48:58Z

doc/jbuild.rst

+
+.. code:: scheme
+
+    (run foo "${^}")


This is not true at the moment. The double quotes are only seen by the lexer. From the point of view of jbuilder, hello and "hello" are exactly the same atom.

We could change jbuilder to keep this information and do different things according to whether atoms are quoted or not. This might make sense given that the syntax is different from the user point of view. However, we should have this discussion in a different ticket to avoid delaying this PR.

For now, the user has to add a space at the beginning or end of the atom:

(run foo "${^} ")

Opened #408

ghost · 2018-01-09T10:49:19Z

src/super_context.ml

+        let has_bang, var = parse_bang key in
+        if has_bang then
+          Format.eprintf "@{<warning>Warning@}: The use of the variable \
+                          prefix '!' is deprecated, simply use '${%s}'@." var;


Can you use Loc.warn loc "The use ..."?

ghost · 2018-01-09T10:49:38Z

src/super_context.ml

-               (* CR-someday jdimino: this should be an error *)
-               Strings ([""], cos)
+                Format.eprintf "@{<warning>Warning@}: Variable '<' used with \
+                                no explicit dependencies@.";


Same remark about Loc.warn

ghost · 2018-01-09T10:51:39Z

Thanks, I left some minor comment but otherwise it looks ready

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

See also ocaml#408

Chris00 · 2018-01-12T18:58:31Z

Also fixed the conflicts.

ghost · 2018-01-15T09:32:47Z

Thanks

Chris00 mentioned this pull request Nov 23, 2017

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

Closed

rgrinberg requested a review from dra27 November 26, 2017 10:19

Chris00 force-pushed the split_var branch from 25788ec to 8da1407 Compare November 26, 2017 10:22

Chris00 force-pushed the split_var branch from 8da1407 to 742be51 Compare November 26, 2017 21:31

Chris00 mentioned this pull request Dec 6, 2017

Can't install on OS X with latest release janestreet/re2#22

Closed

ghost suggested changes Dec 21, 2017

View reviewed changes

Chris00 force-pushed the split_var branch 3 times, most recently from c65f1a2 to d6bd318 Compare December 25, 2017 12:19

Chris00 added a commit to Chris00/mesh that referenced this pull request Jan 4, 2018

Add alternative CC command

1f73741

This requires ${CC} to be a split variable, see ocaml/dune#336

ghost suggested changes Jan 9, 2018

View reviewed changes

Chris00 added 7 commits January 12, 2018 19:16

Let variables say whether they are Concat or Split

1a4649b

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

Automatically concatenate split variables when needed

300960f

Run ocp-indent on action.ml and super_context.ml

2e2b502

Update documentation for variables containing a list of items

d0863a3

Adapt the documentation of "Split" to the deprecation of ${!...}

8b317ae

Treat ${<} and ${^} as split vars

31fba45

Update "parse_bang" and issue a deprecation warning for ${!...}

af58034

Chris00 added 2 commits January 12, 2018 19:18

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

0f3e3cb

Use Loc.warn to report warnings for variables

2e54070

Chris00 mentioned this pull request Jan 12, 2018

Let the parser distinguish quoted variables #408

Closed

Update the documentation for quoted variables

6273771

See also ocaml#408

Chris00 force-pushed the split_var branch from d6bd318 to 6273771 Compare January 12, 2018 18:57

ghost merged commit f8617b5 into ocaml:master Jan 15, 2018

Chris00 deleted the split_var branch January 15, 2018 10:20

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Chris00 commented Nov 23, 2017

Chris00 commented Nov 23, 2017

rgrinberg commented Nov 26, 2017

Chris00 commented Nov 26, 2017 •

edited

Loading

Chris00 commented Nov 26, 2017

Chris00 commented Nov 26, 2017

Chris00 commented Nov 26, 2017

ghost commented Dec 20, 2017

Chris00 commented Dec 20, 2017

Chris00 commented Dec 20, 2017

ghost Dec 21, 2017

ghost commented Dec 21, 2017

Chris00 commented Dec 25, 2017

ghost Jan 9, 2018

Chris00 Jan 12, 2018

ghost Jan 9, 2018

ghost Jan 9, 2018

ghost commented Jan 9, 2018

Chris00 commented Jan 12, 2018

ghost commented Jan 15, 2018

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

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

Conversation

Chris00 commented Nov 23, 2017

Chris00 commented Nov 23, 2017

rgrinberg commented Nov 26, 2017

Chris00 commented Nov 26, 2017 • edited Loading

Chris00 commented Nov 26, 2017

Chris00 commented Nov 26, 2017

Chris00 commented Nov 26, 2017

ghost commented Dec 20, 2017

Chris00 commented Dec 20, 2017

Chris00 commented Dec 20, 2017

ghost Dec 21, 2017

Choose a reason for hiding this comment

ghost commented Dec 21, 2017

Chris00 commented Dec 25, 2017

ghost Jan 9, 2018

Choose a reason for hiding this comment

Chris00 Jan 12, 2018

Choose a reason for hiding this comment

ghost Jan 9, 2018

Choose a reason for hiding this comment

ghost Jan 9, 2018

Choose a reason for hiding this comment

ghost commented Jan 9, 2018

Chris00 commented Jan 12, 2018

ghost commented Jan 15, 2018

Chris00 commented Nov 26, 2017 •

edited

Loading