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

Diffing and promoting multiple files #3567

Closed
jfthuong opened this issue Jun 19, 2020 · 10 comments · Fixed by #6933
Closed

Diffing and promoting multiple files #3567

jfthuong opened this issue Jun 19, 2020 · 10 comments · Fixed by #6933
Milestone

Comments

@jfthuong
Copy link

jfthuong commented Jun 19, 2020

Desired Behavior

I'm writing some ("custom") tests run with dune and want naturally to use dune promote to update my expected results (which are stored in external files as my tested libraries are generated files).

As I have several files, I am currently doing the following:

   (no-infer (progn
      (diff expected/lex/actor.lex obtained/lex/actor.lex)
      (diff expected/lex/basic.lex obtained/lex/basic.lex)
      (diff expected/lex/cover.lex obtained/lex/cover.lex)
      (diff expected/lex/empty_osc.lex obtained/lex/empty_osc.lex)
      (diff expected/lex/enum.lex obtained/lex/enum.lex)
      ...
   ))

And this file is updated by a script (to be changed by creating an executable to update it).

One limitation is that if many files differ, we need to promote, run the tests again, promote, run ...
The other issue is that diff with difference will stop the actions (and with-accepted-exit-codes does not support diff ).

It could be nice to have a way to be able to:

  • compare directories (optionally with a "recursive option" like the -r of diff)
  • promote all the differences in a single run (with an individual confirmation for each difference and maybe an option to force promotion, like dune promote --force)

We could also consider a promote alias to customize what dune promote could do.

Examples

We could add either one or both of the following actions:

diffn and diffn?

This would be a cross-over between progn and diff and could be used as such:

   (no-infer (diffn
      (expected/lex/actor.lex obtained/lex/actor.lex)
      (expected/lex/basic.lex obtained/lex/basic.lex)
      (expected/lex/cover.lex obtained/lex/cover.lex)
      (expected/lex/empty_osc.lex obtained/lex/empty_osc.lex)
      (expected/lex/enum.lex obtained/lex/enum.lex)
      ...
   ))

The main advantage would be to allow promotion of all the differences with a single dune promote.

If this action is part of progn, we would continue to the next action only if no difference has been found.

diffd

This action would compare directories but there would be no inference of targets (so no need to have diffd?). We could have something like diffd+ to have recursive differences.

It could be used as such:

  (diffd expected/ obtained/)

This could also allow multiple promotion.

Cheers

@ghost
Copy link

ghost commented Jun 23, 2020

diffn and diffd are interesting ideas. I suspect that for these to be usable we also need @snowleopard proposal for directory targets: #3316

BTW, have you tried using multiple rules with one diff per rule? That should provide exactly the behaviour you want, i.e. dune will display all the diffs without stopping after the first one and you can promote all the corrections at once.

@rgrinberg
Copy link
Member

One limitation is that if many files differ, we need to promote, run the tests again, promote, run ...

Can't this be addressed by writing an individual rule for every diff action?

@jfthuong
Copy link
Author

diffn and diffd are interesting ideas. I suspect that for these to be usable we also need @snowleopard proposal for directory targets: #3316

BTW, have you tried using multiple rules with one diff per rule? That should provide exactly the behaviour you want, i.e. dune will display all the diffs without stopping after the first one and you can promote all the corrections at once.

Indeed. However, before I needed the execution of another command that was generating the files to compare:

(test
 (name test)
 (libraries alcotest unix)
 (deps
    (glob_files inputs/*.*)
  )
 (action
   (progn
    (with-accepted-exit-codes (or 0 1) (run %{test} -e))  ; continue diff, especially if test failed
    (no-infer (progn
      (expected/lex/actor.lex obtained/lex/actor.lex)
      (expected/lex/basic.lex obtained/lex/basic.lex)
      (expected/lex/cover.lex obtained/lex/cover.lex)
    ))
  )
 )
)

The workaround was to:

  1. Create a target with output file generated when running the test
  2. Splitting each diff in a runtest alias like you suggested

Which gives something like:

(executables
 (names test)
 (libraries alcotest unix)
)

(rule
 (target test_run.out)
 (deps
    (glob_files inputs/*.*)
  )
 (action (progn
  (with-accepted-exit-codes (or 0 1) (run %{dep:test.exe} -e))  ; continue diff, especially if test failed
  (write-file %{target} "Test Run")
 ))
)

(rule
 (alias runtest)
 (deps test_run.out)
 (action (no-infer
  (diff expected/lex/actor.lex obtained/lex/actor.lex)
 ))
)

(rule
 (alias runtest)
 (deps test_run.out)
 (action (no-infer
  (diff expected/lex/basic.lex obtained/lex/basic.lex)
 ))
)

(rule
 (alias runtest)
 (deps test_run.out)
 (action (no-infer
  (diff expected/lex/cover.lex obtained/lex/cover.lex)
 ))
)

It's working but lot of duplicate code.

It would be nice to have a way to declare a function or macro (like in Shell, Jinja2 Macro, or Makefile) => maybe I could record an enhancement issue :)

Merci Jeremy!

@ghost
Copy link

ghost commented Jun 29, 2020

Derien Jeff :)

It would be nice to have a way to declare a function or macro (like in Shell, Jinja2 Macro, or Makefile) => maybe I could record an enhancement issue :)

Yes, that would be nice indeed. We have been talking about this for a while. We kind of don't want to design a bad programming language though. We are hopping to be able to use plain OCaml to factorise dune files.

@jfthuong
Copy link
Author

jfthuong commented Jul 1, 2020

You mean like being able to call OCaml functions from dune?

@ghost
Copy link

ghost commented Jul 1, 2020

We are not completely sure of the details yet, but if you use complex logic to produce the dune files, it feels like this logic should be written in OCaml rather than a made-up language we invented.

@Zimmi48
Copy link
Contributor

Zimmi48 commented Aug 3, 2020

We're also interested to have multiple files diffing / promotion for building Coq. Here are more details about our use case:

We've got a tool called doc_grammar whose purpose is to compile all the parsing rules of Coq and sprinkle them throughout our rst manual. Concretely, there are two intermediate files that are committed in the repository as well, fullGrammar and orderedGrammar. The former is auto-generated from the Coq parsing files and the latter must hold the same content as fullGrammar, but edited using some rules defined in another file, and arbitrarily re-ordered. The last step is to detect some special directives in the rst source files to update the following grammar snippets with an extract from orderedGrammar.

Currently, our Dune rules for this are:

(executable
 (name doc_grammar)
 (libraries coq.clib coqpp))

(env (_ (binaries doc_grammar.exe)))

(rule
 (alias check-gram)
 (deps
  (:input
   ; Main grammar
   (glob_files %{project_root}/parsing/*.mlg)
   (glob_files %{project_root}/toplevel/*.mlg)
   (glob_files %{project_root}/vernac/*.mlg)
   ; All plugins except SSReflect and Ltac2 for now (mimicking what is done in Makefile.doc)
   (glob_files %{project_root}/plugins/btauto/*.mlg)
   (glob_files %{project_root}/plugins/cc/*.mlg)
   (glob_files %{project_root}/plugins/derive/*.mlg)
   (glob_files %{project_root}/plugins/extraction/*.mlg)
   (glob_files %{project_root}/plugins/firstorder/*.mlg)
   (glob_files %{project_root}/plugins/funind/*.mlg)
   (glob_files %{project_root}/plugins/ltac/*.mlg)
   (glob_files %{project_root}/plugins/micromega/*.mlg)
   (glob_files %{project_root}/plugins/nsatz/*.mlg)
   (glob_files %{project_root}/plugins/omega/*.mlg)
   (glob_files %{project_root}/plugins/rtauto/*.mlg)
   (glob_files %{project_root}/plugins/setoid_ring/*.mlg)
   (glob_files %{project_root}/plugins/syntax/*.mlg)
   ; Sphinx files
   (glob_files %{project_root}/doc/sphinx/language/*.rst)
   (glob_files %{project_root}/doc/sphinx/proof-engine/*.rst)
   (glob_files %{project_root}/doc/sphinx/user-extensions/*.rst)
   (glob_files %{project_root}/doc/sphinx/practical-tools/*.rst)
   (glob_files %{project_root}/doc/sphinx/addendum/*.rst)
   (glob_files %{project_root}/doc/sphinx/language/core/*.rst)
   (glob_files %{project_root}/doc/sphinx/language/extensions/*.rst)
   (glob_files %{project_root}/doc/sphinx/proofs/writing-proofs/*.rst)
   (glob_files %{project_root}/doc/sphinx/proofs/automatic-tactics/*.rst)
   (glob_files %{project_root}/doc/sphinx/proofs/creating-tactics/*.rst)
   (glob_files %{project_root}/doc/sphinx/using/libraries/*.rst)
   (glob_files %{project_root}/doc/sphinx/using/tools/*.rst))
  common.edit_mlg
  orderedGrammar)
 (action
  (progn
   (chdir %{project_root} (run doc_grammar -check-cmds -no-update %{input}))
   (diff? fullGrammar fullGrammar.new)
   (diff? orderedGrammar orderedGrammar.new))))

First, as you can see, we could do some serious simplification in the list of dependencies if we had recursive globs (#1866). Then, you can notice the two diff? which in practice mean that whenever the two files have to be updated, we need to rerun the tool after promoting the first one. Finally, the rule doesn't take care (yet) of updating the various rst files. If we wanted to do this, we would need to add a Dune file in every directory containing an rst file and repeat the call to the doc_grammar tool many times (we could optimize it for this use case though). For now, the workaround to promote rst files is simply to cp the content of _build/default/doc/sphinx into doc/sphinx, but this means that the support for this process is less good than what we provide with traditional Makefiles.

cc @jfehrle FYI

@ghost
Copy link

ghost commented Aug 3, 2020

Indeed, that doesn't seem ideal. If we had #3668, you could use the "generate and commit" method to avoid having to maintain the boilerplate by hand. Like what we do in doc/dune in the dune repository.

Apart from that, recursive globs would indeed help. Another thing we have been talking in the past is to add the ability for commands to talk to the build system. For instance, it seems quite easy to allow commands to dynamically generate diff? actions. We started working on adding a RPC to Dune, and we will likely rely on it for such things. So in the future you can expect to have a small library to emit diff? actions on the fly.

@jfehrle
Copy link

jfehrle commented Aug 3, 2020

Finally, the rule doesn't take care (yet) of updating the various rst files. If we wanted to do this, we would need to add a Dune file in every directory containing an rst file and repeat the call to the doc_grammar tool many times (we could optimize it for this use case though).

Note that the tool does consistency checks across the .rst files as a whole to identify missing and duplicated items. Perhaps that would have to be a separate step if each .rsts is updated in its own step.

@bn-d
Copy link

bn-d commented Sep 1, 2022

We recently also encountered a case where we need to diff two dirs. If diffn/diffd is implemented, it will greatly shorten our dune file length and reduce duplicated codes. I think diffd can work perfectly with the dir target.

@rgrinberg rgrinberg added this to the 3.8.0 milestone Feb 12, 2023
@rgrinberg rgrinberg linked a pull request Feb 12, 2023 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants