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.

Sign up for GitHub

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

Jump to bottom

Support enabled_if in more places #1387

Merged
2 commits merged into from Oct 3, 2018
Merged

Support enabled_if in more places #1387

2 commits merged into from Oct 3, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 3, 2018

This PR adds support for enabled_if in more places:

  • rule
  • menhir
  • ocamllex
  • ocamlyacc

I refactored Blang a bit as well and added some tests.

@jeremiedimino
Support enabled_if in more places
dc7798d 
- rule
- menhir
- ocamllex
- ocamlyacc

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost ghost requested review from emillon and rgrinberg as code owners October 3, 2018 11:53
@nojb
Copy link
Collaborator

nojb commented Oct 3, 2018 •
edited

What about executable (and even library)? Is it more complicated technically?

@ghost
Copy link
Author

ghost commented Oct 3, 2018

@nojb it is slightly more complicated, as the list of libraries and public executables is computed before creating the super context, which is used for the evaluation of the boolean language. It's probably just a refactoring away, but it requires more work. There are also odd cases to handle, such as this one:

(library
 (name a)
 (enabled_if %{lib-available:b}))

(library
 (name b)
 (enabled_if %{lib-available:a}))

@ghost ghost merged commit 7db865e into ocaml:master Oct 3, 2018
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 4, 2018
@rgrinberg
[new release] dune (1.4.0)
f5fc609 
CHANGES:

- Do not fail if the output of `ocamlc -config` doesn't include
  `standard_runtime` (ocaml/dune#1326, @diml)

- Let `Configurator.V1.C_define.import` handle negative integers
  (ocaml/dune#1334, @Chris00)

- Re-execute actions when a target is modified by the user inside
  `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml)

- Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml)

- Fix bad interaction between multi-directory libraries the `menhir`
  stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml)

- Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon)

- Better error message when using `(self_build_stubs_archive ...)` and
  `(c_names ...)` or `(cxx_names ...)` simultaneously.
  (ocaml/dune#1375, fix ocaml/dune#1306, @nojb)

- Improve name detection for packages when the prefix isn't an actual package
  (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg)

- Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg)

- Do not remove flags when compiling compatibility modules for wrapped mode
  (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg)

- Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc)

- Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`,
  `ocamlyacc` (ocaml/dune#1387, @diml)

- Exit gracefully when a signal is received (ocaml/dune#1366, @diml)

- Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344,
  @rgrinberg)

- Allow to use libraries `bytes`, `result` and `uchar` without `findlib`
  installed (ocaml/dune#1391, @nojb)
@rgrinberg rgrinberg mentioned this pull request Oct 4, 2018
Closed
@trefis
Copy link
Collaborator

trefis commented Oct 4, 2018

I don't think this does exactly what it claims: the parsing of ocamllex and ocamlyacc stanza wasn't updated.

@rgrinberg
Copy link
Member

Good point. Actually, how would it even work for ocamllex and ocamlyacc. If we can't generate the ml files, then the build will simply fail.

@ghost
Copy link
Author

ghost commented Oct 4, 2018

Indeed, I forgot about these. @rgrinberg the idea is that you can have several ocamllex stanza and select only one via enabled_if fields in each of them.

@trefis
Copy link
Collaborator

trefis commented Oct 4, 2018

FTR that’s precisely merlin’s use case.

@ghost
Copy link
Author

ghost commented Oct 4, 2018

Fixed in #1399

@rgrinberg
Copy link
Member

I see. Although tbh, this is where something like (cond ..) or (match ..) would be better suited than enabled_if. I understand that extending enabled_if is a lot simpler though.

@trefis
Copy link
Collaborator

trefis commented Oct 4, 2018

I didn't know about either of this. Can you point me to a place where they are discussed?

@rgrinberg
Copy link
Member

#924 (comment)

Not exactly the same context, but I think the point still stands.

@trefis
Copy link
Collaborator

trefis commented Oct 4, 2018

I had completely forgotten.
Thank you!

@trefis
Copy link
Collaborator

trefis commented Oct 5, 2018

Btw, having enabled_if on ocamllex stanzas also make sense when the mode is promote.
So I think it's still good to have

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 10, 2018
@rgrinberg
[new release] dune (1.4.0)
9e08c98 
CHANGES:

- Do not fail if the output of `ocamlc -config` doesn't include
  `standard_runtime` (ocaml/dune#1326, @diml)

- Let `Configurator.V1.C_define.import` handle negative integers
  (ocaml/dune#1334, @Chris00)

- Re-execute actions when a target is modified by the user inside
  `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml)

- Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml)

- Fix bad interaction between multi-directory libraries the `menhir`
  stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml)

- Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon)

- Better error message when using `(self_build_stubs_archive ...)` and
  `(c_names ...)` or `(cxx_names ...)` simultaneously.
  (ocaml/dune#1375, fix ocaml/dune#1306, @nojb)

- Improve name detection for packages when the prefix isn't an actual package
  (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg)

- Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg)

- Do not remove flags when compiling compatibility modules for wrapped mode
  (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg)

- Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc)

- Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`,
  `ocamlyacc` (ocaml/dune#1387, @diml)

- Exit gracefully when a signal is received (ocaml/dune#1366, @diml)

- Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344,
  @rgrinberg)

- Allow to use libraries `bytes`, `result` and `uchar` without `findlib`
  installed (ocaml/dune#1391, @nojb)

- Take argument to self_build_stubs_archive into account. (ocaml/dune#1395, @nojb)

- Fix bad interaction between `env` customization and vendored
  projects: when a vendored project didn't have its own `env` stanza,
  the `env` stanza from the enclosing project was in effect (ocaml/dune#1408,
  @diml)

- Fix stop early bug when scanning for watermarks (ocaml/dune#1423, @diml)
@rgrinberg rgrinberg mentioned this pull request Oct 10, 2018
Merged
@dra27 dra27 mentioned this pull request Oct 15, 2018
Closed
shonfeder pushed a commit to shonfeder/dune that referenced this pull request Dec 31, 2018
@jeremiedimino @shonfeder
Support enabled_if in more places (ocaml#1387)
cbea943 
- rule
- menhir
- ocamllex
- ocamlyacc

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@db4
Copy link

db4 commented Jul 31, 2020

It would be great if this functionality were mentioned in the docs (for rule, menhir, ocamllex, and ocamlyacc stanzas)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon Awaiting requested review from emillon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nojb @trefis @rgrinberg @db4 @jeremiedimino