Add `opam tree` subcommand #5171

cannorin · 2022-07-01T00:38:50Z

Closes #3775 .

This PR adds a subcommand opam tree which displays a dependency tree as a Unicode art or ASCII art.

It can also display a reverse-dependency tree (through opam tree --rev-deps option or opam why alias), which can be useful to examine how dependency versions get constrained.

TODO

rjbou

Thank you for feature & PR!
First part of the review, most comments are about opam lib usage, organisation, etc. Default behaviour code is ok, I'll review other functions later.
Regarding the feature:

As mentioned in one of the comments, it handles only installed packages. It needs to be checked/specified or extended, depending on what we need
We need to add handling of test/doc/build dependencies, ftm they are dropped
Should this feature be at toplevel or a subcommand of opam list? In that case how can it be combined? Module itself is quite long also too.

I'll also add a test, to be populated.

src/client/opamCommands.ml

src/client/opamTreeCommand.ml

cannorin · 2022-07-06T07:16:18Z

..., it handles only installed packages. It needs to be checked/specified or extended, depending on what we need

All of cargo tree, npm ls, and yarn why, which are mentioned in #3775 and inspired the current implementation, only take account of installed packages. So I guess it is ok to support only the installed packages.

We need to add handling of test/doc/build dependencies, ftm they are dropped

Yes 🚀

Should test/doc/build deps be distinguishable in the tree e.g.

foo 1.0.0
|--- bar 1.0.0 (>= 1.0 & with-test)
'--- ...

? It would be a bit complicated to do this since test and doc are processed in OpamSwitchState.universe and the information seems to be lost.

Should this feature be at toplevel or a subcommand of opam list? In that case how can it be combined? Module itself is quite long also too.

Personally I'm against opam list --tree: it would be confusing if opam list --tree took a completely different set of options than in opam list.

Also, man opam list would become too noisy because something like (used with --tree)/(can't be used with --tree) would need to appear in every command option.

cannorin · 2022-07-06T09:27:12Z

It seems I have to either get rid of List.fold_left_map or implement it on OpamStd.

rjbou · 2022-07-06T15:26:31Z

Good for me (and more consistent about the command itself, its difference with list) to keep only installed packages.
About dependencies filters, they can be retrieved unfiltered via st.opams: OpamFile.OPAM.depends (OpamSwitchState.opam st pkg), and construct universe with those variables. I began a canevas for that. I've added doc/test/tools, post & dev (copy paste from opam list). Maybe we should also add build, and i'm not sure about dev if it is really needed?

I've also added a test in tests/reftests/tree.test, you can find the test files syntax in tests/reftests/run.ml top comment. You can run it with make reftest-tree.

cannorin · 2022-07-12T13:49:14Z

@rjbou I'm not very sure how to add a new subcommand to master_changes.md.

## Global CLI
  * ◈ Add `tree` subcommand to ...

or

## ◈ Tree
  * Add `tree` subcommand to ...

or something else?

rjbou · 2022-07-12T13:53:20Z

The first one: an entry for tree, the other for why. We'll need also need to add API changes (addition of modules / changes of signtures, etc. but better once the PR is no more wip to avoid updating it each time.

cannorin · 2022-07-12T14:01:52Z

I think this is now ready for review 🎉

I accidentally edited the API changes in master_changes.md too, so I'll edit it once more after all the future reviews are resolved.

src/client/opamTreeCommand.ml

src/core/opamConsole.ml

tests/reftests/tree.test

smorimoto · 2022-07-25T16:16:20Z

It would be nice if we could also support JSON output for CI use.

smorimoto · 2022-07-25T16:17:54Z

I'm working on more inclusive Opam dependency support on GitHub, which requires that: ocaml/setup-ocaml#555

cannorin · 2022-07-28T02:38:53Z

@smorimoto I've just added a basic support of JSON output. It would look like this:
example.txt

What do you think about it?

kit-ty-kate · 2022-07-28T11:11:24Z

Could the JSON thing be its own PR instead? This is going to make the review process even more long and there would be more things to argue on.

smorimoto · 2022-07-28T11:21:56Z

@cannorin That looks good to me.
@kit-ty-kate Um, is that really true? What we need to do is just review one small commit, and the rest is just the opam development team's intention. If the code and intent are ok, it won't take much time to get there.

kit-ty-kate · 2022-07-28T11:35:30Z

@smorimoto this is not "one small commit", a json output would be used by programs outside of opam so the format is extremely important to get right as a change in the format would break ever users of it, and that requires extra time

dra27 · 2022-07-28T11:53:10Z

Agreed - please can this PR not acquire new features. PRs against your own fork are a better way to start previewing the next stage while this part gets reviewed.

smorimoto · 2022-07-28T12:50:53Z

On second thought, it surely makes sense.

cannorin · 2022-07-29T08:42:05Z

ok, I'll revert it.

rjbou · 2022-09-09T15:12:48Z

src/client/opamCommands.ml

+  in
+  let no_switch =
+    mk_flag ~cli (cli_from cli2_2) ["no-switch"] ~section:selection_docs
+      "Do not take into account switch consistency"


Is it intelligible?

As far as I'm concerned, no :-)

new version

"Ignore active switch and simulate installing packages from an empty \ switch to draw the forest"

installing packages to an empty switch?

src/client/opamTreeCommand.mli

src/client/opamTreeCommand.ml

src/client/opamTreeCommand.mli

kit-ty-kate · 2022-09-12T14:41:57Z

Thanks a lot!

rjbou · 2022-09-12T14:50:18Z

Thanks all!

cannorin · 2022-09-12T14:51:09Z

Thanks everyone 🚀 🚀 🚀

cannorin marked this pull request as draft July 1, 2022 00:39

rjbou added the PR: WIP Not for merge at this stage label Jul 1, 2022

rjbou reviewed Jul 5, 2022

View reviewed changes

cannorin force-pushed the opam-tree branch 2 times, most recently from bd9a6b8 to b30a667 Compare July 6, 2022 09:03

cannorin marked this pull request as ready for review July 12, 2022 14:00

kit-ty-kate removed the PR: WIP Not for merge at this stage label Jul 12, 2022

kit-ty-kate changed the title ~~[WIP] Add opam tree subcommand~~ Add opam tree subcommand Jul 12, 2022

kit-ty-kate added AREA: UI AREA: ENGINE labels Jul 12, 2022

kit-ty-kate reviewed Jul 12, 2022

View reviewed changes

src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved

src/core/opamConsole.ml Outdated Show resolved Hide resolved

tests/reftests/tree.test Show resolved Hide resolved

tests/reftests/tree.test Outdated Show resolved Hide resolved

rjbou self-requested a review July 19, 2022 10:20

cannorin requested a review from kit-ty-kate July 28, 2022 02:40

kit-ty-kate force-pushed the opam-tree branch 2 times, most recently from f946b3f to dbb7550 Compare July 29, 2022 11:24

rjbou reviewed Sep 9, 2022

View reviewed changes

src/client/opamTreeCommand.mli Show resolved Hide resolved

rjbou reviewed Sep 9, 2022

View reviewed changes

src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved

rjbou reviewed Sep 9, 2022

View reviewed changes

src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved

rjbou reviewed Sep 9, 2022

View reviewed changes

src/client/opamTreeCommand.ml Outdated Show resolved Hide resolved

kit-ty-kate reviewed Sep 9, 2022

View reviewed changes

src/client/opamTreeCommand.mli Outdated Show resolved Hide resolved

cannorin added 11 commits September 9, 2022 19:29

compat: add Int.equal

c2ad920

std.list: add find_map_opt and fold_left_map

226756b

formula: add formula_to_cnf and formula_to_cnf

ce78ed1

filter: add customisation function to to_string

a24f0ef

types: add condition type, filter of filtered formula

1f51378

console: add tree printing module

fde4a3a

solution: add dry_run function to simulate a state from a solution

fafdf10

tree: implement tree command

29ac61b

add opam tree command

8ea9c3b

reftests: add tree test

30754fc

update changes

eece100

rjbou force-pushed the opam-tree branch from af6a110 to eece100 Compare September 9, 2022 17:29

kit-ty-kate merged commit 6eeffc7 into ocaml:master Sep 12, 2022

rjbou added this to the 2.2.0~alpha milestone Sep 12, 2022

smorimoto mentioned this pull request Sep 12, 2022

Opam Dependency Submission ocaml/setup-ocaml#555

Merged

cannorin deleted the opam-tree branch September 13, 2022 08:44

smorimoto mentioned this pull request Sep 23, 2022

opam tree JSON output for Opam Dependency Submission #5298

Closed

rjbou removed the STATE: READY TO MERGE label Oct 14, 2022

rjbou mentioned this pull request Jul 3, 2023

Add support for opam tree <dir> #5287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `opam tree` subcommand #5171

Add `opam tree` subcommand #5171

cannorin commented Jul 1, 2022 •

edited

Loading

rjbou left a comment

cannorin commented Jul 6, 2022

cannorin commented Jul 6, 2022

rjbou commented Jul 6, 2022 •

edited

Loading

cannorin commented Jul 12, 2022

rjbou commented Jul 12, 2022

cannorin commented Jul 12, 2022

smorimoto commented Jul 25, 2022

smorimoto commented Jul 25, 2022

cannorin commented Jul 28, 2022

kit-ty-kate commented Jul 28, 2022

smorimoto commented Jul 28, 2022

kit-ty-kate commented Jul 28, 2022

dra27 commented Jul 28, 2022

smorimoto commented Jul 28, 2022

cannorin commented Jul 29, 2022

rjbou Sep 9, 2022

dbuenzli Sep 9, 2022

rjbou Sep 9, 2022

cannorin Sep 10, 2022

kit-ty-kate commented Sep 12, 2022

rjbou commented Sep 12, 2022

cannorin commented Sep 12, 2022

Add opam tree subcommand #5171

Add opam tree subcommand #5171

Conversation

cannorin commented Jul 1, 2022 • edited Loading

TODO

rjbou left a comment

Choose a reason for hiding this comment

cannorin commented Jul 6, 2022

cannorin commented Jul 6, 2022

rjbou commented Jul 6, 2022 • edited Loading

cannorin commented Jul 12, 2022

rjbou commented Jul 12, 2022

cannorin commented Jul 12, 2022

smorimoto commented Jul 25, 2022

smorimoto commented Jul 25, 2022

cannorin commented Jul 28, 2022

kit-ty-kate commented Jul 28, 2022

smorimoto commented Jul 28, 2022

kit-ty-kate commented Jul 28, 2022

dra27 commented Jul 28, 2022

smorimoto commented Jul 28, 2022

cannorin commented Jul 29, 2022

rjbou Sep 9, 2022

Choose a reason for hiding this comment

dbuenzli Sep 9, 2022

Choose a reason for hiding this comment

rjbou Sep 9, 2022

Choose a reason for hiding this comment

cannorin Sep 10, 2022

Choose a reason for hiding this comment

kit-ty-kate commented Sep 12, 2022

rjbou commented Sep 12, 2022

cannorin commented Sep 12, 2022

Add `opam tree` subcommand #5171

Add `opam tree` subcommand #5171

cannorin commented Jul 1, 2022 •

edited

Loading

rjbou commented Jul 6, 2022 •

edited

Loading