enabled_if #819

rgrinberg · 2018-05-29T08:52:06Z

Problem

The problem this PR is trying to solve is a bit specific to dune, but it's likely that it will be encountered by other projects as well. Currently, dune has a complicated script for generating test definitions that make sure tests run only in environments which support them. For example, some tests may not run on win32. A mechanism to enable tests conditionally would fix these problems.

Proposal

Add an enabled_if field to aliases to toggle the execution of the alias. This field will be valued by a little EDSL for expressing boolean expressions. Here's an example of the kind of conditions we'd express with it:

(alias
 ((name runtest)
  (deps (foo.exe))
  (action (run ${<}))
  (enabled_if (and (<> ${os} win32) (>= ${ocaml_version} 4.0.5)))))

Progress

This stalled a bit since I'm not sure how to do handle type safety here. Ideally, we only allow numbers and versions to be compared with >=, <, etc. I guess we really need to annotate which variables are comparable and making sure that we don't compare numbers to strings or versions.

ghost · 2018-05-30T18:54:21Z

this boolean language looks quite simple and during development all expressions should be evaluated on every run, so runtime checks will do.

Currently we already keep the type of variable expansion (string or path). We can add a version type

rgrinberg · 2018-05-31T05:00:11Z

Okay, makes sense. I'm wondering how we should organize things however. Blang could certainly live in its own module, but Var_expansion.t isn't really specific to actions anymore.

ghost · 2018-05-31T13:42:42Z

Maybe a new Value module? That's basically what it represents. BTW, let's do #701 (comment) first as it formalizes the interpretation of variables.

rgrinberg · 2018-07-05T15:42:45Z

@diml now that we have a Value module, I'd like to move this along. One question is how to handle expressions like : (enabled_if %{ocaml_version}). In other words, what happens if we try to use a non boolean valued expression as the condition in enabled_if. Just a runtime error?

ghost · 2018-07-06T07:41:58Z

Yes, a runtime error seems fine for now. These expressions are very simple and will be evaluated often during development, so there is no need for heavy machinery.

rgrinberg · 2018-07-12T13:35:39Z

Okay. One last point is that we might be closing the door on things like (enabled_if %{read:file-with-flag}) since there would be no way to read a file into a boolean like this. This is a bit awkward, but I can imagine wanting a toggle via env var for example. In which case, an awkward form would have to be invented: (enabled_if %{env-var-set-p:FOO})

ghost · 2018-07-12T14:09:01Z

That seems fine, but if we add support for reading environment variables like this, then (setenv X foo (echo %{env:X}) must print "foo".

rgrinberg · 2018-07-20T22:09:50Z

Okay, this PR has been revived and seems to be in a good enough form for a v1 of the feature. The only thing that remains is to make sure that the field is available only for 1.1 version of the language. The current version doesn't support advanced features like %{read:..} stanzas, but should be good enough for our use case in dune.

rgrinberg · 2018-07-21T18:48:50Z

I added support for the (1, 1) version guard.

rgrinberg · 2018-07-22T18:21:22Z

This PR is a pre-req as well for #924.

ghost · 2018-07-30T12:20:46Z

Ok, could you write the documentation for this feature? I'd like to read the semantic of boolean expressions before reading the code that implements it.

rgrinberg · 2018-07-30T12:25:56Z

Okay, will add that to the TODO list. As a tl;dr:

Value.String "false" is false, Value.String "true" is true. Everything else is an error when evaluated as a boolean. This is to match the way things are in $ ocamlc -config.
Comparison operators are done on Value.t list as string lists (with some optimization to avoid conversion when it isn't necessary).

I considered introducing Value.Boolean constructor, but if there's no way for the user to actually input this value as a literal, the use seems limited.

ghost · 2018-07-30T14:47:33Z

Thanks, that seems reasonable

ghost · 2018-07-30T14:49:34Z

src/blang.ml

+  | Or of 'a t list
+  | Compare of Op.t * 'a * 'a
+
+let compare_vals ~dir (x : Value.t) (y : Value.t) =


What about putting this in Value.L?

ghost · 2018-07-30T14:54:53Z

src/simple_rules.ml

-         ~targets:Alias
-         ~targets_dir:dir
-         ~scope)
+  let add_alias () =


I think this code would be slightly clearer if written this way:

let enabled = match alias_conf.enabled_if with | None -> true | Some blang -> ... in if enabled then add_alias sctx ...

ghost · 2018-07-30T14:56:24Z

src/stdune/sexp.ml

@@ -81,6 +81,11 @@ module Of_sexp = struct
    Printf.ksprintf (fun msg ->
      of_sexp_error loc ?hint ("No variables allowed " ^ msg)) fmt

+  let _fix f =


Why not modify the type of fix instead of adding this function?

This is just an artefact from the rebase. Removed.

ghost · 2018-07-30T15:01:07Z

src/jbuild.ml

+           Compare (op, x, y))))
+    in
+    let t =
+    fix begin fun (t : String_with_vars.t Blang.t Sexp.Of_sexp.t) ->


Indentation looks wrong

ghost · 2018-07-30T15:02:05Z

src/jbuild.ml

+      [ "or", repeat t >>| (fun x -> Or x)
+      ; "and", repeat t >>| (fun x -> And x)
+      ] @ ops
+      |> sum ~rest:(String_with_vars.t >>| fun v -> Expr v)


I'm dubious about this rest argument. I feel like it's going to make error messages worse. Could you use if_list instead?

rgrinberg · 2018-07-30T19:31:08Z

@diml I've addressed all your comments. Actually, ~rest was only there because it's older than if_list. I've simply didn't reconsider the implementation using the new API after rebasing.

ghost · 2018-07-31T07:51:56Z

doc/dune-files.rst

+
+   expr := (and <expr>+)
+         | (or <expr>+)
+         | (compare <op> <template> <template>)


should be: (<op> <template> <template>)

ghost · 2018-07-31T07:58:00Z

src/value.ml

+let compare x y =
+  match x, y with
+  | String x, String y -> String.compare x y
+  | Path x, Path y -> Path.compare x y


The Dir x, Dir y case is missing

Actually, we don't need this function anyway. I got rid of it.

ghost · 2018-07-31T08:21:46Z

Looks good, but it seems that the doc comment I added on Value.L.compare_vals disappeared

rgrinberg · 2018-07-31T08:57:30Z

@diml oops, the subsequent commit removed it during the rebase. The doc is now back.

This field controls whether the alias/test will be run or not. Boolean expressions are defined using the Blang.t type. This type represents simple boolean expressions that become useful when we allow to interpolate variables into them. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg · 2018-07-31T09:01:37Z

Squashed & Rebased. Also added a change log entry.

@rgrinberg

CHANGES: - Fix lookup of command line specified files when `--root` is given. Previously, passing in `--root` in conjunction with `--workspace` or `--config` would not work correctly (ocaml/dune#997, @rgrinberg) - Add support for customizing env nodes in workspace files. The `env` stanza is now allowed in toplevel position in the workspace file, or for individual contexts. This feature requires `(dune lang 1.1)` (ocaml/dune#1038, @rgrinberg) - Add `enabled_if` field for aliases and tests. This field controls whether the test will be ran using a boolean expression language. (ocaml/dune#819, @rgrinberg) - Make `name`, `names` fields optional when a `public_name`, `public_names` field is provided. (ocaml/dune#1041, fix ocaml/dune#1000, @rgrinberg) - Interpret `X` in `--libdir X` as relative to `PREFIX` when `X` is relative (ocaml/dune#1072, fix ocaml/dune#1070, @diml) - Add support for multi directory libraries by writing `(include_subdirs unqualified)` (ocaml/dune#1034, @diml) - Add `(staged_pps ...)` to support staged ppx rewriters such as ones using the OCaml typer like `ppx_import` (ocaml/dune#1080, fix ocaml/dune#193, @diml) - Use `-opaque` in the `dev` profile. This option trades off binary quality for compilation speed when compiling .cmx files. (ocaml/dune#1079, fix ocaml/dune#1058, @rgrinberg) - Fix placeholders in `dune subst` documentation (ocaml/dune#1090, @emillon, thanks @trefis for the bug report) - Add locations to errors when a missing binary in PATH comes from a dune file (ocaml/dune#1096, fixes ocaml/dune#1095, @rgrinberg)

rgrinberg requested a review from a user May 29, 2018 08:52

trefis mentioned this pull request Jun 28, 2018

Adding some conditionals in the language #924

Open

rgrinberg added this to the 1.1.0 milestone Jul 11, 2018

rgrinberg force-pushed the enabled-if branch from f08b80e to 5dcba6d Compare July 20, 2018 22:05

rgrinberg changed the title ~~enabled_if [WIP]~~ enabled_if Jul 20, 2018

rgrinberg force-pushed the enabled-if branch from 5dcba6d to 2bf67ac Compare July 21, 2018 18:48

ghost reviewed Jul 30, 2018

View reviewed changes

ghost suggested changes Jul 30, 2018

View reviewed changes

rgrinberg force-pushed the enabled-if branch from 2bf67ac to 7403843 Compare July 30, 2018 19:04

ghost suggested changes Jul 31, 2018

View reviewed changes

rgrinberg force-pushed the enabled-if branch 2 times, most recently from f7a9dc8 to bf89dee Compare July 31, 2018 08:09

rgrinberg force-pushed the enabled-if branch from bf89dee to 018a453 Compare July 31, 2018 08:57

rgrinberg force-pushed the enabled-if branch from 018a453 to 3d9612f Compare July 31, 2018 09:00

CHANGES entry

3e4720e

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

ghost approved these changes Jul 31, 2018

View reviewed changes

rgrinberg merged commit de0ccfa into ocaml:master Jul 31, 2018

rgrinberg mentioned this pull request Aug 6, 2018

[new release] dune (1.1.0) ocaml/opam-repository#12442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enabled_if #819

enabled_if #819

rgrinberg commented May 29, 2018 •

edited

Loading

ghost commented May 30, 2018

rgrinberg commented May 31, 2018

ghost commented May 31, 2018

rgrinberg commented Jul 5, 2018

ghost commented Jul 6, 2018

rgrinberg commented Jul 12, 2018

ghost commented Jul 12, 2018

rgrinberg commented Jul 20, 2018

rgrinberg commented Jul 21, 2018

rgrinberg commented Jul 22, 2018

ghost commented Jul 30, 2018

rgrinberg commented Jul 30, 2018

ghost commented Jul 30, 2018

ghost Jul 30, 2018

ghost Jul 30, 2018 •

edited by ghost

Loading

ghost Jul 30, 2018

rgrinberg Jul 30, 2018

ghost Jul 30, 2018

ghost Jul 30, 2018

rgrinberg commented Jul 30, 2018

ghost Jul 31, 2018

ghost Jul 31, 2018

rgrinberg Jul 31, 2018

ghost Jul 31, 2018

ghost commented Jul 31, 2018

rgrinberg commented Jul 31, 2018

rgrinberg commented Jul 31, 2018

enabled_if #819

enabled_if #819

Conversation

rgrinberg commented May 29, 2018 • edited Loading

Problem

Proposal

Progress

ghost commented May 30, 2018

rgrinberg commented May 31, 2018

ghost commented May 31, 2018

rgrinberg commented Jul 5, 2018

ghost commented Jul 6, 2018

rgrinberg commented Jul 12, 2018

ghost commented Jul 12, 2018

rgrinberg commented Jul 20, 2018

rgrinberg commented Jul 21, 2018

rgrinberg commented Jul 22, 2018

ghost commented Jul 30, 2018

rgrinberg commented Jul 30, 2018

ghost commented Jul 30, 2018

Choose a reason for hiding this comment

ghost Jul 30, 2018 • edited by ghost 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

rgrinberg commented Jul 30, 2018

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 31, 2018

rgrinberg commented Jul 31, 2018

rgrinberg commented Jul 31, 2018

rgrinberg commented May 29, 2018 •

edited

Loading

ghost Jul 30, 2018 •

edited by ghost

Loading