Skip to content

Add opam tree subcommand#5171

Merged
kit-ty-kate merged 11 commits into
ocaml:masterfrom
cannorin:opam-tree
Sep 12, 2022
Merged

Add opam tree subcommand#5171
kit-ty-kate merged 11 commits into
ocaml:masterfrom
cannorin:opam-tree

Conversation

@cannorin

@cannorin cannorin commented Jul 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

screenshot

TODO

  • add .mli
  • ask for feedbacks
  • complete doc and man
  • add tests
  • update master_changes.md file

@cannorin cannorin marked this pull request as draft July 1, 2022 00:39
@rjbou rjbou added the PR: WIP Not for merge at this stage label Jul 1, 2022
rjbou
rjbou reviewed Jul 5, 2022
View reviewed changes

@rjbou rjbou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread src/client/opamCommands.ml Outdated
Comment thread src/client/opamCommands.ml Outdated
Comment thread src/client/opamCommands.ml Outdated
Comment thread src/client/opamCommands.ml Outdated
Comment thread src/client/opamCommands.ml Outdated
Comment thread src/client/opamTreeCommand.ml Outdated
Comment thread src/client/opamTreeCommand.ml Outdated
Comment thread src/client/opamTreeCommand.ml Outdated
Comment thread src/client/opamTreeCommand.ml Outdated
Comment thread src/client/opamTreeCommand.ml Outdated
@cannorin

cannorin commented Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

..., 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 cannorin force-pushed the opam-tree branch 2 times, most recently from bd9a6b8 to b30a667 Compare July 6, 2022 09:03
@cannorin

cannorin commented Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

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

@rjbou

rjbou commented Jul 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

cannorin commented Jul 12, 2022

Copy link
Copy Markdown
Contributor Author

@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

rjbou commented Jul 12, 2022

Copy link
Copy Markdown
Collaborator

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 cannorin marked this pull request as ready for review July 12, 2022 14:00
@cannorin

cannorin commented Jul 12, 2022

Copy link
Copy Markdown
Contributor Author

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.

@kit-ty-kate kit-ty-kate removed the PR: WIP Not for merge at this stage label Jul 12, 2022
@kit-ty-kate kit-ty-kate changed the title [WIP] Add opam tree subcommand Add opam tree subcommand Jul 12, 2022
kit-ty-kate
kit-ty-kate reviewed Jul 12, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.ml Outdated
Comment thread src/core/opamConsole.ml Outdated
Comment thread tests/reftests/tree.test
Comment thread tests/reftests/tree.test Outdated
@rjbou rjbou self-requested a review July 19, 2022 10:20
@smorimoto

smorimoto commented Jul 25, 2022

Copy link
Copy Markdown
Member

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

@smorimoto

smorimoto commented Jul 25, 2022

Copy link
Copy Markdown
Member

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

@cannorin

cannorin commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

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

What do you think about it?

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

kit-ty-kate commented Jul 28, 2022

Copy link
Copy Markdown
Member

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

smorimoto commented Jul 28, 2022

Copy link
Copy Markdown
Member

@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

kit-ty-kate commented Jul 28, 2022

Copy link
Copy Markdown
Member

@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

dra27 commented Jul 28, 2022

Copy link
Copy Markdown
Member

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

smorimoto commented Jul 28, 2022

Copy link
Copy Markdown
Member

On second thought, it surely makes sense.

@cannorin

cannorin commented Jul 29, 2022

Copy link
Copy Markdown
Contributor Author

ok, I'll revert it.

@kit-ty-kate kit-ty-kate force-pushed the opam-tree branch 2 times, most recently from f946b3f to dbb7550 Compare July 29, 2022 11:24
rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.mli
rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamCommands.ml Outdated
rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamCommands.ml Outdated
in
let no_switch =
mk_flag ~cli (cli_from cli2_2) ["no-switch"] ~section:selection_docs
"Do not take into account switch consistency"

@rjbou rjbou Sep 9, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it intelligible?

@dbuenzli dbuenzli Sep 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@rjbou rjbou Sep 9, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

new version

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

@cannorin cannorin Sep 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

installing packages to an empty switch?

rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.mli
rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.ml Outdated
rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.ml Outdated
rjbou
rjbou reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.ml Outdated
kit-ty-kate
kit-ty-kate reviewed Sep 9, 2022
View reviewed changes
Comment thread src/client/opamTreeCommand.mli Outdated
@kit-ty-kate

kit-ty-kate commented Sep 12, 2022

Copy link
Copy Markdown
Member

Thanks a lot!

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

rjbou commented Sep 12, 2022

Copy link
Copy Markdown
Collaborator

Thanks all!

@cannorin

cannorin commented Sep 12, 2022

Copy link
Copy Markdown
Contributor Author

Thanks everyone 🚀 🚀 🚀

@rjbou rjbou added this to the 2.2.0~alpha milestone Sep 12, 2022
@smorimoto smorimoto mentioned this pull request Sep 12, 2022
Merged
@cannorin cannorin deleted the opam-tree branch September 13, 2022 08:44
@smorimoto smorimoto mentioned this pull request Sep 23, 2022
Closed
@rjbou rjbou mentioned this pull request Jul 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

@rjbou rjbou rjbou left review comments

@AltGr AltGr AltGr approved these changes

+1 more reviewer

@dbuenzli dbuenzli dbuenzli left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AREA: ENGINE AREA: UI

Projects

None yet

Milestone

2.2.0~alpha

Development

Successfully merging this pull request may close these issues.

Add opam tree subcommand to display a dependency tree

7 participants

@cannorin @rjbou @smorimoto @kit-ty-kate @dra27 @dbuenzli @AltGr