[PATCH] import: go: Handle subpackage versioning correctly.

  • Open
  • quality assurance status badge
Details
5 participants
  • Timo Wilken
  • Ludovic Courtès
  • Christopher Baines
  • wolf
  • Simon Tournier
Owner
unassigned
Submitted by
Timo Wilken
Severity
normal
T
T
Timo Wilken wrote on 21 May 2023 23:18
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
(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-----


L
L
Ludovic Courtès wrote on 14 Jun 2023 23:09
Re: bug#63631: [PATCH] import: go: Handle subpackage versioning correctly.
(name . Timo Wilken)(address . guix@twilken.net)
87pm5xrbsg.fsf@gnu.org
Hi Timo,

Timo Wilken <guix@twilken.net> skribis:

Toggle quote (18 lines)
> Some Go source repositories (notably the Google Cloud SDK) contain multiple
> submodules and use a `refs/tags/<submodule>/<version>' tagging scheme.
>
> Fixes <https://bugs.gnu.org/54097>.
>
> * 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.

What you propose looks similar to part of the work Simon Tournier

What would you suggest? Simon?

Thanks for the patch, Timo!

Ludo’.
T
T
Timo Wilken wrote on 17 Jun 2023 17:12
(name . Ludovic Courtès)(address . ludo@gnu.org)
CTF06XBYWPT0.1MV6QA1B2OB98@lap.twilken.net
Hi Ludo', (hi everyone,)

On Wed Jun 14, 2023 at 11:09 PM CEST, Ludovic Courtès wrote:
Toggle quote (11 lines)
> Timo Wilken <guix@twilken.net> skribis:
> > 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.

I've been testing my patch further this weekend, and I have a couple more
patches in the pipeline; I suppose I ought to clean those up and submit them.

In particular, I've got fixes for the following queued up locally:

1. Finding the `module-path-subdir' needs another case for e.g.
cloud.google.com/go/*.

2. My patch sometimes generates an unnecessary `go-version->git-ref' call.

3. Go versions need to be parsed from go.mod, since some packages require a
newer Go compiler than our default. This I've got a patch for, but this Go
version also ought to propagate up the dependency tree. I haven't found an
easy way to do that, since the importer seems to generate top-level
packages first, before descending the dep tree...

4. `fetch-module-meta-data' ought to ignore 4xx HTTP errors to follow the
spec; gonum.org/v1/gonum specifically depends on this behaviour.

I've been trying to recursively import github.com/matrix-org/dendrite, which
has a particularly large and hairy dependency tree. While I can now import it
without crashes, I can't build it from the imported package definitions yet --
mainly because of lots of dependency cycles in the generated packages, but
there may be more issues hidden beneath that.

Still, I can recommend it as a test of everyone's importer patches, since
it'll find a lot of edge cases in importing alone!

Toggle quote (3 lines)
> What you propose looks similar to part of the work Simon Tournier
> submitted at <https://issues.guix.gnu.org/63647>.

It seems lots of people have been working on the same problem -- in addition
to Simon's patches, I found a patch submitted by Elbek (issues 64035 & 64036;
Cc'd). I also forgot about the issue I submitted months ago (63001)...

Toggle quote (2 lines)
> What would you suggest? Simon?

Here's a brief comparison between Simon's patches and mine -- Simon's seem to
contain fixes for a couple more things than mine currently does:

1. Simon sorts available versions in an error message; this can presumably be
merged independently since it doesn't conflict with other patches.

2. Simon always prepends a "SUBDIR/" prefix to the tag if found, whereas I try
to find the plain "vX" tag first, then fall back to "SUBDIR/vX". Judging by
https://go.dev/ref/mod#vcs-version,Simon's approach seems more correct.
I'll change my implementation to match and try it out.

3. For detecting the `module-path-subdirectory' in Simon's patches: that's the
same approach I used initially, but I found I have to try `(substring
module-path (string-length import-prefix))' first (to handle e.g.
cloud.google.com/go/*). This is one of the things I haven't submitted
yet...

Toggle quote (2 lines)
> Thanks for the patch, Timo!

Thanks for your work in sorting through all of this, Ludo'!

Cheers,
Timo
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEU+w8BoVog92SNVvCL8eFBGgfabAFAmSNzaUACgkQL8eFBGgf
abDEaA//Y6k/Uckhvq+wllES0TjACxVG+2bY2E/wLZNoQShXF5Tn3ZQ9o2HOU7Gr
p8er/t/JkPLYm9w2MidUtvTms6qxp8H/YIZ12gzZ+GP/eoKa/iiqt42+KZvLS/Is
B2JWEcaJLFYy0w9nuubeuQpgcWEmxyk7/zb++GovD0tfYgczII3N1Z5fhRmeAj0N
FbKqfkBocxruYvBfcI5KhMV6Q1TjCwuAH+j2iwNmsiSnm7NTKbJxfCyO9+p5UF9w
O/em01p9J+Cg6EEFunwoi1pjoEAkVZCRtgaiP1SMgapq74UKKagvsWqMoW+JHpDf
uuiaVn1sLuTt+u9KGamYqaCj1NRcCfN7SjPxS7oTM4hsytVDmUB/eIHD6t3HntCP
Ev43fJKJwLoybQV0ptajnJggEEaEY1cegQG683jsD0ycrToh4YEhLe5fQsGhaLMA
7MSLjjf0K73BoFyZi+wYyfDPYqXRQmpb3Z6cKJPlF3v4IF13UtSxP0yBRdz930aI
1b1Q5khLQzQBI0fvxMNcZGPeA2RjPyNsXE8pKn9zEkIunzX9VYN8az++EjX928Ox
mmgjEUsLWMPCSMwGcEoZye6R5wJUG34LEkoQTbySf+314hlELPuGcLCatFgZZbXq
1Hn1upt13XYRYVCX/SXIpBJL9gdNbi2sSg8FVm4Aq+sQQqZzAKs=
=fjJn
-----END PGP SIGNATURE-----


S
S
Simon Tournier wrote on 16 Aug 2023 17:59
Re: bug#63001: bug#63631: [PATCH] import: go: Handle subpackage versioning correctly.
875y5ff05i.fsf@gmail.com
Hi Timo,

On Sat, 17 Jun 2023 at 17:12, "Timo Wilken" <guix@twilken.net> wrote:

Toggle quote (19 lines)
>> What would you suggest? Simon?
>
> Here's a brief comparison between Simon's patches and mine -- Simon's seem to
> contain fixes for a couple more things than mine currently does:
>
> 1. Simon sorts available versions in an error message; this can presumably be
> merged independently since it doesn't conflict with other patches.
>
> 2. Simon always prepends a "SUBDIR/" prefix to the tag if found, whereas I try
> to find the plain "vX" tag first, then fall back to "SUBDIR/vX". Judging by
> https://go.dev/ref/mod#vcs-version, Simon's approach seems more correct.
> I'll change my implementation to match and try it out.
>
> 3. For detecting the `module-path-subdirectory' in Simon's patches: that's the
> same approach I used initially, but I found I have to try `(substring
> module-path (string-length import-prefix))' first (to handle e.g.
> cloud.google.com/go/*). This is one of the things I haven't submitted
> yet...

Sorry if I have missed some patches or overlooked something. Do you
plan to send another patch series handling all?


Cheers,
simon
C
C
Christopher Baines wrote on 17 Oct 2023 10:29
tag 63631 moreinfo
(address . control@debbugs.gnu.org)
87a5shwt1b.fsf@cbaines.net
tags 63631 + moreinfo
quit
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 63631@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 63631
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch