Add counts of deps and rev-deps in the list of packages - #259
Conversation
|
Pushed to staging. Let's see how this looks. |
kit-ty-kate
left a comment
There was a problem hiding this comment.
I'm not immediately convinced by these two fields. While the number of direct reverse dependencies can be useful (to gauge usage), the number of direct dependencies (as opposed to the number of overall dependencies, as-in recursive) is of debatable utility i feel.
While more costly to generate i feel like it would be more useful to give the number of overall dependencies for each packages. This way it would be easy to see at a glance how big the dependency chain/attack surface of a package is. As-is in your PR for example, packages such as async show only 11 dependencies whereas in reality it requires 114 packages to be installed
| <th>Dependencies</th> | ||
| <th>Reverse dependencies</th> |
There was a problem hiding this comment.
The full size names (extending the size of the column) feel a bit cluttery on the screen, maybe adding shorter names for the columns would help, like Deps and RDeps
There was a problem hiding this comment.
Renamed to Rev-deps and Dep-cone (but I can change to, say, RDeps and DepCone)
| (* A graph where nodes are packages and there is an edge a->b if package a depends on package b. *) | ||
| module PkgGraph = struct | ||
| type t = package_set package_map | ||
| module V = OpamPackage | ||
| let iter_vertex f deps = OpamPackage.Map.iter (fun pkg _ -> f pkg) deps | ||
| let iter_succ f deps pkg = | ||
| match OpamPackage.Map.find_opt pkg deps with | ||
| | None -> () | ||
| | Some pkgs -> OpamPackage.Set.iter f pkgs | ||
| end | ||
|
|
||
| module PkgTopo = Graph.Topological.Make (PkgGraph) | ||
|
|
||
| let dependency_cone_sizes depends = | ||
| (* packages in reverse topological order *) | ||
| let pkgs = PkgTopo.fold (fun pkg acc -> pkg :: acc) depends [] in |
There was a problem hiding this comment.
why do you need a graph here? Isn't getting the cardinal of max disjunction sufficient?
There was a problem hiding this comment.
Not sure what you mean by "max disjuction". If we computed the dependency cone for each package in the switch independently (using, for ex, OpamSwitchState.dependencies) there'd be too much recomputation.
Argument depends here is already the representation of a directed graph (a map from pkg to list of pkgs, ie, an adjacency list representation). Using this, we can reuse OcamlGraph for traversals to avoid recomputation. Or we can write a custom traversal (but it's still a graph traversal)
There was a problem hiding this comment.
ah, yeah i was originally thinking of a brute force approach + hashtable to avoid recomputation, but it is 25x slower if you take the whole universe. The topological graph approach is clever, neat approximation.
| let cone_depends st = | ||
| OpamPackage.Map.fold (fun pkg opam -> | ||
| let { build; post; test; doc; dev_setup; depopts; _} : OpamListCommand.dependency_toggles = | ||
| OpamListCommand.default_dependency_toggles in | ||
| let deps = | ||
| OpamFormula.packages st.packages @@ | ||
| OpamPackageVar.all_depends ~build ~post ~test | ||
| ~doc ~dev_setup ~depopts ~filter_default:true |
There was a problem hiding this comment.
Something like this would be closer to reality. Counting post dependencies will break the graph half the time anyway and inflates the number of dependencies of the ocaml package artificially. Same for disjuctions
| let cone_depends st = | |
| OpamPackage.Map.fold (fun pkg opam -> | |
| let { build; post; test; doc; dev_setup; depopts; _} : OpamListCommand.dependency_toggles = | |
| OpamListCommand.default_dependency_toggles in | |
| let deps = | |
| OpamFormula.packages st.packages @@ | |
| OpamPackageVar.all_depends ~build ~post ~test | |
| ~doc ~dev_setup ~depopts ~filter_default:true | |
| let rec remove_disjunction = function | |
| | (OpamFormula.Empty as x) | (Atom _ as x) -> x | |
| | Block x -> remove_disjunction x | |
| | And (a, b) -> And (remove_disjunction a, remove_disjunction b) | |
| | Or (a, _) -> remove_disjunction a | |
| let cone_depends st = | |
| OpamPackage.Map.fold (fun pkg opam -> | |
| let deps = | |
| OpamFormula.packages st.OpamStateTypes.packages @@ | |
| remove_disjunction @@ | |
| OpamPackageVar.all_depends ~build:true ~post:false ~test:false | |
| ~doc:false ~dev_setup:false ~depopts:false ~filter_default:true |
| (* A graph where nodes are packages and there is an edge a->b if package a depends on package b. *) | ||
| module PkgGraph = struct | ||
| type t = package_set package_map | ||
| module V = OpamPackage | ||
| let iter_vertex f deps = OpamPackage.Map.iter (fun pkg _ -> f pkg) deps | ||
| let iter_succ f deps pkg = | ||
| match OpamPackage.Map.find_opt pkg deps with | ||
| | None -> () | ||
| | Some pkgs -> OpamPackage.Set.iter f pkgs | ||
| end | ||
|
|
||
| module PkgTopo = Graph.Topological.Make (PkgGraph) | ||
|
|
||
| let dependency_cone_sizes depends = | ||
| (* packages in reverse topological order *) | ||
| let pkgs = PkgTopo.fold (fun pkg acc -> pkg :: acc) depends [] in |
There was a problem hiding this comment.
ah, yeah i was originally thinking of a brute force approach + hashtable to avoid recomputation, but it is 25x slower if you take the whole universe. The topological graph approach is clever, neat approximation.
| OpamPackage.Map.add pkg cone acc) | ||
| OpamPackage.Map.empty pkgs | ||
| in | ||
| OpamPackage.Map.map OpamPackage.Name.Set.cardinal cones |
There was a problem hiding this comment.
To avoid having every almost package start at 10 dependencies, we could cheat a little and only show the number of dependencies starting from the ocaml package. What do you think?
| OpamPackage.Map.map OpamPackage.Name.Set.cardinal cones | |
| let ocaml_cardinal = | |
| OpamPackage.Name.Set.cardinal | |
| (OpamPackage.Map.find (OpamPackage.of_string "ocaml.5.5.0") cones) | |
| in | |
| OpamPackage.Map.map (fun set -> | |
| Int.max 0 (OpamPackage.Name.Set.cardinal set - ocaml_cardinal)) | |
| cones |
There was a problem hiding this comment.
That's a good suggestion. But we should not substract ocaml_cardinal unconditionally from every cone, only from those with pacakges which included ocaml explicitly as a dependency. (If we did, many packages would end up with a cone of size < 0). Instead of updating cardinalities I've removed the ocaml dependency earlier in the calculation.
An interesting example. Inspecting the dependency cone of alcobar (opam list --rec --required-by "alcobar.0.3.1") we see jbuilder. I asked an LLM for an explanation, and I got this back:
alcobar depends on alcotest with no version constraint, so that resolves to every alcotest in the repository — 1.0.1 through 1.9.1. alcotest.1.0.1 is a 2020 release, and it wants re {>= "1.7.2"}, which again is open-ended upward but reaches down to re.1.7.2 — a 2018 release, from back when re was built with jbuilder, dune's predecessor.
It further suggests:
If you wanted the number closer to "what would actually get installed", the lever is picking a single version per dependency name — the latest satisfying one, say — rather than all of them. That's a change in cone_depends, where OpamFormula.packages currently returns the full satisfying set.
Not planning to do anything about it in this PR, since opam list behaves the same way.
done in commit bde1fb7 |
|
I don't have too much time for this particular review this week but i've at least pushed it to |
Adds columns
DependenciesandReverse dependencies(counts) to the list of packages.Adds column
Dependency cone(count), an over-approximation of counting the output ofopam list --rec --required-by, because it includes packages of every platform (os, arch, ...).Note: the dependency cone is calculated by first doing a topological sort of dependencies with OcamlGraph. This wouldn't produce an accurate cone if the graph had cycles. As far as I can tell, this shouldn't happen, because we skip dependencies marked as
post(Comment aboutpostin the opam manual: