Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dh-make-golang estimate #240

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Fix dh-make-golang estimate #240

merged 6 commits into from
Jan 14, 2025

Conversation

n-peugnet
Copy link
Contributor

@n-peugnet n-peugnet commented Jan 5, 2025

As support for the gopath mode of "go get" has been removed in Go 1.22, we cannot use the GO111MODULE=off trick any more:

go get is no longer supported outside of a module in the legacy GOPATH mode (that is, with GO111MODULE=off).

To fix the estimate command, we now download the sources of the repo manually then run "go get" in the repo dir to make use of the information contained in the go.mod file and download the dependencies.

For go packages that are not modules yet, we init ourselves a go module using "go mod init".

Closes: #159
Closes: #231
Closes: #89
Supersedes: #137


The third commit introduces a new method for retrieving the dependencies based on go mod graph. From my (limited) testing this is faster and more robust, but it is still a little bit imprecise. See the commit message for more information.


The fourth commit adds checks for other major versions already packaged in Debian. It is a bit imprecise because the XS-Go-Import-Path: metadata in debian/control is often outdated and does not reflect the current import path. But IMO it is better to show it than to hide it, as upgrading a package to a new major version is not always easy/possible.

For example with the same example as in the previous commit, we get this kind of output:

2025/01/06 16:22:07 github.com/charmbracelet/x/conpty is packaged as github.com/charmbracelet/x in Debian
2025/01/06 16:22:07 github.com/charmbracelet/x/termios is packaged as github.com/charmbracelet/x in Debian
2025/01/06 16:22:07 github.com/charmbracelet/x/errors is packaged as github.com/charmbracelet/x in Debian
2025/01/06 16:22:07 github.com/charmbracelet/x/ansi is packaged as github.com/charmbracelet/x in Debian
2025/01/06 16:22:07 github.com/charmbracelet/x/exp/golden is packaged as github.com/charmbracelet/x in Debian
2025/01/06 16:22:07 github.com/charmbracelet/x/term is packaged as github.com/charmbracelet/x in Debian
2025/01/06 16:22:07 Bringing github.com/charmbracelet/vhs to Debian requires packaging the following Go packages:
github.com/charmbracelet/vhs
  github.com/caarlos0/env/v11	(github.com/caarlos0/env in Debian)
  github.com/charmbracelet/ssh
  github.com/erikgeiser/coninput
  github.com/go-rod/rod
    github.com/ysmood/fetchup
      github.com/ysmood/got
      github.com/ysmood/gop
    github.com/ysmood/goob
      github.com/ysmood/gotrace
    github.com/ysmood/gson
    github.com/ysmood/leakless
  github.com/mattn/go-localereader

There are still some issues with repos that contain multiple modules (go workspaces) and repos that do not have their go module at the root. But this can be enhanced later (or maybe in this PR if I have the time).
Just did it in the fifth commit.


As a last proposition, but this is more a matter of taste, I pushed in a separate branch the commit n-peugnet@d156df1 (best viewed which whitespaces disabled). It shows duplicated dependencies but dimmed, the commit message explains why. Here is a before and after this commit:

2025-01-06-232316_497x197_scrot

After n-peugnet@d156df1:

2025-01-06-231804_497x392_scrot

@n-peugnet n-peugnet force-pushed the fix-estimate branch 3 times, most recently from f6ec11b to 999efa4 Compare January 5, 2025 23:19
As support for the gopath mode of "go get" has been removed in latest
versions of Go, we cannot use the GO111MODULE=off trick any more.
To fix the estimate command, we now download the sources of the repo
manually then run "go get" in the repo dir to make use of the
information contained in the go.mod file and download the dependencies.

For go packages that are not modules yet, we init ourselves a go module
using "go mod init".
Previously, some files could not be removed because of the directories
permissions, which made os.RemoveAll() fail silently and left a lot of
garbage files in /tmp.

This commit adds a new forceRemoveAll() function that removes all files
and directories more reliably, in a new fs_utils.go file for filesystem
utilities. isFile() is also moved to this new file.

Also log errors when forceRemoveAll fails to detect these issues sooner
in the future.
@n-peugnet
Copy link
Contributor Author

This PR is now ready for review.

The technique previously used for retrieving the module graph was costly
and britle, with many errors returned from importgraph.Build for unknown
reasons.

As go modules now provide a simple API to fetch the import graph of a
packages we can now use this more reliable interface instead. It is
faster and delivers more accurate results. For example:

Before

$ go run . estimate github.com/charmbracelet/vhs 2> /dev/null
github.com/charmbracelet/vhs
  github.com/go-rod/rod
  github.com/charmbracelet/ssh

$ go run . estimate github.com/go-rod/rod 2> /dev/null
github.com/go-rod/rod
  github.com/ysmood/goob
  github.com/ysmood/gson
  github.com/ysmood/got
  github.com/ysmood/gotrace
      github.com/ysmood/fetchup
      github.com/ysmood/leakless
  github.com/gobwas/ws

After

$ go run . estimate github.com/charmbracelet/vhs 2> /dev/null
github.com/charmbracelet/vhs
  github.com/charmbracelet/ssh
  github.com/erikgeiser/coninput
  github.com/go-rod/rod
    github.com/ysmood/fetchup
      github.com/ysmood/got
      github.com/ysmood/gop
    github.com/ysmood/goob
      github.com/ysmood/gotrace
    github.com/ysmood/gson
    github.com/ysmood/leakless
  github.com/mattn/go-localereader

See that previously github.com/go-rod/rod was indicated as a single
dependency of github.com/charmbracelet/vhs, when in fact it had itself
multiple missing dependencies. The new implementation reports all of
them from the beginning.

It is still quite imprecise, probably due to module graph pruning [1],
but also because sometimes go mod graph returns a little bit too much
direct dependencies for an unknown reason.

[1] https://go.dev/ref/mod#graph-pruning
Instead of immediately checking that the repo root path is in the
list of packages in Debian, use the full import path (including the
potential major version suffix), then check for other major versions
if applicable, and finally, check for the repo root.

This allows to display that another major version is currently packaged
in Debian, and prevents hiding necessary major upgrades to already
packaged Go modules with a mismatched version.
@n-peugnet
Copy link
Contributor Author

Hehe sorry, I ended up renaming some things and adding another commit. But now it is ready 😅

Instead of running "go mod tidy" (which need the root of the repo to be
the root of the Go module) in the root of the repo, we now generate a
dummy module (called "dummymod") to be able to run "go get pkg/..."
again. This has multiple advantages:

1. It does not clone the full repo, which can be costly on very big
   repos.
2. It uses released/stable verisons of the module when available,
   which is more is more similar to what will be packaged in Debian.
3. It can handle repos with multiple modules or with a module in a
   subdirectory.
@ottok
Copy link
Contributor

ottok commented Jan 7, 2025

Nice work!

@n-peugnet
Copy link
Contributor Author

n-peugnet commented Jan 7, 2025

I did one last experiment in n-peugnet@d1d0372 (not included in this PR) that only highlights the actual repositories that need packaging. With the same example as before, but hacked a little to make as if github.com/charmbracelet/x is not yet packaged in Debian:

2025-01-07-145405_481x555_scrot

We can see that:

  1. /v11 is dimmed, as the repository root is in fact github.com/caarlos0/env (maybe it would be better not to dim it, I'm not sure).
  2. /conpty is dimmed as well, as the module is in a subfolder of the github.com/charmbracelet/x repository
  3. All modules of github.com/charmbracelet/x afterwards are completely dimmed, as they are part of a repository already printed

This allows to easily find the corresponding repositories (by copying only the highlighted parts) and to better estimate the packaging work needed.

In fact, If I'm not mistaken, the highlighted part of this picture is the exact output of the estimate command when it was still working.

Please tell me what you think about it, and if I should include it in this PR.

@ottok
Copy link
Contributor

ottok commented Jan 13, 2025

I will merge this in a couple of days unless somebody else reviews and comments something.

@n-peugnet
Copy link
Contributor Author

Just to make sure everyone is aware, this PR currently generates this output (still the same example):

2025/01/13 13:26:10 github.com/charmbracelet/x/conpty is packaged as github.com/charmbracelet/x in Debian
2025/01/13 13:26:10 github.com/charmbracelet/x/termios is packaged as github.com/charmbracelet/x in Debian
2025/01/13 13:26:10 github.com/charmbracelet/x/errors is packaged as github.com/charmbracelet/x in Debian
2025/01/13 13:26:10 github.com/charmbracelet/x/ansi is packaged as github.com/charmbracelet/x in Debian
2025/01/13 13:26:10 github.com/charmbracelet/x/exp/golden is packaged as github.com/charmbracelet/x in Debian
2025/01/13 13:26:10 github.com/charmbracelet/x/term is packaged as github.com/charmbracelet/x in Debian
2025/01/13 13:26:10 Bringing github.com/charmbracelet/vhs to Debian requires packaging the following Go modules:
github.com/charmbracelet/vhs
  github.com/caarlos0/env/v11	(github.com/caarlos0/env in Debian)
  github.com/charmbracelet/ssh
  github.com/go-rod/rod
    github.com/ysmood/fetchup
      github.com/ysmood/got
      github.com/ysmood/gop
    github.com/ysmood/goob
      github.com/ysmood/gotrace
    github.com/ysmood/gson
    github.com/ysmood/leakless
  github.com/erikgeiser/coninput
  github.com/mattn/go-localereader

I will propose the enhancement with dimmed colours in a following PR.

@ottok ottok merged commit e0737f9 into Debian:master Jan 14, 2025
2 checks passed
@n-peugnet n-peugnet deleted the fix-estimate branch January 14, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants