go importer does not honor multi-module repositories

  • Open
  • quality assurance status badge
Details
5 participants
  • Björn Höfling
  • Blake Shaw
  • Timo Wilken
  • raingloom
  • wolf
Owner
unassigned
Submitted by
Björn Höfling
Severity
normal
B
B
Björn Höfling wrote on 21 Feb 2022 23:43
(address . bug-guix@gnu.org)
20220221234354.7698f141@alma-ubu.fritz.box
Go usually has the 1 repository=1 module convention. However, it
is also allowed that one repository contains multiple go modules.

If repository "foo" contains only one module, then versions are tagged
"v1.2.3".

However, if the repository "foo" contains modules "bar" and "baz", each
in a sub-directory of "foo", the versions will be tagged with their
respective prefix, i.e.:

foo/v1.2.3
bar/v4.5.6

See here:


However, our go-importer does not honor this. The Google Cloud API
modules are structured into sub-modules, but our importer searches the
wrong tag and raises an exception:

$ ./pre-inst-env guix import go cloud.google.com/go/storage
Backtrace:
In ice-9/boot-9.scm:
1752:10 17 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
16 (apply-smob/0 #<thunk 7f5b3d24f0c0>)
In ice-9/boot-9.scm:
724:2 15 (call-with-prompt _ _ #<procedure default-prompt-handle?>)
In ice-9/eval.scm:
619:8 14 (_ #(#(#<directory (guile-user) 7f5b3d255c80>)))
In guix/ui.scm:
2209:7 13 (run-guix . _)
2172:10 12 (run-guix-command _ . _)
In guix/scripts/import.scm:
124:11 11 (guix-import . _)
In ice-9/boot-9.scm:
1752:10 10 (with-exception-handler _ _ #:unwind? _ # _)
In guix/scripts/import/go.scm:
116:29 9 (_)
In ice-9/exceptions.scm:
406:15 8 (go-module->guix-package* . _)
In ice-9/boot-9.scm:
1752:10 7 (with-exception-handler _ _ #:unwind? _ # _)
In guix/import/go.scm:
525:18 6 (go-module->guix-package "cloud.google.com/go/storage" # ?)
In guix/git.scm:
277:4 5 (update-cached-checkout _ #:ref _ #:recursive? _ # _ # _ ?)
266:18 4 (resolve _)
In git/reference.scm:
60:8 3 (_ _ _)
In git/bindings.scm:
77:2 2 (raise-git-error _)
In ice-9/boot-9.scm:
1685:16 1 (raise-exception _ #:continuable? _)
1683:16 0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1683:16: In procedure raise-exception:
Git error: reference 'refs/tags/v1.21.0' not found


The correct git reference to look for is:

refs/tags/storage/v1.21.0

Björn
-----BEGIN PGP SIGNATURE-----

iF0EAREKAB0WIQQiGUP0np8nb5SZM4K/KGy2WT5f/QUCYhQVqgAKCRC/KGy2WT5f
/ZkFAJ95cF6s1cJeWpRDWStaIRt/4l+pqACfYTBZk5zVa/zMVkRtlci2Myuj+bA=
=KgGG
-----END PGP SIGNATURE-----


R
R
raingloom wrote on 22 Feb 2022 01:38
(name . Björn Höfling)(address . bjoern.hoefling@bjoernhoefling.de)(address . 54097@debbugs.gnu.org)
20220222013848.06d515a1@riseup.net
On Mon, 21 Feb 2022 23:43:54 +0100
Björn Höfling <bjoern.hoefling@bjoernhoefling.de> wrote:

Toggle quote (72 lines)
> Go usually has the 1 repository=1 module convention. However, it
> is also allowed that one repository contains multiple go modules.
>
> If repository "foo" contains only one module, then versions are tagged
> "v1.2.3".
>
> However, if the repository "foo" contains modules "bar" and "baz",
> each in a sub-directory of "foo", the versions will be tagged with
> their respective prefix, i.e.:
>
> foo/v1.2.3
> bar/v4.5.6
>
> See here:
>
> https://github.com/golang/go/wiki/Modules#publishing-a-release
> https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories
> https://stackoverflow.com/questions/64701064/golang-separate-versioning-of-multiple-modules
>
> However, our go-importer does not honor this. The Google Cloud API
> modules are structured into sub-modules, but our importer searches the
> wrong tag and raises an exception:
>
> $ ./pre-inst-env guix import go cloud.google.com/go/storage
> URL FOR VERSIONS:
> https://proxy.golang.org/cloud.google.com/go/storage/@v/list
> FETCH_GO_MOD:
> https://proxy.golang.org/cloud.google.com/go/storage/@v/v1.21.0.mod
> Backtrace: In ice-9/boot-9.scm: 1752:10 17 (with-exception-handler _
> _ #:unwind? _ # _) In unknown file:
> 16 (apply-smob/0 #<thunk 7f5b3d24f0c0>)
> In ice-9/boot-9.scm:
> 724:2 15 (call-with-prompt _ _ #<procedure
> default-prompt-handle?>) In ice-9/eval.scm:
> 619:8 14 (_ #(#(#<directory (guile-user) 7f5b3d255c80>)))
> In guix/ui.scm:
> 2209:7 13 (run-guix . _)
> 2172:10 12 (run-guix-command _ . _)
> In guix/scripts/import.scm:
> 124:11 11 (guix-import . _)
> In ice-9/boot-9.scm:
> 1752:10 10 (with-exception-handler _ _ #:unwind? _ # _)
> In guix/scripts/import/go.scm:
> 116:29 9 (_)
> In ice-9/exceptions.scm:
> 406:15 8 (go-module->guix-package* . _)
> In ice-9/boot-9.scm:
> 1752:10 7 (with-exception-handler _ _ #:unwind? _ # _)
> In guix/import/go.scm:
> 525:18 6 (go-module->guix-package "cloud.google.com/go/storage" #
> ?) In guix/git.scm:
> 277:4 5 (update-cached-checkout _ #:ref _ #:recursive? _ # _ # _
> ?) 266:18 4 (resolve _)
> In git/reference.scm:
> 60:8 3 (_ _ _)
> In git/bindings.scm:
> 77:2 2 (raise-git-error _)
> In ice-9/boot-9.scm:
> 1685:16 1 (raise-exception _ #:continuable? _)
> 1683:16 0 (raise-exception _ #:continuable? _)
>
> ice-9/boot-9.scm:1683:16: In procedure raise-exception:
> Git error: reference 'refs/tags/v1.21.0' not found
>
>
> The correct git reference to look for is:
>
> refs/tags/storage/v1.21.0
>
> Björn
>

I think this has been mentioned before, but I really think we should
use Go's built-in tooling to extract this info, instead of
reimplementing it in Guile. There is already some massive ineffeciency
introduced by the importer cloning whole repos, whereas `go get` is
smart enough to only download `go.mod` files.
Just my 2c.
B
B
Blake Shaw wrote on 22 Feb 2022 13:59
(name . Björn Höfling)(address . bjoern.hoefling@bjoernhoefling.de)(address . 54097@debbugs.gnu.org)
87k0dn12hd.fsf@nonconstructivism.com
great investigation, thanks for sharing!
--
“In girum imus nocte et consumimur igni”
T
T
Timo Wilken wrote 7 days ago
[PATCH] import: go: Handle subpackage versioning correctly.
6dd1de3dd4d968876fa55f5126056834c77b0244.1684703258.git.guix@twilken.net
Some Go source repositories (notably the Google Cloud SDK) contain multiple
submodules and use a `refs/tags/<submodule>/<version>' tagging scheme.


* guix/import/go.scm (vcs->origin): Accept a module-path-suffix.
(go-module->guix-package): Use the new parameter.
---
Here's a patch that fixes the reported issue (bug#54097) for me. I've only
tested this on the github.com/googleapis/google-cloud-go/compute package so
far, though it seems to work there. Perhaps others have more testcases?

I don't know enough about Go tooling to use it, so I've just patched the Guile
logic of the importer. (I don't write Go, I just want to package stuff written
in it.) In terms of performance, at least the repo contents are apparently
cached by the first `git-checkout-hash' call, even if it fails, so the second
call doesn't have to redownload them.

guix/import/go.scm | 56 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 43 insertions(+), 13 deletions(-)

Toggle diff (110 lines)
diff --git a/guix/import/go.scm b/guix/import/go.scm
index 0357e6a1eb..652ac58b6f 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -7,6 +7,7 @@
;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2023 Timo Wilken <guix@twilken.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -89,6 +90,7 @@ (define-module (guix import go)
;;; TODO list
;;; - get correct hash in vcs->origin for Mercurial and Subversion
+;;; - handle subdir/vX.Y versioning in vcs->origin for Mercurial and Subversion
;;; Code:
@@ -513,29 +515,54 @@ (define* (git-checkout-hash url reference algorithm)
`(tag-or-commit . ,reference)))))
(file-hash* checkout #:algorithm algorithm #:recursive? #true)))
-(define (vcs->origin vcs-type vcs-repo-url version)
+(define (vcs->origin vcs-type vcs-repo-url module-path-suffix version)
"Generate the `origin' block of a package depending on what type of source
control system is being used."
(case vcs-type
((git)
- (let ((plain-version? (string=? version (go-version->git-ref version)))
- (v-prefixed? (string-prefix? "v" version)))
+ (let ((v-prefixed? (string-prefix? "v" version))
+ (path-prefixed? #f)
+ (trimmed-path-suffix (string-trim-both module-path-suffix #\/))
+ (checkout-hash (false-if-git-not-found
+ (git-checkout-hash
+ vcs-repo-url
+ (go-version->git-ref version)
+ (hash-algorithm sha256)))))
+ ;; If `checkout-hash' is false, that must mean that a tag named after
+ ;; the version doesn't exist. Some repos that contain submodules use a
+ ;; <submodule>/<version> tagging scheme instead, so try that.
+ (unless checkout-hash
+ (when (string=? "" trimmed-path-suffix)
+ ;; If this isn't a submodule, <submodule>/<version> tagging makes no sense.
+ ;; Tell the user we couldn't find the original version.
+ (raise
+ (formatted-message (G_ "could not find git reference '~a' in repository '~a'")
+ (go-version->git-ref version) vcs-repo-url)))
+ (set! path-prefixed? #t)
+ (set! checkout-hash (git-checkout-hash
+ vcs-repo-url
+ (go-version->git-ref
+ (string-append trimmed-path-suffix "/" version))
+ (hash-algorithm sha256))))
`(origin
(method git-fetch)
(uri (git-reference
(url ,vcs-repo-url)
- ;; This is done because the version field of the package,
- ;; which the generated quoted expression refers to, has been
- ;; stripped of any 'v' prefixed.
- (commit ,(if (and plain-version? v-prefixed?)
- '(string-append "v" version)
- '(go-version->git-ref version)))))
+ ;; The 'v' is prepended again because the version field of
+ ;; the package, which the generated quoted expression refers
+ ;; to, has been stripped of any 'v' prefixed.
+ (commit (go-version->git-ref
+ ,(cond
+ (path-prefixed?
+ `(string-append
+ ,trimmed-path-suffix "/"
+ ,@(if v-prefixed? '("v" version) '(version))))
+ (v-prefixed? '(string-append "v" version))
+ (else 'version))))))
(file-name (git-file-name name version))
(sha256
(base32
- ,(bytevector->nix-base32-string
- (git-checkout-hash vcs-repo-url (go-version->git-ref version)
- (hash-algorithm sha256))))))))
+ ,(bytevector->nix-base32-string checkout-hash))))))
((hg)
`(origin
(method hg-fetch)
@@ -614,6 +641,9 @@ (define* (go-module->guix-package module-path #:key
(match:prefix (string-match "([\\./]v[0-9]+)?$" module-path)))
(guix-name (go-module->guix-package-name module-path))
(root-module-path (module-path->repository-root module-path))
+ (module-path-suffix ; subdirectory inside the source repo
+ (substring module-path-sans-suffix
+ (string-prefix-length root-module-path module-path-sans-suffix)))
;; The VCS type and URL are not included in goproxy information. For
;; this we need to fetch it from the official module page.
(meta-data (fetch-module-meta-data root-module-path))
@@ -627,7 +657,7 @@ (define* (go-module->guix-package module-path #:key
(name ,guix-name)
(version ,(strip-v-prefix version*))
(source
- ,(vcs->origin vcs-type vcs-repo-url version*))
+ ,(vcs->origin vcs-type vcs-repo-url module-path-suffix version*))
(build-system go-build-system)
(arguments
'(#:import-path ,module-path

base-commit: e499cb2c12d7f1c6d2f004364c9cc7bdb7e38cd5
--
2.40.1
W
W
wolf wrote 7 days ago
(name . Timo Wilken)(address . guix@twilken.net)
ZGqTGVrlJsLi9hxW@ws
Attachment: file
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEt4NJs4wUfTYpiGikL7/ufbZ/wakFAmRqkxkACgkQL7/ufbZ/
wamucg//aKKPWvermh5Y9aM+7NGoKaq2DqtFlIO+A32wnJcZQupMRWzKyRzfnT77
CXIwMuKwz53Dr6OaB2HP3oiXjREkISg1PdHUBAJ4ziJX+SAywlZihqjlh5698e1K
m0izXVjx3IIMjLqEv6EMCgAKLRG/4HpdGmG7oTtsMN0KGHD2Kufo1Mz3lR0zYUqx
LDciOb10EV7XyJvjeYHUKtzz8EzF4NQO4+S0gCY6kN1PYjFEx9wy9s996W+i7ngY
EZWMZLL1ppbQtmcDzdqjgLebZ5+/P8ZjmuuELBmuBtCaUhtu3ViOSC7ieOkVkD+m
8OWW078xqHYDiTWc9Qz6K/+PsTQUJxFJRro3cHXzPV+Go9gGer921Vr0Ua4r1cCg
K0WIuTjS2wMjIOdsnqPfPVWGiks6zVe07fQFze38OtcbfadVJe2fxCiBXHeq5p7p
kwhZDrtcMrlBE0moNaFjPC061EtW4s9BrsLPy1NszeGp+gW12/NUyUeLv3BK3gbM
8DvAQo7QYGioIB2H0xPewMj4mArMoDhbFlZZ0cnhqGrd3snir3XXZu0Cw4hSJQBt
uPIvS4XsTmCKWV4c11mYKW4T78iFkP7IFHV1JiIcLYP1W5E2fCZhcHMoUB+VFvcF
lU2v6NhJzuQuPeMB/Ngj4Dey1IXrs5SbvFsISpPOboL3nRuRGgQ=
=zjKZ
-----END PGP SIGNATURE-----


T
T
Timo Wilken wrote 6 days ago
(name . wolf)(address . wolf@wolfsden.cz)(address . 54097@debbugs.gnu.org)
CST1MCR6Z0PA.2YU25GTLRLRIJ@lap.twilken.net
Hi wolf,

On Sun May 21, 2023 at 11:54 PM CEST, wolf wrote:
Toggle quote (5 lines)
> Please give the github.com/Azure/go-autorest/tracing@v0.6.0 a go. My code
> failed on it, and (assuming I applied the patch correctly) your does as well.
> Here are reproduction steps to make it easier for you (please tell me if I did
> something wrong):

I don't think you did anything wrong there.

That's an issue I've run into in the past as well, though it seems to be a bug
in the Go build system, not the importer (and in my patch I only touch the
importer).

Toggle quote (6 lines)
> I will not pretend to have a full grasp on how (guix build-system go) works,
> however my debugging lead me to the observation that it tries to unpack two
> dependencies into one file system tree overlayed on top of each other. I think
> the current way (GO111MODULE=off) of building of golang packages does not play
> very well with well, go modules.

Fair enough! I don't know much about Go -- I don't write software in it, I
just want to package some stuff written in it; in my case, that's
Matrix-related programs.

Toggle quote (6 lines)
> Either the build system needs to be smarter about unpacking dependencies (and
> doing it in a correct order), or we should start using go modules for the builds
> (it can still be down offline, just the dependencies are in different paths).
> The second approach is what I wanted to explore, but did not get to it yet (and
> likely will not for a month or two).

Your second approach sounds sensible!

If I can find the time and motivation to dig in to this, I might have a go as
well... But if you get anything working, that would be much appreciated! :)

Cheers,
Timo
?