Tests: do not use ocaml syntax in the dune file #1082

emillon · 2019-10-16T15:25:27Z

Hi,

dune cannot directly generate a list of rules depending on things like file system layout, but there are several workarounds for this.

The first one is "ocaml syntax" - a dune file can be written as an OCaml script which builds a large string depending on its execution.

The second one is dune file generation: an executable generates a dune.inc (which is stored in the git repository) which is included in the main dune file. An extra diff action makes sure that dune.inc stays up to date.

This PR switches from ocaml syntax to dune file generation. Here's a list of pros & cons:

👍 Reasons to switch to dune file generation

ocaml syntax could go away in a future dune version, and does not work well with incremental builds
it makes it possible to use external libraries in the code generation, such as stdio in this example.
it makes debugging easier since it's only a (generated) file to inspect

👎 Reasons to stay on ocaml syntax

since the file is generated, it needs to be kept up to date. Concretely, it means that when a test file is created or delete, it will be necessary to run dune promote to update dune.inc. This seems to be rare enough to be workable.
the file is generated so it's visible in git and makes a large positive diffstat in this PR.

Let me know what you think.

Thanks!

facebook-github-bot · 2019-10-16T15:25:35Z

Hi emillon! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Julow

I think the pros overcome the cons !
Also makes dune slightly faster.

@gpetiot What do you think, as you wrote that script ?

test/passing/gen/gen.ml

gpetiot · 2019-10-16T16:12:08Z

I think the pros overcome the cons !
Also makes dune slightly faster.

@gpetiot What do you think, as you wrote that script ?

I will have a look tomorrow, but if I remember Hugo wrote this script.

gpetiot · 2019-10-17T07:35:54Z

The diff between the old dune file and the new gen/gen.ml looks goo to me.
Of course the shorter the generated file, the better, but I'm not proficient enough with the dune syntax to be of any use with that.
Don't forget to update CHANGES.md though.

Julow

Looks good !

Julow · 2019-10-17T09:17:55Z

test/passing/gen/gen.ml

+ (deps .ocamlformat %s)
+ (action
+   (with-outputs-to %%{targets}
+     (system "%%{bin:ocamlformat}%s %%{dep:%s} || true"))))


Unrelated but true won't work on Windows.
Also, I think we should not ignore errors here. Then the diff between the whole file and the error message is shown and can be promoted, which is not useful and looks bad.

Ah yes I forgot to mention that. Some tests do fail, though. What do you think about the following (in a separate PR):

by default we generate (run %{bin:ocamlformat} ...) (fails on error)

if the TEST.should-fail file exists, this is converted to (system "! ocamlformat ...") (fails on success

We can convert that to with-accepted-exit-codes in dune 2.

Yes that's a good idea.

`dune` cannot directly generate a list of rules depending on things like file system layout, but there are several workarounds for this. The first one is "ocaml syntax" - a dune file can be written as an OCaml script which builds a large string depending on its execution. The second one is dune file generation: an executable generates a dune.inc (which is stored in the git repository) which is included in the main dune file. An extra diff action makes sure that dune.inc stays up to date. This switches from ocaml syntax to dune file generation, for the following reasons: - ocaml syntax could go away in a future dune version, and does not work well with incremental builds - it makes it possible to use external libraries in the code generation, such as stdio in this example. - it makes debugging easier since it's only a (generated) file to inspect Some things to note: - since the file is generated, it needs to be kept up to date. Concretely, it means that when a test file is created or delete, it will be necessary to run dune promote to update dune.inc. This seems to be rare enough to be workable. - the file is generated so it's visible in git and makes a large positive diffstat in this cmmit. - `max-iters` is also set in .ocamlformat, which changes a test that was setting `--config` which overrides `OCAMLFORMAT`.

emillon · 2019-10-17T09:31:02Z

Just squashed

emillon · 2019-10-17T09:34:50Z

All right, let's do this!

emillon · 2019-10-17T09:35:07Z

I think this can get merged :)

CHANGES: #### Changes + Set "conventional" as the default profile (ocaml-ppx/ocamlformat#1060) (Guillaume Petiot) This new profile is made to better match the most used style and is encouraged. To continue using the previous default, use `profile = ocamlformat` in your `.ocamlformat`. + CLI: Allow both values of boolean options (ocaml-ppx/ocamlformat#1062) (Jules Aguillon) Now, both `--opt` and --no-opt` are available on the CLI for any boolean option "opt". Previously, only one of them were available depending on the default value. + Auto mode for `break-string-literals` (ocaml-ppx/ocamlformat#1057) (Guillaume Petiot) `wrap`, `newlines` and `newlines-and-wrap` values of `break-string-literals` are removed. `auto` replaces them, it is equivalent to `newlines-and-wrap`. + Dock collection brackets (ocaml-ppx/ocamlformat#1014) (Guillaume Petiot) `after-and-docked` value of `break-separators` is removed and is replaced by a new `dock-collection-brackets` option. + Preserve `begin` and `end` keywords in if-then-else (ocaml-ppx/ocamlformat#978) (Jules Aguillon) Previously, `begin`/`end` keywords around if-then-else branches were turned into parentheses. #### New features + Give a hint when warning 50 is raised (ocaml-ppx/ocamlformat#1111) (Guillaume Petiot) + Add a message when a config value is removed (ocaml-ppx/ocamlformat#1089) (Etienne Millon) Explain what replaces removed options and avoid printing a parsing error. + Implement `sequence-blank-line=preserve-one` for let bindings (ocaml-ppx/ocamlformat#1077) (Jules Aguillon) Preserve a blank line after `let .. in` when `sequence-blank-line` set to `preserve-one`. Previously, only blank lines after `;` could be preserved. + Parse toplevel directives (ocaml-ppx/ocamlformat#1020) (Jules Aguillon) Allow `#directives` in `.ml` files. Previously, files containing a directive needed to be parsed as "use file". The "use file" mode is removed and `--use-file` is now the same as `--impl`. + Don't require `--name`, require kind, forbid `--inplace`, allow `--check`, make `--enable-outside-detected-project` implicit when reading from stdin (ocaml-ppx/ocamlformat#1018) (Guillaume Petiot) + Parse code in docstrings (ocaml-ppx/ocamlformat#941) (Guillaume Petiot) Format OCaml code in cinaps-style comments `(*$ code *)` and code blocks in documentation comments `(** {[ code ]} *)`. + Parse documentation comments with Odoc (ocaml-ppx/ocamlformat#721) (Jules Aguillon) Formatting of documentation comments is more robust and support newer Odoc syntaxes. Internally, Odoc replaces Octavius as the documentation parser. #### Bug fixes + Fix unstabilizing comments on assignments (ocaml-ppx/ocamlformat#1093) (Guillaume Petiot) + Fix the default value documentation for `max-indent` (ocaml-ppx/ocamlformat#1105) (Guillaume Petiot) + Fix closing parenthesis exceeding the margin in function application (ocaml-ppx/ocamlformat#1098) (Jules Aguillon) + Missing break before attributes of `Pmty_with` (ocaml-ppx/ocamlformat#1103) (Josh Berdine) + Fix closing quote exceeding the margin (ocaml-ppx/ocamlformat#1096) (Jules Aguillon) + Fix break before the closing bracket of collections (exceeding the margin) (ocaml-ppx/ocamlformat#1073) (Guillaume Petiot) + Fix precedence of Dot wrt Hash (ocaml-ppx/ocamlformat#1058) (Guillaume Petiot) + Fix break in variant type definition to not exceed the margin (ocaml-ppx/ocamlformat#1064) (Guillaume Petiot) + Fix newlines and indentation in toplevel extension points (ocaml-ppx/ocamlformat#1054) (Guillaume Petiot) + Fix placement of doc comments around extensions (ocaml-ppx/ocamlformat#1052) (Jules Aguillon) + Inline extensions that do not break (ocaml-ppx/ocamlformat#1050) (Guillaume Petiot) + Add missing cut before attributes in type declarations (ocaml-ppx/ocamlformat#1051) (Guillaume Petiot) + Fix alignment of cases (ocaml-ppx/ocamlformat#1046) (Guillaume Petiot) + Fix blank line after comments at the end of lists (ocaml-ppx/ocamlformat#1045) (Guillaume Petiot) + Fix indexing operators precedence (ocaml-ppx/ocamlformat#1039) (Jules Aguillon) + Fix dropped comment after infix op (ocaml-ppx/ocamlformat#1030) (Guillaume Petiot) + No newline if the input is empty (ocaml-ppx/ocamlformat#1031) (Guillaume Petiot) + Fix unstable comments around attributes (ocaml-ppx/ocamlformat#1029) (Guillaume Petiot) + Fix extra blank line in sequence (ocaml-ppx/ocamlformat#1021) (Jules Aguillon) + Check functor arguments when computing placement of doc comments (ocaml-ppx/ocamlformat#1013) (Jules Aguillon) + Fix indentation of labelled args (ocaml-ppx/ocamlformat#1006) (Guillaume Petiot) + Fix linebreak between or-cases with comments when `break-cases=all` (ocaml-ppx/ocamlformat#1002) (Guillaume Petiot) + Fix unstable unattached doc comment in records (ocaml-ppx/ocamlformat#998) (Jules Aguillon) + Fix string literal changed (ocaml-ppx/ocamlformat#995) (Jules Aguillon) + Fix type variable (ocaml-ppx/ocamlformat#996) (Jules Aguillon) + Fix crash on extension sequence (ocaml-ppx/ocamlformat#992) (Guillaume Petiot) + Fix position of expressions regarding of comments in infix-op expressions (ocaml-ppx/ocamlformat#986) (Guillaume Petiot) + Escape special characters in external declaration (ocaml-ppx/ocamlformat#988) (Jules Aguillon) + Fix parens around constrained expr with attrs (ocaml-ppx/ocamlformat#987) (Guillaume Petiot) + Fix the margin, and correctly breaks comments (ocaml-ppx/ocamlformat#957) (Guillaume Petiot) + Fix formatting of custom indexing operators (ocaml-ppx/ocamlformat#975) (Guillaume Petiot) + Fix position of comments of labelled arrow types (ocaml-ppx/ocamlformat#976) (Guillaume Petiot) + No box around inline odoc styles (ocaml-ppx/ocamlformat#971) (Guillaume Petiot) + Fix boxing of collection expressions/patterns (ocaml-ppx/ocamlformat#960) (Guillaume Petiot) + Fix crash on record expr with pack fields (ocaml-ppx/ocamlformat#963) (Jules Aguillon) + Fix letop in subexpr (ocaml-ppx/ocamlformat#956) (hhugo) #### Internal + Take file kind from --name when formatting stdin (ocaml-ppx/ocamlformat#1119) (Jules Aguillon) + Make Fmt.t abstract (ocaml-ppx/ocamlformat#1109) (Jules Aguillon) + Future-proof Fmt API in case Fmt.t goes abstract (ocaml-ppx/ocamlformat#1106) (Etienne Millon) + Future-proof `Fmt` API in case `Fmt.t` goes abstract (ocaml-ppx/ocamlformat#1106) (Etienne Millon) + Optional names for formatting boxes in debug output (ocaml-ppx/ocamlformat#1083) (Guillaume Petiot) + Check ocamlformat error codes in the testsuite (ocaml-ppx/ocamlformat#1084) (Etienne Millon) + Clean `Translation_unit` (ocaml-ppx/ocamlformat#1078) (Guillaume Petiot) + Use dune file generation in test/passing/dune (ocaml-ppx/ocamlformat#1082) (Etienne Millon) + CI: factorize tests and check reason build (ocaml-ppx/ocamlformat#1079) (Guillaume Petiot) + Use short form for action in src/dune (ocaml-ppx/ocamlformat#1076) (Etienne Millon) + Cleanup `sequence_blank_line` (ocaml-ppx/ocamlformat#1075) (Jules Aguillon) + CI: use a script travis-ci.sh to simplify .travis.yml (ocaml-ppx/ocamlformat#1063) (Guillaume Petiot) + Remove utility functions from `Fmt_ast` (ocaml-ppx/ocamlformat#1059) (Guillaume Petiot) + CI: use opam-2.0.5 in Travis (ocaml-ppx/ocamlformat#1044) (Anton Kochkov) + CI: check the build with OCaml 4.07.1 and 4.08.0 (ocaml-ppx/ocamlformat#1036) (Jules Aguillon) + Use the same sets of options for both branches by default in `test_branch.sh` (ocaml-ppx/ocamlformat#1033) (Guillaume Petiot) + Fix `test_branch.sh` and CI checking of CHANGES.md (ocaml-ppx/ocamlformat#1032, ocaml-ppx/ocamlformat#1034) (Jules Aguillon) + Fix flag of git-worktree in `test_branch.sh` and `bisect.sh` (ocaml-ppx/ocamlformat#1027) (Guillaume Petiot) + Remove the `bisect_ppx` dependency and clean the `Makefile` (ocaml-ppx/ocamlformat#1005) (Jules Aguillon) + Use a `CHANGES.md` log file again (ocaml-ppx/ocamlformat#1023) (Guillaume Petiot) + Support OCaml 4.09.0 (add the odoc.1.4.2 dependency) (ocaml-ppx/ocamlformat#1024) (Guillaume Petiot) + Update labels of issue templates (ocaml-ppx/ocamlformat#1017) (Guillaume Petiot) + Update labels in `CONTRIBUTING.md` (ocaml-ppx/ocamlformat#1007) (Guillaume Petiot) + Allow to ignore invalid options (ocaml-ppx/ocamlformat#984) (hhugo) The `--ignore-invalid-option` flag is added to ignore invalid options in `.ocamlformat` files. + Improve the documentation of `--doc-comments` (ocaml-ppx/ocamlformat#982) (Jules Aguillon) + Remove symbolic links and change naming convention of tests (ocaml-ppx/ocamlformat#980) (Guillaume Petiot) + Change the type of `fmt_code` (ocaml-ppx/ocamlformat#974) (Guillaume Petiot) + Simplify `Makefile` (ocaml-ppx/ocamlformat#973) (hhugo) + Dune should not be flagged as a build dep anymore (ocaml-ppx/ocamlformat#954) (Guillaume Petiot)

`dune` cannot directly generate a list of rules depending on things like file system layout, but there are several workarounds for this. The first one is "ocaml syntax" - a dune file can be written as an OCaml script which builds a large string depending on its execution. The second one is dune file generation: an executable generates a dune.inc (which is stored in the git repository) which is included in the main dune file. An extra diff action makes sure that dune.inc stays up to date. This switches from ocaml syntax to dune file generation, for the following reasons: - ocaml syntax could go away in a future dune version, and does not work well with incremental builds - it makes it possible to use external libraries in the code generation, such as stdio in this example. - it makes debugging easier since it's only a (generated) file to inspect Some things to note: - since the file is generated, it needs to be kept up to date. Concretely, it means that when a test file is created or delete, it will be necessary to run dune promote to update dune.inc. This seems to be rare enough to be workable. - the file is generated so it's visible in git and makes a large positive diffstat in this cmmit. - `max-iters` is also set in .ocamlformat, which changes a test that was setting `--config` which overrides `OCAMLFORMAT`.

Julow reviewed Oct 16, 2019

View reviewed changes

test/passing/gen/gen.ml Show resolved Hide resolved

test/passing/gen/gen.ml Outdated Show resolved Hide resolved

test/passing/gen/gen.ml Outdated Show resolved Hide resolved

gpetiot added the CLA Signed label Oct 17, 2019

gpetiot changed the title ~~Dune file: do not use ocaml syntax~~ Tests: do not use ocaml syntax in the dune file Oct 17, 2019

emillon force-pushed the dune-no-ocaml-syntax branch 2 times, most recently from 242d298 to 9597c49 Compare October 17, 2019 08:59

Julow approved these changes Oct 17, 2019

View reviewed changes

emillon force-pushed the dune-no-ocaml-syntax branch from 9597c49 to d5deb34 Compare October 17, 2019 09:30

Julow merged commit 1955939 into ocaml-ppx:master Oct 17, 2019

emillon deleted the dune-no-ocaml-syntax branch October 18, 2019 16:26

Julow mentioned this pull request Nov 4, 2019

[new release] ocamlformat (0.12) ocaml/opam-repository#15208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: do not use ocaml syntax in the dune file #1082

Tests: do not use ocaml syntax in the dune file #1082

emillon commented Oct 16, 2019

facebook-github-bot commented Oct 16, 2019

Julow left a comment

gpetiot commented Oct 16, 2019

gpetiot commented Oct 17, 2019

Julow left a comment

Julow Oct 17, 2019

emillon Oct 17, 2019

Julow Oct 17, 2019

emillon commented Oct 17, 2019

emillon commented Oct 17, 2019

emillon commented Oct 17, 2019

Tests: do not use ocaml syntax in the dune file #1082

Tests: do not use ocaml syntax in the dune file #1082

Conversation

emillon commented Oct 16, 2019

facebook-github-bot commented Oct 16, 2019

Julow left a comment

Choose a reason for hiding this comment

gpetiot commented Oct 16, 2019

gpetiot commented Oct 17, 2019

Julow left a comment

Choose a reason for hiding this comment

Julow Oct 17, 2019

Choose a reason for hiding this comment

emillon Oct 17, 2019

Choose a reason for hiding this comment

Julow Oct 17, 2019

Choose a reason for hiding this comment

emillon commented Oct 17, 2019

emillon commented Oct 17, 2019

emillon commented Oct 17, 2019