estimate output improvements

[rhansen]

The significant changes in this PR:

• Disable colorized output on non-terminals and avoid hard-coded colorization escape codes by using a terminal color module.
• Don't re-render an already rendered dependency subtree. This eliminates some unnecessary noise, and it makes it possible to render dependency cycles (e.g., golang.org/x/net <-> golang.org/x/crypto).
• List the Debian package name and version when printing "in NEW" lines.

There are also some related code cleanups.

(please do not squash the commits in this PR)

[n-peugnet]

I didn't test this yet, but I am mostly OK with these changes, just a few comments:

• Disable colorized output on non-terminals and avoid hard-coded colorization escape codes by using a terminal color module.

We usually try to avoid adding too much dependencies to dh-make-golang, especially to make it easier to backport. I didn't check these specific dependencies, they might be alright (stable and already in Debian), but if it is easy to write without adding dependencies it would be appreciated.

• Don't re-render an already rendered dependency subtree. This eliminates some unnecessary noise, and it makes it possible to render dependency cycles (e.g., golang.org/x/net <-> golang.org/x/crypto).

Does this mean the repeated dependencies (with a number in parenthesis) are not displayed any more? I did add this on purpose, see the reasoning here: #241

• List the Debian package name and version when printing "in NEW" lines.

Seems nice, I originally decided to simply put an hyperlink to reduce the clutter, but this might also look good.
BTW, the link URL might be outdated now that the DFSG-team has its own dashboard.

There are also some related code cleanups.

These also look nice

[ottok]

Overall looks good! We should however not merge this until the new dependency is packaged, or alternatively move that commit into a separate PR.

[rhansen]

• Disable colorized output on non-terminals and avoid hard-coded colorization escape codes by using a terminal color module.

We usually try to avoid adding too much dependencies to dh-make-golang, especially to make it easier to backport. I didn't check these specific dependencies, they might be alright (stable and already in Debian), but if it is easy to write without adding dependencies it would be appreciated.

I removed that dependency. I will open a separate PR to use github.com/fatih/color (packaged as golang-github-fatih-color-dev) once fatih/color#255 is merged and the Debian package is updated to include it.

• Don't re-render an already rendered dependency subtree. This eliminates some unnecessary noise, and it makes it possible to render dependency cycles (e.g., golang.org/x/net <-> golang.org/x/crypto).

Does this mean the repeated dependencies (with a number in parenthesis) are not displayed any more? I did add this on purpose, see the reasoning here: #241

The number is still there, but it now counts something different: Instead of showing the number of paths from the root module (the root node of the dependency graph) to the dependency it now counts the number of edges pointing to the dependency (the number of times the dependency appears as a requirement in a go.mod). I think this is a more useful metric anyway. I updated the commit message to explain this.

• List the Debian package name and version when printing "in NEW" lines.

Seems nice, I originally decided to simply put an hyperlink to reduce the clutter, but this might also look good. BTW, the link URL might be outdated now that the DFSG-team has its own dashboard.

I changed the URL to the DFSG team dashboard.

Thank you both for reviewing!

[n-peugnet]

Does this mean the repeated dependencies (with a number in parenthesis) are not displayed any more? I did add this on purpose, see the reasoning here: #241

The number is still there, but it now counts something different: Instead of showing the number of paths from the root module (the root node of the dependency graph) to the dependency it now counts the number of edges pointing to the dependency (the number of times the dependency appears as a requirement in a go.mod). I think this is a more useful metric anyway. I updated the commit message to explain this.

I understand the reasoning, but there is an issue with this approach, it can miss some dependencies. Consider for example this estimate command: dh-make-golang estimate -git_revision v1.8.4 github.com/livekit/livekit-server

With your branch I get:

Whereas on master I get:

Note the "buf.build" dependencies especially that get cut out in your branch. This is because of Module graph pruning, which will remove a dependency from the graph if is is not directly used by the code that requires it.
This means it is possible that some instances of the missing modules produce a tree with more dependencies than the others, and this instance is not necessarily the first encountered.

I know that this is an "estimate" command anyway, but I for one would prefer that it show more redundant lines rather than make the work appear simpler than what it really is.

[rhansen]

Note the "buf.build" dependencies especially that get cut out in your branch.

That's due to a bug in graph construction done by estimate. Note that github.com/livekit/protocol was visited earlier, but it's not the same node in the graph as the github.com/livekit/protocol node that is visited later. If it was the same node, the dependencies of github.com/livekit/protocol would have been printed the first time it was visited.

I would fix that bug (and others I've noticed) before merging this PR, but there's a chicken-and-egg problem: if I fix the graph construction bugs, dependency cycles become common (e.g., golang.org/x/net <-> golang.org/x/crypto). Without this PR, these dependency cycles cause estimate to go into an infinite loop and eventually OOM.

(BTW, I recently wrote gomoddepgraph to address the graph construction bugs in estimate, and would like to eventually switch to it after getting consensus on the mailing list.)

[n-peugnet]

That's due to a bug in graph construction done by estimate. Note that github.com/livekit/protocol was visited earlier, but it's not the same node in the graph as the github.com/livekit/protocol node that is visited later. If it was the same node, the dependencies of github.com/livekit/protocol would have been printed the first time it was visited.

So, do you think it does not come from module graph pruning? I assumed it was because of this that multiple occurrences of the same modules could have a different list of dependencies.

I would fix that bug (and others I've noticed) before merging this PR, but there's a chicken-and-egg problem: if I fix the graph construction bugs, dependency cycles become common (e.g., golang.org/x/net <-> golang.org/x/crypto). Without this PR, these dependency cycles cause estimate to go into an infinite loop and eventually OOM.

What do you think about keeping only the refactoring and color related parts in this PR, and do another one that completely replaces the dependency graph part, and fixes the output accordingly?

(BTW, I recently wrote gomoddepgraph to address the graph construction bugs in estimate, and would like to eventually switch to it after getting consensus on the mailing list.)

I did read most of this package's documentation, it seems really nice! Do you plan on merging it inside dh-make-golang or keep it as a separate module?
Your plan would be to switch to the RequirementsComplete() function, parsing the go.mod files ourselves instead of relying on go mod graph right? This seems like a good idea and a more robust solution than what I quickly made to fix the estimate command when it was completely broken.