Extend `(:include )` to anything that looks like a flag #153

dra27 · 2017-06-15T10:14:29Z

As per the title - with thanks and due credit to @diml for patient pointers through the internals of jbuilder!

Longer term the aim is to also extend ${read ...} forms from the action DSL, but this is a necessary first step, and possibly the ${read ...} change should be for 1.1 anyway?

It would be very good to have this in 1.0 - I shall be experimenting with it later to convert some of my OCaml Syntax jbuild files back to s-expression only.

dra27 · 2017-06-15T10:17:32Z

/cc @samoht

ghost · 2017-06-15T10:35:16Z

src/module_compilation.ml

           ; cmt_args
-           ; Dyn Lib.include_flags
+           ; Dyn (fun (include_flags, _) -> Lib.include_flags include_flags)


This should say fun (libs, _) -> ...

Oops - changed!

ghost · 2017-06-15T10:49:08Z

Thanks, yh I agree that this should go in 1.0.0 as it unifies the format of the various flags fields. BTW, any chance you could convert the last few remaining flags fields as well? There following fields are still not using this scheme:

flags for js_of_ocaml
library_flags for libraries
link_flags for executables

Also I think Super_context.expand_and_eval_set should do expand_vars as well. Currently the variable expansion is only done for library_flags which is a bit random. Do you think you can do that as well?

dra27 · 2017-06-15T10:54:35Z

I'll have a look later - temporarily paused by a lack of lithium ions :)

dra27 · 2017-06-16T09:23:25Z

That should be the flags for js_of_ocaml, but I can't test it - can anyone using js_of_ocaml have a go with this?

dra27 · 2017-06-16T10:29:21Z

@diml - what strategy should I adopt for having both String_with_vars.t and Ordered_set_lang.Unexpanded.t - functorising Sexp.t (so you can have string Sexp.t and String_with_vars.t Sexp.t) looks to be a very invasive change. The alternative I think is to convert the strings computed in Super_context.expand_and_eval_set to String_with_vars.t and then expand variables (I think that just requires exposing String_with_vars.of_string) but that feels a bit hacky. What do you think?

ghost · 2017-06-26T08:12:28Z

@dra27 I think the right way to do that is to change Ordered_set_lang.Unexpanded.t to something like this:

module Ast = struct
   type 'a t =
      | Element of 'a
      | Union of 'a t list
      | Diff of 'a t * 'a t
      | Include of 'a
end

type t = string Ast.t

dra27 · 2017-07-05T20:24:36Z

@diml - thanks! Rebased (original commits unchanged). I've switched (hopefully correctly) both expanded and unexpanded sets to use an AST internally. Rather than introduce a third kind of set (Ordered_set_lang.With_vars.t or something), I added an extra parameter to Ordered_set_lang.Unexpanded.expand which passes the S-exp AST for each atom. The diff ends up being a little big as context parameters need adding to various rules, but there's nothing particularly tricky about it (assuming I got it right - but, hey, it types!)

ghost · 2017-07-13T14:52:10Z

src/gen_rules.ml

      match force_custom_bytecode, Context.compiler ctx mode with
-      | false, Some compiler -> (mode, link_flags, compiler)
-      | _                    -> (Byte, "-custom" :: link_flags, ctx.ocamlc)
+      | false, Some compiler -> (mode, (fun x -> x), compiler)


I think link_custom should just be a list, either [] or ["-custom"]

I'll do anything to avoid an @ 😉 Changed!

ghost · 2017-07-13T14:53:22Z

src/gen_rules.ml

-      (libs_and_cm >>>
+      (libs_and_cm
+       &&&
+       Build.fanout


Build.fanout3?

It allowed fewer underscores later on... if we were going to flatten it completely, we'd really need Build.fanout4 - is that worth adding? (cf. a few lines later at Build.run)

Hmm, yh that's true, libs_and_cm is already a pair. Let's not bother

ghost · 2017-07-13T15:02:09Z

src/ordered_set_lang.ml

@@ -1,40 +1,59 @@
 open! Import

-type t = Sexp.Ast.t
+type expanded
+type unexpanded


I think in general it's recommended to do:

type expanded = Expanded type unexpanded = Unexpanded

to make it clear that the two types are distinct. I think here it's fine as the types are only used inside this module, but we might as well use the canonical thing.

ghost · 2017-07-13T15:03:25Z

src/ordered_set_lang.ml

+let t t : t = parse_general t ~f:(function Atom (_, s) -> s | List _ -> assert false)
+
+let eval t ~special_values =
+  let rec of_sexp (t : t) =


this shouldn't be called of_sexp anymore

Renamed to of_ast

ghost · 2017-07-13T15:10:19Z

src/ordered_set_lang.ml

  in
  of_sexp t

 let is_standard : t -> bool = function
-  | Atom (_, ":standard") -> true
+  | Ast.Element ":standard" -> true


shouldn't this be Ast.Special (_, "standard")?

ghost · 2017-07-13T15:19:36Z

Ok, seems good, I just a added a few minor comments

flags, ocaml_flags and ocamlopt_flags now support the (:include ) form of the ordered set language.

dra27 · 2017-08-03T08:24:59Z

Rebased and points addressed (except Build.fanout3)

Extend Ordered_set_lang.Unexpanded.expand to include a mapping function for the S-expression for each atom. The previous behaviour can be achieved with ~f:Sexp.Of_sexp.string, but this allows the S-expression to be parsed using String_with_vars.t, thus allowing variable expansion.

library_flags already included variable expansion.

ghost · 2017-08-03T16:27:58Z

Ok, looks good, thanks!

jberdine · 2017-08-23T10:18:19Z

With this PR, is it expected for e.g. (flags ((:include $(SCOPE_ROOT)/jbuild.flags))) to work? I get:

Error: exception Invalid_argument("index out of bounds")
Backtrace:
Raised by primitive operation at file "src/string_with_vars.ml", line 32, characters 18-27
Called from file "src/string_with_vars.ml", line 60, characters 22-40
Called from file "src/super_context.ml", line 900, characters 41-66
Called from file "src/ordered_set_lang.ml", line 117, characters 27-32
Called from file "list.ml", line 67, characters 20-23
Called from file "list.ml", line 67, characters 32-39
Called from file "src/ordered_set_lang.ml", line 123, characters 14-57
Called from file "list.ml", line 67, characters 20-23
Called from file "src/ordered_set_lang.ml", line 123, characters 14-57
Called from file "src/super_context.ml", line 903, characters 14-88
Called from file "src/ocaml_flags.ml" (inlined), line 41, characters 13-62
Called from file "src/ocaml_flags.ml", line 42, characters 44-85
Called from file "src/gen_rules.ml", line 469, characters 16-64
Called from file "src/gen_rules.ml", line 714, characters 13-87
Called from file "src/import.ml", line 40, characters 12-15
Called from file "src/gen_rules.ml", line 707, characters 4-392
Called from file "list.ml", line 85, characters 12-15
Called from file "src/gen_rules.ml", line 725, characters 11-47
Called from file "src/gen_rules.ml", line 1056, characters 19-50
Called from file "src/future.ml", line 88, characters 40-45
Called from file "src/future.ml", line 83, characters 65-70
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 37, characters 9-12
Re-raised at file "src/future.ml", line 17, characters 36-47
Called from file "src/future.ml", line 40, characters 9-23
Called from file "src/import.ml", line 416, characters 8-11
Re-raised at file "src/import.ml", line 418, characters 30-37
Called from file "src/future.ml", line 657, characters 6-29
Called from file "vendor/cmdliner/src/cmdliner_term.ml", line 27, characters 19-24
Called from file "vendor/cmdliner/src/cmdliner.ml", line 106, characters 32-39
Called from file "vendor/cmdliner/src/cmdliner.ml", line 136, characters 18-36
Called from file "vendor/cmdliner/src/cmdliner.ml", line 251, characters 22-48
Called from file "bin/main.ml", line 977, characters 10-51

rgrinberg · 2017-08-23T12:14:23Z

@jberdine that exception looks pretty concerning. But I think you meant ${SCOPE_ROOT}

jberdine · 2017-08-23T12:21:34Z

I think I tried both forms. IIUC, the docs indicate either should work: http://jbuilder.readthedocs.io/en/latest/jbuild.html#variables-expansion

rgrinberg · 2017-08-23T17:01:36Z

@jberdine oops, you're right. In any case, no variable expansion isn't supported. :include only supports the ordered set language (things like :standard, \, etc). While the expansion that you've shown is only supported for "actions".

We can ask @dra27 if this limitation can be lifted.

dra27 · 2017-08-25T10:19:22Z

@jberdine, @rgrinberg - addressing the unacceptable exception first! You needed to quote the filename because you're in an S-expression but used brackets. ${SCOPE_ROOT}/jbuild.flags and "$(SCOPE_ROOT)/jbuild.flags" produce the same error message. #232 converts the exception to an even more cryptic error message Error: undefined symbol include which I've worked on in #233.

dra27 · 2017-08-25T10:20:02Z

Cross-referencing @rgrinberg's change in #231

ghost suggested changes Jun 15, 2017

View reviewed changes

dra27 force-pushed the more-reads branch 2 times, most recently from aa6cf7a to b537f72 Compare June 16, 2017 09:15

dra27 force-pushed the more-reads branch 2 times, most recently from 293f4fa to e46171a Compare June 16, 2017 09:32

dra27 changed the title ~~Extend (:include ) to flags, ocamlc_flags and ocamlopt_flags~~ Extend (:include ) to anything that looks like a flag Jun 16, 2017

samoht mentioned this pull request Jun 16, 2017

port to jbuilder mirage/mirage-tcpip#319

Merged

dra27 force-pushed the more-reads branch from b9b86f1 to c8631c2 Compare June 23, 2017 13:20

dra27 force-pushed the more-reads branch 2 times, most recently from 8cf87eb to 1caa583 Compare July 5, 2017 20:23

ghost mentioned this pull request Jul 13, 2017

Testing: be sure we test thoroughly on Flambda ocsigen/lwt#439

Merged

ghost reviewed Jul 13, 2017

View reviewed changes

dra27 mentioned this pull request Aug 3, 2017

Jbuilder support ocaml/opam#3015

Merged

dra27 added 4 commits August 3, 2017 09:10

Make Ocaml_flags.t abstract

ad2fc10

Extend (:include ) form to OCaml flags

82ac994

flags, ocaml_flags and ocamlopt_flags now support the (:include ) form of the ordered set language.

Extend (:include ) form to js_of_ocaml flags

1bae743

Convert link_flags to the ordered set DSL

b01ba64

dra27 force-pushed the more-reads branch from 1caa583 to d0ada67 Compare August 3, 2017 08:24

dra27 added 3 commits August 3, 2017 09:43

Use an AST for Ordered_set_lang

d5375ba

Extend (:include ) form to library_flags

f77886b

library_flags already included variable expansion.

dra27 force-pushed the more-reads branch from d0ada67 to f77886b Compare August 3, 2017 08:45

ghost merged commit be4b5fa into ocaml:master Aug 3, 2017

dra27 mentioned this pull request Aug 25, 2017

Improve error handling for (:include ) #233

Merged

emillon mentioned this pull request Jan 11, 2024

Change compiler flags based on OCaml version #90

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `(:include )` to anything that looks like a flag #153

Extend `(:include )` to anything that looks like a flag #153

dra27 commented Jun 15, 2017

dra27 commented Jun 15, 2017

ghost Jun 15, 2017

dra27 Jun 16, 2017

ghost commented Jun 15, 2017

dra27 commented Jun 15, 2017

dra27 commented Jun 16, 2017

dra27 commented Jun 16, 2017

ghost commented Jun 26, 2017

dra27 commented Jul 5, 2017 •

edited

Loading

ghost Jul 13, 2017

dra27 Aug 3, 2017

ghost Jul 13, 2017

dra27 Aug 3, 2017

ghost Aug 3, 2017

ghost Jul 13, 2017

dra27 Aug 3, 2017

ghost Jul 13, 2017

dra27 Aug 3, 2017

ghost Jul 13, 2017

dra27 Aug 3, 2017

ghost commented Jul 13, 2017

dra27 commented Aug 3, 2017

ghost commented Aug 3, 2017

jberdine commented Aug 23, 2017

rgrinberg commented Aug 23, 2017

jberdine commented Aug 23, 2017

rgrinberg commented Aug 23, 2017

dra27 commented Aug 25, 2017

dra27 commented Aug 25, 2017

Extend (:include ) to anything that looks like a flag #153

Extend (:include ) to anything that looks like a flag #153

Conversation

dra27 commented Jun 15, 2017

dra27 commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 15, 2017

dra27 commented Jun 15, 2017

dra27 commented Jun 16, 2017

dra27 commented Jun 16, 2017

ghost commented Jun 26, 2017

dra27 commented Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 13, 2017

dra27 commented Aug 3, 2017

ghost commented Aug 3, 2017

jberdine commented Aug 23, 2017

rgrinberg commented Aug 23, 2017

jberdine commented Aug 23, 2017

rgrinberg commented Aug 23, 2017

dra27 commented Aug 25, 2017

dra27 commented Aug 25, 2017

Extend `(:include )` to anything that looks like a flag #153

Extend `(:include )` to anything that looks like a flag #153

dra27 commented Jul 5, 2017 •

edited

Loading