Format dune files in core by nojb · Pull Request #3536 · ocaml/dune

nojb · 2020-06-08T18:50:54Z

This was discussed in the weekly meeting https://github.com/ocaml/dune/wiki/dev-meeting-20200429

Closes: #3420

ghost

LGTM!

nojb · 2020-06-09T10:31:12Z

LGTM!

Thanks! I will update the tests and add a Changes entry.

ghost · 2020-06-09T10:45:06Z

BTW, thinking about it ideally we would do the formatting via an action. i.e. instead of having the build action be Write_file (<file>, <formatted-contents>), it would be Format_dune_file <input>. This way the contents of the file doesn't appear in the build rule which then has a smaller memory footprint, which could matter for large builds.

nojb · 2020-06-09T10:47:40Z

BTW, thinking about it ideally we would do the formatting via an action. i.e. instead of having the build action be Write_file (<file>, <formatted-contents>), it would be Format_dune_file <input>. This way the contents of the file doesn't appear in the build rule which then has a smaller memory footprint, which could matter for large builds.

Got it. This means adding a new constructor the Action.t type, right?

ghost · 2020-06-09T10:58:17Z

Yep, that's exactly what it means

nojb · 2020-06-09T11:33:39Z

I pushed a commit with this change.

nojb · 2020-06-09T11:34:34Z

src/dune/action_ast.ml

            , Dune_lang.Syntax.since Stanza.syntax (2, 6) >>> t >>| fun t ->
              No_infer t )
+          ; ( "format-dune-file"
+            , Dune_lang.Syntax.since Stanza.syntax (2, 7) >>>


I versioned the action, even though it is not supposed to be used by users.

In fact, we don't need to parse it. We do that for a few other private actions.

OK, I took this out.

nojb · 2020-06-09T11:35:12Z

src/dune/action.ml

    | Dynamic_run _ -> true
    | System _ -> true
    | Bash _ -> true
+    | Format_dune_file _ -> memoize


Wasn't sure about this one.

That seems good

Basically this means that:

the action would not be sandboxed (no need since we don't run external commands)

the result would go in the shared cache

the result would go in the distributed shared cache

Got it, thanks!

ghost

Looks all good, thanks!

nojb · 2020-06-09T14:29:07Z

Travis seems to have disappeared...

ghost · 2020-06-09T15:19:19Z

There was one build queued but with the wrong commit number, I just canceled it. Though the build of master seems to be broken because of ppx_expect, looking at it now

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

nojb · 2020-06-12T11:35:27Z

CI green, merging. Thanks @jeremiedimino for fixing the CI!

ghost · 2020-06-12T11:53:07Z

No problem!

@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 emillon as a code owner June 8, 2020 18:50

This was referenced Jun 8, 2020

dune format-dune-file doesn't respect the formatting stanza #3516

Open

Do not look up dune in PATH #3421

Closed

ghost approved these changes Jun 9, 2020

View reviewed changes

nojb commented Jun 9, 2020

View reviewed changes

nojb force-pushed the format_dune_files_in_core branch from eb8d75b to 8c0a8f8 Compare June 9, 2020 11:59

ghost approved these changes Jun 9, 2020

View reviewed changes

nojb closed this Jun 9, 2020

nojb reopened this Jun 9, 2020

nojb force-pushed the format_dune_files_in_core branch from 8c0a8f8 to 44525a4 Compare June 9, 2020 14:25

nojb closed this Jun 9, 2020

nojb reopened this Jun 9, 2020

nojb added 4 commits June 9, 2020 21:31

Format dune files in core (format-dune-file command no longer used)

730256a

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Accept tests

7f05d65

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

CHANGES.md

8cb2fae

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Use specific action format-dune-file

54335e3

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

nojb force-pushed the format_dune_files_in_core branch from 44525a4 to 54335e3 Compare June 9, 2020 19:31

nojb closed this Jun 12, 2020

nojb reopened this Jun 12, 2020

Merge branch 'master' into format_dune_files_in_core

f80ecaf

nojb merged commit 882023b into ocaml:master Jun 12, 2020

nojb deleted the format_dune_files_in_core branch June 12, 2020 11:35

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 Nov 13, 2020

Add versioning for dune auto-formatting style #3946

Merged

Conversation

nojb commented Jun 8, 2020

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

nojb commented Jun 9, 2020

Uh oh!

ghost commented Jun 9, 2020

Uh oh!

nojb commented Jun 9, 2020

Uh oh!

ghost commented Jun 9, 2020

Uh oh!

nojb commented Jun 9, 2020

Uh oh!

nojb Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

nojb Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

nojb Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

nojb Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

nojb commented Jun 9, 2020

Uh oh!

ghost commented Jun 9, 2020

Uh oh!

nojb commented Jun 12, 2020

Uh oh!

ghost commented Jun 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant