Allow (subdir ...) in dune files used via (include ...) #3676

nojb · 2020-08-04T20:04:33Z

Currently, the (include ...) stanza is processed after the (subdir ...) stanzas have been removed from the input. This makes it impossible to support (subdir ...) in a file used via (include ...). The PR adjusts things so that the (include ...) stanza is processed simultaneously with (subdir ...), so that this is allowed.

I found the code that handles (subdir ...) a bit tricky, so would appreciate a review by someone familiar with it. Thanks!

Fixes #3419

rgrinberg · 2020-08-04T20:07:07Z

Doesn't this break generating include in ocaml syntax dune files?

nojb · 2020-08-04T20:17:03Z

Doesn't this break generating include in ocaml syntax dune files?

Good catch, it does :)

Will look into it.

rgrinberg · 2020-08-04T20:19:30Z

We'll probably need to evaluate include in both stages (file_tree and dune_file).

rgrinberg · 2020-08-04T20:21:10Z

I found the code that handles (subdir ...) a bit tricky, so would appreciate a review by someone familiar with it. Thanks!

Sorry about that. I can try to add some comments to the parts you found tricky.

nojb · 2020-08-04T20:36:59Z

I found the code that handles (subdir ...) a bit tricky, so would appreciate a review by someone familiar with it. Thanks!

Sorry about that. I can try to add some comments to the parts you found tricky.

No need! I meant that it took me some time to figure out how the logic for (subtree ...) worked, but the code itself is very clear :)

nojb · 2020-08-05T00:11:49Z

We'll probably need to evaluate include in both stages (file_tree and dune_file).

I pushed a commit doing this.

rgrinberg · 2020-08-05T08:22:12Z

There seems to be a failure in CI:

File "test/blackbox-tests/test-cases/dialects.t/bad1/dune-project", line 9, characters 1-74:
395 9 |  (name d)
39610 |  (implementation (extension foo2))
39711 |  (interface (extension bar2)))
398Error: dialect "d" is already defined

nojb · 2020-08-05T08:22:50Z

There seems to be a failure in CI:

Ya, I'm looking into it, thanks.

nojb · 2020-08-05T08:37:00Z

There seems to be a failure in CI:

File "test/blackbox-tests/test-cases/dialects.t/bad1/dune-project", line 9, characters 1-74:
395 9 |  (name d)
39610 |  (implementation (extension foo2))
39711 |  (interface (extension bar2)))
398Error: dialect "d" is already defined

It is strange: this is the expected output of the test. On the other hand, I can only reproduce the problem in this branch, not in master...

nojb · 2020-08-05T09:37:59Z

I think I understand what is happening: with this patch, (include ...) is evaluated more eagerly. In particular, it seem that the include in test/blackbox-tests/dune is being loaded too eagerly, which causes the error above as soon as dune is being built.

nojb · 2020-08-05T10:43:11Z

OK, I think I found the bug: I am not correctly merging the data_only_dir stanzas from the included files.

nojb · 2020-08-05T12:23:08Z

OK, I reworked the patch to fix the problem. I modified the approach a bit: now I process (include ...) in a separate pass before processing (subdir ...) and the rest of the stanzas. The first pass removes any (include ...) node and replaces it by the contents of the included file in the AST, which is then passed onwards and processed as usual.

nojb · 2020-08-05T16:32:08Z

I rebased and cleaned up the history. This is ready for review. Thanks!

rgrinberg

Looks good. I'm surprised how simple this turned out to be.

@jeremiedimino do you think this feature should require 2.7? I'm leaning towards making it available since subdir was introduced and just treating this as a bugfix.

This needs a CHANGES entry.

nojb · 2020-08-05T20:45:45Z

Looks good. I'm surprised how simple this turned out to be.

Thanks for the quick review!

This needs a CHANGES entry.

Done!

nojb · 2020-08-06T05:54:52Z

I realised that there may be a small adjustment to make. Suppose that we have:

(subdir a (include dune.inc))

Is the file dune.inc supposed to be found inside a or in the current directory? At the moment, the PR looks for it within the current directory, not within a. It should be easy to adjust, but wanted to confirm first.

rgrinberg · 2020-08-06T05:58:53Z

It should be relative to directory “a" I think Rudi.

…

On Aug 5, 2020, 10:55 PM -0700, Nicolás Ojeda Bär ***@***.***>, wrote: I realised that there may be a small adjustment to make. Suppose that we have: (subdir a (include dune.inc)) Is the file dune.inc supposed to be found inside a or in the current directory? At the moment, the PR looks for it within the current directory, not within a. It should be easy to adjust, but wanted to confirm first. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

nojb · 2020-08-06T06:15:25Z

It should be relative to directory “a" I think Rudi.

Indeed, it's that. I pushed a commit (and a corresponding test) for this. It should be good to go now.

rgrinberg · 2020-08-06T17:50:48Z

@nojb I just talked to Jeremie and we decided that it's best to gate this feature behind 2.7. If users end up relying on this feature without putting the appropriate constraint in their opam file, it will lead to some issues down the road.

If it's not too hard, could you add the version check?

src/dune/sub_dirs.ml

nojb · 2020-08-06T18:24:39Z

Version check (+ test) added.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

rgrinberg · 2020-08-11T18:50:24Z

@nojb are you still working on this PR?

nojb · 2020-08-11T19:46:33Z

@nojb are you still working on this PR?

No, the PR is ready to go as far as I am concerned.

rgrinberg · 2020-08-11T20:19:35Z

Okay. Then I will merge it today unless you'll do the honors and do it before me.

nojb · 2020-08-11T20:21:29Z

Okay. Then I will merge it today unless you'll do the honors and do it before me.

Thanks. I'll let you do it; I am away from home with horrible internet service :)

@NathanReb

…lugin, dune-private-libs and dune-glob (2.7.0) CHANGES: - Write intermediate files in a `.mdx` folder for each `mdx` stanza to prevent the corresponding actions to be executed as part of the `@all` alias (ocaml/dune#3659, @NathanReb) - Read Coq flags from `env` (ocaml/dune#3547 , fixes ocaml/dune#3486, @gares) - Allow bisect_ppx to be enabled/disabled via dune-workspace. (ocaml/dune#3404, @stephanieyou) - Formatting of dune files is now done in the executing dune process instead of in a separate process. (ocaml/dune#3536, @nojb) - Add a `--debug-artifact-substution` flag to help debug problem with version not being captured by `dune-build-info` (ocaml/dune#3589, @jeremiedimino) - Allow the use of the `context_name` variable in the `enabled_if` fields of executable(s) and install stanzas. (ocaml/dune#3568, fixes ocaml/dune#3566, @voodoos) - Fix compatibility with OCaml 4.12.0 when compiling empty archives; no .a file is generated. (ocaml/dune#3576, @dra27) - `$ dune utop` no longer tries to load optional libraries that are unavailable (ocaml/dune#3612, fixes ocaml/dune#3188, @anuragsoni) - Fix dune-build-info on 4.10.0+flambda (ocaml/dune#3599, @emillon, @jeremiedimino). - Allow multiple libraries with `inline_tests` to be defined in the same directory (ocaml/dune#3621, @rgrinberg) - Run exit hooks in jsoo separate compilation mode (ocaml/dune#3626, fixes ocaml/dune#3622, @rgrinberg) - Add (alias ...), (mode ...) fields to (copy_fields ...) stanza (ocaml/dune#3631, @nojb) - (copy_files ...) now supports copying files from outside the workspace using absolute file names (ocaml/dune#3639, @nojb) - Dune does not use `ocamlc` as an intermediary to call C compiler anymore. Configuration flags `ocamlc_cflags` and `ocamlc_cppflags` are always prepended to the compiler arguments. (ocaml/dune#3565, fixes ocaml/dune#3346, @voodoos) - Revert the build optimization in ocaml/dune#2268. This optimization slows down building individual executables when they're part of an `executables` stanza group (ocaml/dune#3644, @rgrinberg) - Use `{dev}` rather than `{pinned}` in the generated `.opam` file. (ocaml/dune#3647, @kit-ty-kate) - Insert correct extension name when editing `dune-project` files. Previously, dune would just insert the stanza name. (ocaml/dune#3649, fixes ocaml/dune#3624, @rgrinberg) - Fix crash when evaluating an `mdx` stanza that depends on unavailable packages. (ocaml/dune#3650, @craigfe) - Fix typo in `cache-check-probablity` field in dune config files. This field now requires 2.7 as it wasn't usable before this version. (ocaml/dune#3652, @edwintorok) - Add `"odoc" {with-doc}` to the dependencies in the generated `.opam` files. (ocaml/dune#3667, @kit-ty-kate) - Do not allow user actions to capture dune's stdin (ocaml/dune#3677, fixes ocaml/dune#3672, @rgrinberg) - `(subdir ...)` stanzas can now appear in dune files used via `(include ...)`. (ocaml/dune#3676, @nojb)

@NathanReb

…lugin, dune-private-libs and dune-glob (2.7.0) CHANGES: - Write intermediate files in a `.mdx` folder for each `mdx` stanza to prevent the corresponding actions to be executed as part of the `@all` alias (ocaml/dune#3659, @NathanReb) - Read Coq flags from `env` (ocaml/dune#3547 , fixes ocaml/dune#3486, @gares) - Add instrumentation framework to toggle instrumentation by `bisect_ppx`, `landmarks`, etc, via dune-workspace and/or the command-line. (ocaml/dune#3404, ocaml/dune#3526 @stephanieyou, @nojb) - Formatting of dune files is now done in the executing dune process instead of in a separate process. (ocaml/dune#3536, @nojb) - Add a `--debug-artifact-substution` flag to help debug problem with version not being captured by `dune-build-info` (ocaml/dune#3589, @jeremiedimino) - Allow the use of the `context_name` variable in the `enabled_if` fields of executable(s) and install stanzas. (ocaml/dune#3568, fixes ocaml/dune#3566, @voodoos) - Fix compatibility with OCaml 4.12.0 when compiling empty archives; no .a file is generated. (ocaml/dune#3576, @dra27) - `$ dune utop` no longer tries to load optional libraries that are unavailable (ocaml/dune#3612, fixes ocaml/dune#3188, @anuragsoni) - Fix dune-build-info on 4.10.0+flambda (ocaml/dune#3599, @emillon, @jeremiedimino). - Allow multiple libraries with `inline_tests` to be defined in the same directory (ocaml/dune#3621, @rgrinberg) - Run exit hooks in jsoo separate compilation mode (ocaml/dune#3626, fixes ocaml/dune#3622, @rgrinberg) - Add (alias ...), (mode ...) fields to (copy_fields ...) stanza (ocaml/dune#3631, @nojb) - (copy_files ...) now supports copying files from outside the workspace using absolute file names (ocaml/dune#3639, @nojb) - Dune does not use `ocamlc` as an intermediary to call C compiler anymore. Configuration flags `ocamlc_cflags` and `ocamlc_cppflags` are always prepended to the compiler arguments. (ocaml/dune#3565, fixes ocaml/dune#3346, @voodoos) - Revert the build optimization in ocaml/dune#2268. This optimization slows down building individual executables when they're part of an `executables` stanza group (ocaml/dune#3644, @rgrinberg) - Use `{dev}` rather than `{pinned}` in the generated `.opam` file. (ocaml/dune#3647, @kit-ty-kate) - Insert correct extension name when editing `dune-project` files. Previously, dune would just insert the stanza name. (ocaml/dune#3649, fixes ocaml/dune#3624, @rgrinberg) - Fix crash when evaluating an `mdx` stanza that depends on unavailable packages. (ocaml/dune#3650, @craigfe) - Fix typo in `cache-check-probablity` field in dune config files. This field now requires 2.7 as it wasn't usable before this version. (ocaml/dune#3652, @edwintorok) - Add `"odoc" {with-doc}` to the dependencies in the generated `.opam` files. (ocaml/dune#3667, @kit-ty-kate) - Do not allow user actions to capture dune's stdin (ocaml/dune#3677, fixes ocaml/dune#3672, @rgrinberg) - `(subdir ...)` stanzas can now appear in dune files used via `(include ...)`. (ocaml/dune#3676, @nojb)

nojb requested a review from rgrinberg as a code owner August 4, 2020 20:04

nojb force-pushed the subdir_in_include branch from cf1ae98 to b3b27ce Compare August 5, 2020 12:20

nojb force-pushed the subdir_in_include branch 4 times, most recently from cc976e8 to b7da698 Compare August 5, 2020 16:26

rgrinberg approved these changes Aug 5, 2020

View reviewed changes

nojb force-pushed the subdir_in_include branch from dd7db2a to 1405faf Compare August 6, 2020 06:35

rgrinberg reviewed Aug 6, 2020

View reviewed changes

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

nojb force-pushed the subdir_in_include branch from 1d562b0 to f238483 Compare August 6, 2020 18:23

nojb force-pushed the subdir_in_include branch from f238483 to 4df0aad Compare August 6, 2020 18:27

rgrinberg added this to the 2.7 milestone Aug 7, 2020

nojb added 8 commits August 7, 2020 12:51

Process (include ...) before (subdir ...)

51739ab

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Add test

9addb5b

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

CHANGES.md

b9f72a7

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Fix handling of (include ...) within (subdir ...)

072a10f

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Add test

54a042a

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Simplify

96129bb

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Guard against version < 2.7

e1a3830

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Update tests

37f6af9

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb force-pushed the subdir_in_include branch from 4df0aad to 37f6af9 Compare August 7, 2020 10:51

rgrinberg merged commit 057fa87 into ocaml:master Aug 12, 2020

rgrinberg mentioned this pull request Aug 14, 2020

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

Merged

nojb mentioned this pull request Aug 19, 2020

dir stanza does not work in include stanza #2711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow (subdir ...) in dune files used via (include ...) #3676

Allow (subdir ...) in dune files used via (include ...) #3676

nojb commented Aug 4, 2020

rgrinberg commented Aug 4, 2020

nojb commented Aug 4, 2020

rgrinberg commented Aug 4, 2020

rgrinberg commented Aug 4, 2020

nojb commented Aug 4, 2020

nojb commented Aug 5, 2020

rgrinberg commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

rgrinberg left a comment

nojb commented Aug 5, 2020

nojb commented Aug 6, 2020

rgrinberg commented Aug 6, 2020 via email

nojb commented Aug 6, 2020

rgrinberg commented Aug 6, 2020

nojb commented Aug 6, 2020

rgrinberg commented Aug 11, 2020

nojb commented Aug 11, 2020

rgrinberg commented Aug 11, 2020

nojb commented Aug 11, 2020

Allow (subdir ...) in dune files used via (include ...) #3676

Allow (subdir ...) in dune files used via (include ...) #3676

Conversation

nojb commented Aug 4, 2020

rgrinberg commented Aug 4, 2020

nojb commented Aug 4, 2020

rgrinberg commented Aug 4, 2020

rgrinberg commented Aug 4, 2020

nojb commented Aug 4, 2020

nojb commented Aug 5, 2020

rgrinberg commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

nojb commented Aug 5, 2020

rgrinberg left a comment

Choose a reason for hiding this comment

nojb commented Aug 5, 2020

nojb commented Aug 6, 2020

rgrinberg commented Aug 6, 2020 via email

nojb commented Aug 6, 2020

rgrinberg commented Aug 6, 2020

nojb commented Aug 6, 2020

rgrinberg commented Aug 11, 2020

nojb commented Aug 11, 2020

rgrinberg commented Aug 11, 2020

nojb commented Aug 11, 2020