Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow enabled_if in executable(s) stanza #3137

Merged
merged 6 commits into from
Feb 14, 2020

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Feb 14, 2020

Fix #1690

This PR add support for enabled_if in executable(s) stanza.

Any attempt to build an executable whose enable_if field evaluates to false will raise a "Don't know how to build" error.

I am unsure whether or not that behaviour is the correct one. Other options are:

  • do nothing
  • warn the user that the enable_if field is not satisfied.

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

Any attempt to build an executable whose enable_if field evaluates to false will raise a "Don't know how to build" error.

Isn't this the same for libraries with enabled_if that evaluates to false?

@voodoos
Copy link
Collaborator Author

voodoos commented Feb 14, 2020

Yes it is indeed the case.

Current behaviour is: nothing for aliases, "don't know" for rules and libraries, and a more precise error when depending on a disabled library.

From enabled_if/run.t (which is missing the case "explicitly build a disabled library"):

This alias is disabled, building it should do nothing:
  $ dune build @x

This one is enabled:
  $ dune build @y
  Building alias y

This rule is disabled, trying to build a should fail:
  $ dune build a
  Error: Don't know how to build a
  Hint: did you mean b?
  [1]

This one is enabled:
  $ dune build b
  Building file b

Test the enabled_if field for libraries:

  $ dune build main.exe
  File "dune", line 33, characters 12-15:
  33 |  (libraries foo))
                   ^^^
  Error: Library "foo" in _build/default is hidden (unsatisfied 'enabled_if').
  Hint: try: dune external-lib-deps --missing main.exe
  [1]

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

i think this is sufficient for now. thanks.

@rgrinberg rgrinberg merged commit c6aed04 into ocaml:master Feb 14, 2020
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Feb 15, 2020
…lugin, dune-private-libs and dune-glob (2.3.0)

CHANGES:

- Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes
  ocaml/dune#3046, @shonfeder)

- `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103,
  fixes ocaml/dune#3088, @shonfeder)

- Avoid linear walk to detect children, this should greatly improve
  performance when a target has a large number of dependencies (ocaml/dune#2959,
  @ejgallego, @aalekseyev, @Armael)

- [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of
  Coq's stdlib (ocaml/dune#3096, @ejgallego)

- [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego)

- Better error reporting for "data only" and "vendored" dirs. Using these with
  anything else than a strict subdirectory or `*` will raise an error. The
  previous behavior was to just do nothing  (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos)

- Fix bootstrap on bytecode only switches on windows or where `-j1` is set.
  (ocaml/dune#3112, @xclerc, @rgrinberg)

- Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690
  @voodoos)

- Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them
  to be used to fail (ocaml/dune#3138, @rgrinberg)

- Introduce a `strict_package_deps` mode that verifies that dependencies between
  packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Feb 15, 2020
…lugin, dune-private-libs and dune-glob (2.3.0)

CHANGES:

- Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes
  ocaml/dune#3046, @shonfeder)

- `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103,
  fixes ocaml/dune#3088, @shonfeder)

- Avoid linear walk to detect children, this should greatly improve
  performance when a target has a large number of dependencies (ocaml/dune#2959,
  @ejgallego, @aalekseyev, @Armael)

- [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of
  Coq's stdlib (ocaml/dune#3096, @ejgallego)

- [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego)

- Better error reporting for "data only" and "vendored" dirs. Using these with
  anything else than a strict subdirectory or `*` will raise an error. The
  previous behavior was to just do nothing  (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos)

- Fix bootstrap on bytecode only switches on windows or where `-j1` is set.
  (ocaml/dune#3112, @xclerc, @rgrinberg)

- Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690
  @voodoos)

- Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them
  to be used to fail (ocaml/dune#3138, @rgrinberg)

- Introduce a `strict_package_deps` mode that verifies that dependencies between
  packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117)

- Make sure the `@all` alias is defined when no `dune` file is present
  in a directory (ocaml/dune#2946, fix ocaml/dune#2927, @diml)
@ghost
Copy link

ghost commented Feb 17, 2020

Did you check what happens if the executable is public? From a quick glance over the path, it seems to me that Dune would try to build the executable and fail.

@voodoos
Copy link
Collaborator Author

voodoos commented Feb 17, 2020

I can confirm that attempting to build @install a project including disabled public executables will trigger a don't know how to build error.

What do you think should be the correct behaviour ?

@ghost
Copy link

ghost commented Feb 19, 2020

I think we should exclude executables with unsatified enabled_if clauses from installation. And while reading this code I found a very embarrassing bug: #3163

Bascially, you should interpret the enabled_if in the keep_if function that #3163 modifies.

mgree pushed a commit to mgree/opam-repository that referenced this pull request Feb 19, 2020
…lugin, dune-private-libs and dune-glob (2.3.0)

CHANGES:

- Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes
  ocaml/dune#3046, @shonfeder)

- `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103,
  fixes ocaml/dune#3088, @shonfeder)

- Avoid linear walk to detect children, this should greatly improve
  performance when a target has a large number of dependencies (ocaml/dune#2959,
  @ejgallego, @aalekseyev, @Armael)

- [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of
  Coq's stdlib (ocaml/dune#3096, @ejgallego)

- [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego)

- Better error reporting for "data only" and "vendored" dirs. Using these with
  anything else than a strict subdirectory or `*` will raise an error. The
  previous behavior was to just do nothing  (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos)

- Fix bootstrap on bytecode only switches on windows or where `-j1` is set.
  (ocaml/dune#3112, @xclerc, @rgrinberg)

- Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690
  @voodoos)

- Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them
  to be used to fail (ocaml/dune#3138, @rgrinberg)

- Introduce a `strict_package_deps` mode that verifies that dependencies between
  packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117)

- Make sure the `@all` alias is defined when no `dune` file is present
  in a directory (ocaml/dune#2946, fix ocaml/dune#2927, @diml)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] enabled_if in executable stanza?
2 participants