8000 Add "dune promotion diff" to display changes only by emillon · Pull Request #6160 · ocaml/dune · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add "dune promotion diff" to display changes only #6160

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
Merged
merged 12 commits into from
Nov 8, 2022
Merged

Add "dune promotion diff" to display changes only #6160

merged 12 commits into from
Nov 8, 2022

Conversation

emillon
Copy link
Collaborator
@emillon emillon commented Sep 22, 2022 •
edited
Loading

dune promotion diff displays the same diff that the previous command (such as dune runtest) did.

dune promotion generalizes dune promote. Commands that manipulate the promotion db can be attached to dune promotion. dune promotion run is added as an alias to dune promote.

The command supports listing files - in that case, the diff for just these files is displayed (with a warning when they do not exist, like in the dune promote case).

Closes #5368

@emillon
Copy link
Collaborator Author
emillon commented Sep 22, 2022

In terms of scope, this feature should compose nicely with the rest of diff-manipulating tools. So it's possible to use dune promotion diff | diffstat, etc, rather than implementing that ourselves.

rgrinberg
rgrinberg reviewed Oct 3, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 3, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 3, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 3, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Oct 3, 2022
View reviewed changes
rgrinberg
rgrinberg requested changes Oct 3, 2022
View reviewed changes
Copy link
Member
@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea looks good. I left some comments about the implementation.

@emillon emillon force-pushed the promotion-diff branch 2 times, most recently from 17c4464 to 6ce091b Compare October 3, 2022 15:44
@rgrinberg
Copy link
Member

@emillon is this ready?

@emillon emillon added this to the 3.6.0 milestone Oct 13, 2022
@emillon
Copy link
Collaborator Author
emillon commented Oct 13, 2022

No, that'll go in 3.6. A first step for the refactoring is #6196, which for some reason has different results in CI.

@emillon
Copy link
Collaborator Author
emillon commented Nov 4, 2022

@rgrinberg I rebased this, this is ready for another round of review. There's a bunch of stuff in that PR:

  • promote is moved to promotion run
  • Print_diff is refactored using an internal Job abstraction that allows either exec'ing into the diff program (former behaviour) or capturing the output
  • promotion diff is added

(it's also missing docs and changelog)

If we agree with the general principle I can land these changes in separate PRs.

@emillon emillon requested a review from rgrinberg November 4, 2022 14:11
rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
@rgrinberg
Copy link
Member

The changes sound fine to me in principle. The command promotion run doesn't sound right though.

rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
src/dune_engine/print_diff.mli
@@ -7,3 +7,16 @@ val print :
-> Path.t
-> Path.t
-> _ Fiber.t
Copy link
Member
@rgrinberg rgrinberg Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be Diff.or_raise : t -> _ Fiber.t I suppose.

Or rather, we can have get return (Diff.t, exn) Fiber.t

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (I chose to return User_message.t - this is ignored anyway)

rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
rgrinberg
rgrinberg reviewed Nov 6, 2022
View reviewed changes
src/dune_engine/diff_promotion.mli
@@ -44,3 +44,5 @@ type files_to_promote =
| These of Path.Source.t list * (Path.Source.t -> unit)

val promote_files_registered_in_last_run : files_to_promote -> unit

val display : files_to_promote -> unit Fiber.t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely related to this PR, but we should change the API away from the ugly callback.

@emillon
Copy link
Collaborator Author
emillon commented Nov 7, 2022

Any suggestions to improve promotion run? promotion apply? promotion patch?

@rgrinberg
Copy link
Member

promotion apply seems reasonable

@emillon
Copy link
Collaborator Author
emillon commented Nov 8, 2022

I renamed to apply and made dune promote a command_alias for it.

@Alizter
Copy link
Collaborator
Alizter commented Nov 8, 2022

Won't this also inherit the common flags like enabling and disabling promotion which are probably noops here.

@emillon
Copy link
Collaborator Author
emillon commented Nov 8, 2022

That's a good point. To a large extent this is due to the fact that this behavior is controlled through globals set via Common, so we can't really fix it before dune 4, but I can try to see if it's possible to pass just the minimum.

@rgrinberg
Copy link
Member

I'd vote not to bother with it. Many commands suffer from this problem so it doesn't make sense to add a hack to solve the issue just for one command.

@emillon emillon force-pushed the promotion-diff branch 3 times, most recently from 7311931 to 33ac53e Compare November 8, 2022 15:31
@emillon
Copy link
Collaborator Author
emillon commented Nov 8, 2022

Thanks @rgrinberg - I'll merge once CI passes.

emillon added 12 commits November 8, 2022 16:51
@emillon
Add promotion diff to just display changes
43dfdfd 
`dune promotion diff` displays the same diff that the previous command
(such as `dune runtest`) did.

`dune promotion` generalizes `dune promote`. Commands that manipulate
the promotion db can be attached to `dune promotion`. `dune promotion
run` is added as an alias to `dune promote`.

The command supports listing files - in that case, the diff for just
these files is displayed (with a warning when they do not exist, like in
the `dune promote` case).

Closes ocaml#5368

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Better names
4fcc9bf 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Document (and rename) Return
F438
e0e5868 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Map in parallel
a1ad49e 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Dune promotion apply
a398527 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Make promote a command alias
ebff55f 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Changelog
ed041e6 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Simplify types in Job
17a623b 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Return a result
02146c8 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Mention promotion diff in the manual
6296673 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Add cmdliner docs
cdf0938 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Rename Job
5dfa8e7 
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon merged commit 3eaf83c into ocaml:main Nov 8, 2022
@emillon emillon deleted the promotion-diff branch November 8, 2022 16:41
@emillon emillon mentioned this pull request Nov 14, 2022
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Nov 14, 2022
@emillon
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
f207d3f 
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.6.0)

CHANGES:

- Forbid multiple instances of dune running concurrently in the same workspace.
  (ocaml/dune#6360, fixes ocaml/dune#236, @rgrinberg)

- Allow promoting into source directories specified by `subdir` (ocaml/dune#6404, fixes
  ocaml/dune#3502, @rgrinberg)

- Make dune describe workspace return the correct root path
  (ocaml/dune#6380, fixes ocaml/dune#6379, @esope)

- Introduce a `$ dune ocaml top-module` subcommand to load modules directly
  without sealing them behind the signature. (ocaml/dune#5940, @rgrinberg)

- [ctypes] do not mangle user written names in the ctypes stanza (ocaml/dune#6374, fixes
  ocaml/dune#5561, @rgrinberg)

- Support `CLICOLOR` and `CLICOLOR_FORCE` to enable/disable/force ANSI
  colors. (ocaml/dune#6340, fixes ocaml/dune#6323, @MisterDA).

- Forbid private libraries with `(package ..)` set from depending on private
  libraries that don't belong to a package (ocaml/dune#6385, fixes ocaml/dune#6153, @rgrinberg)

- Allow `Byte_complete` binaries to be installable (ocaml/dune#4873, @AltGr, @rgrinberg)

- Revive `$ dune external-lib-deps` under `$ dune describe external-lib-deps`.
  (ocaml/dune#6045, @moyodiallo)

- Fix running inline tests in bytecode mode (ocaml/dune#5622, fixes ocaml/dune#5515, @dariusf)

- [ctypes] always re-run `pkg-config` because we aren't tracking its external
  dependencies (ocaml/dune#6052, @rgrinberg)

- [ctypes] remove dependency on configurator in the generated rules (ocaml/dune#6052,
  @rgrinberg)

- Build progress status now shows number of failed jobs (ocaml/dune#6242, @Alizter)

- Allow absolute build directories to find public executables. For example,
  those specified with `(deps %{bin:...})` (ocaml/dune#6326, @anmonteiro)

- Create a fake socket file `_build/.rpc/dune` on windows to allow rpc clients
  to connect using the build directory. (ocaml/dune#6329, @rgrinberg)

- Prevent crash if absolute paths are used in the install stanza and in
  recursive globs. These cases now result in a user error. (ocaml/dune#6331, @gridbugs)

- Add `(glob_files <glob>)` and `(glob_files_rec <glob>)` terms to the `files`
  field of the `install` stanza (ocaml/dune#6250, closes ocaml/dune#6018, @gridbugs)

- Allow `:standard` in the `(modules)` field of the `coq.pp` stanza (ocaml/dune#6229,
  fixes ocaml/dune#2414, @Alizter)

- Fix passing of flags to dune coq top (ocaml/dune#6369, fixes ocaml/dune#6366, @Alizter)

- Extend the promotion CLI to a `dune promotion` group: `dune promote` is moved
  to `dune promotion apply` (the former still works) and the new `dune promotion
  diff` command can be used to just display the promotion without applying it.
  (ocaml/dune#6160, fixes ocaml/dune#5368, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@christinerose christinerose christinerose approved these changes

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

Successfully merging this pull request may close these issues.

dune promote queries
4 participants
@emillon @rgrinberg @Alizter @christinerose
0