opam icon indicating copy to clipboard operation
opam copied to clipboard

Add `opam tree` subcommand

Open cannorin opened this issue 2 years ago • 17 comments

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

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

cannorin avatar Jul 01 '22 00:07 cannorin

..., 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 avatar Jul 06 '22 07:07 cannorin

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

cannorin avatar Jul 06 '22 09:07 cannorin

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.

rjbou avatar Jul 06 '22 15:07 rjbou

@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?

cannorin avatar Jul 12 '22 13:07 cannorin

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.

rjbou avatar Jul 12 '22 13:07 rjbou

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.

cannorin avatar Jul 12 '22 14:07 cannorin

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

smorimoto avatar Jul 25 '22 16:07 smorimoto

I'm working on more inclusive Opam dependency support on GitHub, which requires that: https://github.com/ocaml/setup-ocaml/pull/555

smorimoto avatar Jul 25 '22 16:07 smorimoto

@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 avatar Jul 28 '22 02:07 cannorin

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.

kit-ty-kate avatar Jul 28 '22 11:07 kit-ty-kate

@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.

smorimoto avatar Jul 28 '22 11:07 smorimoto

@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

kit-ty-kate avatar Jul 28 '22 11:07 kit-ty-kate

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.

dra27 avatar Jul 28 '22 11:07 dra27

On second thought, it surely makes sense.

smorimoto avatar Jul 28 '22 12:07 smorimoto

ok, I'll revert it.

cannorin avatar Jul 29 '22 08:07 cannorin

@rjbou @kit-ty-kate I've updated master_changes.md to make it ready to merge.

cannorin avatar Aug 02 '22 10:08 cannorin

Thanks. @rjbou wants to review it first too before merging.

kit-ty-kate avatar Aug 02 '22 11:08 kit-ty-kate

@rjbou Thank you so much! All of your changes look good to me, so please force-push it.

cannorin avatar Aug 12 '22 01:08 cannorin

Requires https://github.com/ocaml/opam/pull/5269 to be merged first to be able to fix CI on Windows (once the CI issues have been fixed too)

kit-ty-kate avatar Sep 01 '22 16:09 kit-ty-kate

We found the CI issue. We just need to merge https://github.com/ocaml/opam/pull/5281, rebase on master, and it should be good to go.

kit-ty-kate avatar Sep 08 '22 16:09 kit-ty-kate

Now that CI itself is fixed, looks like there is a genuine issue with the tests of the --no-switch option on Windows:

 ### opam tree j --no-switch
 The following actions are simulated:
-=== install 1 package
+=== install 2 packages
+  - install a 1 [required by j]
   - install j 1
 
 j.1
+'-- a.1 (os = win32)

kit-ty-kate avatar Sep 09 '22 13:09 kit-ty-kate

oupsy

rjbou avatar Sep 09 '22 13:09 rjbou

updated

rjbou avatar Sep 09 '22 13:09 rjbou

Thanks a lot!

kit-ty-kate avatar Sep 12 '22 14:09 kit-ty-kate

Thanks all!

rjbou avatar Sep 12 '22 14:09 rjbou

Thanks everyone 🚀 🚀 🚀

cannorin avatar Sep 12 '22 14:09 cannorin