Add `enabled_if` to `install` stanzas and enforce variable restrictions by voodoos · Pull Request #3408 · ocaml/dune

voodoos · 2020-04-21T15:15:53Z

This PR fixes #3354 by adding the enabled_if field to the install stanza.

Doing that I discovered that the enabled_if field was not guarded correctly in the executable stanza and any variables could be used. This PR enforce the same variable restrictions for enabled_if in the executables and install stanzas as in the library stanza.

To do that chose to upstream the ad-hoc variable checking code for the install stanza to the enabled_if decoder.

rgrinberg · 2020-04-21T17:57:01Z

Needs a CHANGES entry.

There's a test failure as well:

--- a/test/blackbox-tests/test-cases/enabled_if/run.t
463+++ b/test/blackbox-tests/test-cases/enabled_if/run.t.corrected
464@@ -24,8 +24,8 @@ Test the enabled_if field for libraries:
465   [1]
466 
467   $ dune build main.exe
468-  File "dune", line 34, characters 12-15:
469-  34 |  (libraries foo))
470+  File "dune", line 35, characters 12-15:
471+  35 |  (libraries foo))
472                    ^^^
473   Error: Library "foo" in _build/default is hidden (unsatisfied 'enabled_if').

voodoos · 2020-04-22T08:34:23Z

Needs a CHANGES entry.

There's a test failure as well

Done.

src/dune/dune_file.ml

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

voodoos · 2020-04-23T09:08:30Z

I decided to move the enabled_if related code to it's own module (b8b805a) to stop bloating the Dune_file module.

It is not completely generic yet and does not replace the code in Lib_info that associates different minimal version to each variable.

dune/src/dune/lib_config.ml

Lines 29 to 38 in 0782578

    
           let allowed_in_enabled_if = 
        
             List.map var_map ~f:(fun (var, _) -> 
        
                 let min_version = 
        
                   match var with 
        
                   | "profile" -> (2, 5) 
        
                   | "ccomp_type" -> (2, 0) 
        
                   | "ocaml_version" -> (2, 5) 
        
                   | _ -> (1, 0) 
        
                 in 
        
                 (var, min_version))

However I feel like it could be easily added when needed (when another variable will be allowed in these fields).

What do you think ?

test/blackbox-tests/test-cases/enabled_if-exec/install/dune

rgrinberg · 2020-04-23T15:32:15Z

The Enabled_if module looks good to me.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

@bikallem

…lugin, dune-private-libs and dune-glob (2.6.0) CHANGES: - Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2` results in an error. (ocaml/dune#3444, fix ocaml/dune#3443, @bikallem) - Add and `enabled_ if` field to the `install` stanza. Enforce the same variable restrictions for `enabled_if` fields in the `executable` and `install` stanzas than in the `library` stanza. When using dune lang < 2.6, the usage of forbidden variables in executables stanzas with only trigger a warning to maintain compatibility. (ocaml/dune#3408 and ocaml/dune#3496, fixes ocaml/dune#3354, @voodoos) - Insert a constraint one the version of dune when the user explicitly specify the dependency on dune in the `dune-project` file (ocaml/dune#3434 , fixes ocaml/dune#3427, @diml) - Generate correct META files for sub-libraries (of the form `lib.foo`) that contain .js runtime files. (ocaml/dune#3445, @hhugo) - Add a `(no-infer ...)` action that prevents inference of targets and dependencies in actions. (ocaml/dune#3456, fixes ocaml/dune#2006, @roddyyaga) - Correctly infer targets for the `diff?` action. (ocaml/dune#3457, fixes ocaml/dune#2990, @greedy) - Fix `$ dune print-rules` crashing (ocaml/dune#3459, fixes ocaml/dune#3440, @rgrinberg) - Simplify js_of_ocaml rules using js_of_ocaml.3.6 (ocaml/dune#3375, @hhugo) - Add a new `ocaml-merlin` subcommand that can be used by Merlin to get configuration directly from dune instead of using `.merlin` files. (ocaml/dune#3395, @voodoos) - Remove experimental variants feature and make default implementations part of the language (ocaml/dune#3491, fixes ocaml/dune#3483, @rgrinberg)

@bikallem

…lugin, dune-private-libs and dune-glob (2.6.0) CHANGES: - Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2` results in an error. (ocaml/dune#3444, fix ocaml/dune#3443, @bikallem) - Add and `enabled_ if` field to the `install` stanza. Enforce the same variable restrictions for `enabled_if` fields in the `executable` and `install` stanzas than in the `library` stanza. When using dune lang < 2.6, the usage of forbidden variables in executables stanzas with only trigger a warning to maintain compatibility. (ocaml/dune#3408 and ocaml/dune#3496, fixes ocaml/dune#3354, @voodoos) - Insert a constraint one the version of dune when the user explicitly specify the dependency on dune in the `dune-project` file (ocaml/dune#3434 , fixes ocaml/dune#3427, @diml) - Generate correct META files for sub-libraries (of the form `lib.foo`) that contain .js runtime files. (ocaml/dune#3445, @hhugo) - Add a `(no-infer ...)` action that prevents inference of targets and dependencies in actions. (ocaml/dune#3456, fixes ocaml/dune#2006, @roddyyaga) - Correctly infer targets for the `diff?` action. (ocaml/dune#3457, fixes ocaml/dune#2990, @greedy) - Fix `$ dune print-rules` crashing (ocaml/dune#3459, fixes ocaml/dune#3440, @rgrinberg) - Simplify js_of_ocaml rules using js_of_ocaml.3.6 (ocaml/dune#3375, @hhugo) - Add a new `ocaml-merlin` subcommand that can be used by Merlin to get configuration directly from dune instead of using `.merlin` files. (ocaml/dune#3395, @voodoos) - Remove experimental variants feature and make default implementations part of the language (ocaml/dune#3491, fixes ocaml/dune#3483, @rgrinberg)

@bikallem

…lugin, dune-private-libs and dune-glob (2.6.0) CHANGES: - Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2` results in an error. (ocaml/dune#3444, fix ocaml/dune#3443, @bikallem) - Add and `enabled_ if` field to the `install` stanza. Enforce the same variable restrictions for `enabled_if` fields in the `executable` and `install` stanzas than in the `library` stanza. When using dune lang < 2.6, the usage of forbidden variables in executables stanzas with only trigger a warning to maintain compatibility. (ocaml/dune#3408 and ocaml/dune#3496, fixes ocaml/dune#3354, @voodoos) - Insert a constraint one the version of dune when the user explicitly specify the dependency on dune in the `dune-project` file (ocaml/dune#3434 , fixes ocaml/dune#3427, @diml) - Generate correct META files for sub-libraries (of the form `lib.foo`) that contain .js runtime files. (ocaml/dune#3445, @hhugo) - Add a `(no-infer ...)` action that prevents inference of targets and dependencies in actions. (ocaml/dune#3456, fixes ocaml/dune#2006, @roddyyaga) - Correctly infer targets for the `diff?` action. (ocaml/dune#3457, fixes ocaml/dune#2990, @greedy) - Fix `$ dune print-rules` crashing (ocaml/dune#3459, fixes ocaml/dune#3440, @rgrinberg) - Simplify js_of_ocaml rules using js_of_ocaml.3.6 (ocaml/dune#3375, @hhugo) - Add a new `ocaml-merlin` subcommand that can be used by Merlin to get configuration directly from dune instead of using `.merlin` files. (ocaml/dune#3395, @voodoos) - Remove experimental variants feature and make default implementations part of the language (ocaml/dune#3491, fixes ocaml/dune#3483, @rgrinberg)

voodoos requested review from ejgallego, emillon, nojb and rgrinberg as code owners April 21, 2020 15:15

voodoos force-pushed the install-enabled_if branch from 9c6fcde to cc776cd Compare April 21, 2020 15:16

voodoos changed the title ~~Add enabled_if to install stanzas and enfore variable restrictions~~ Add enabled_if to install stanzas and enforce variable restrictions Apr 21, 2020

voodoos force-pushed the install-enabled_if branch from f93946c to 4a432c0 Compare April 22, 2020 08:42

rgrinberg reviewed Apr 22, 2020

View reviewed changes

src/dune/dune_file.ml Outdated Show resolved Hide resolved

rgrinberg reviewed Apr 22, 2020

View reviewed changes

src/dune/dune_file.ml Outdated Show resolved Hide resolved

voodoos added 11 commits April 23, 2020 11:05

Add parsing for enabled_if in install stanzas

c3ab04a

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Interpret enabled_if for install stanza

cc9ec52

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Bump dune lang version

fa948d7

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Upstream var checking into enabled_if's decoder

c984d7a

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Update tests and add one for forbidden var in enabled_if

32b7426

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Enfore same restrictions to eif in exec and install

3751700

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Add tests illustrating install enable_if and new restrictions

ab27523

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Promote line-change in test

c519852

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Add entry to CHANGES

212a305

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Make allowed var min version specific to exe and install stanzas

c10db20

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Move enabled_if decoder to it's own module.

b8b805a

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

voodoos force-pushed the install-enabled_if branch from 56458a9 to b8b805a Compare April 23, 2020 09:05

rgrinberg reviewed Apr 23, 2020

View reviewed changes

test/blackbox-tests/test-cases/enabled_if-exec/install/dune Outdated Show resolved Hide resolved

voodoos added 2 commits April 24, 2020 10:57

Correct tests indentation

730073b

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

Update cram tests accordingly

0504a5a

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

samoht mentioned this pull request May 5, 2020

Use dune to build the unikernel mirage/mirage#1153

Closed

Merge branch 'master' into install-enabled_if

672d6fc

rgrinberg merged commit 8b0a4c1 into ocaml:master May 6, 2020

samoht mentioned this pull request May 18, 2020

Regression in enabled_if for executables #3494

Closed

rgrinberg mentioned this pull request Jun 6, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.6.0) ocaml/opam-repository#16585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `enabled_if` to `install` stanzas and enforce variable restrictions#3408

Add `enabled_if` to `install` stanzas and enforce variable restrictions#3408
rgrinberg merged 14 commits intoocaml:masterfrom
voodoos:install-enabled_if

voodoos commented Apr 21, 2020

Uh oh!

rgrinberg commented Apr 21, 2020

Uh oh!

voodoos commented Apr 22, 2020

Uh oh!

Uh oh!

Uh oh!

voodoos commented Apr 23, 2020 •

edited

Loading

Uh oh!

Uh oh!

rgrinberg commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

voodoos commented Apr 21, 2020

Uh oh!

rgrinberg commented Apr 21, 2020

Uh oh!

voodoos commented Apr 22, 2020

Uh oh!

Uh oh!

Uh oh!

voodoos commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

voodoos commented Apr 23, 2020 •

edited

Loading