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

Message #5 received at submit@debbugs.gnu.org (full text, mbox, reply):

From: Timo Wilken <guix@twilken.net>

To: guix-patches@gnu.org, 54097@debbugs.gnu.org

Cc: wolf@wolfsden.cz, Timo Wilken <guix@twilken.net>

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

Date: Sun, 21 May 2023 23:18:08 +0200

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.

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

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

Message #8 received at submit@debbugs.gnu.org (full text, mbox, reply):

From: wolf <wolf@wolfsden.cz>

To: Timo Wilken <guix@twilken.net>

Cc: 54097@debbugs.gnu.org, guix-patches@gnu.org

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

Date: Sun, 21 May 2023 23:54:33 +0200

[Message part 1 (text/plain, inline)]

Hi,

What a coincidence, this week I happened to take a look at this same issue (in
my case I wanted to build terraform).  I failed to get it properly working, so
I'm happy someone else took a look at it as well.

On 2023-05-21 23:18:08 +0200, Timo Wilken wrote:
> 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?

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):

    $ echo '(use-modules (guix packages) (guix git-download) (guix build-system go) ((guix licenses) #:prefix license:))' >/tmp/x.scm
    $ ./pre-inst-env guix import go -r github.com/Azure/go-autorest/tracing@v0.6.0 >>/tmp/x.scm
    $ echo go-github-com-azure-go-autorest-tracing >>/tmp/x.scm
    $ guix build -f /tmp/x.scm
    [..]
    starting phase `unpack'
    `/gnu/store/857z63cfgclsh6g52vj9xnm7iv97yz97-go-github-com-azure-go-autorest-tracing-0.6.0-checkout/.gitignore' -> `/tmp/guix-build-go-github-com-azure-go-autorest-tracing-0.6.0.drv-0/src/github.com/Azure/go-autorest/.gitignore'
    error: in phase 'unpack': uncaught exception:
    system-error "copy-file" "~A" ("Permission denied") (13) 
    phase `unpack' failed after 0.0 seconds
    [..]

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.

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).

> 
> 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(-)
> 
> 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
>

I did not really take a look at the scheme code, I'm still Guix and Scheme
beginner, so I'm very much not up to the task of doing actual code review.
Nevertheless, I hope my mail helps at least a bit.

Have a nice day,
W.

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 63631@debbugs.gnu.org (full text, mbox, reply):

From: Ludovic Courtès <ludo@gnu.org>

To: Timo Wilken <guix@twilken.net>

Cc: 63647@debbugs.gnu.org, 63631@debbugs.gnu.org, Simon Tournier <zimon.toutoune@gmail.com>, 54097@debbugs.gnu.org, wolf@wolfsden.cz

Subject: Re: bug#63631: [PATCH] import: go: Handle subpackage versioning correctly.

Date: Wed, 14 Jun 2023 23:09:35 +0200

Hi Timo,

Timo Wilken <guix@twilken.net> skribis:

> 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
submitted at <https://issues.guix.gnu.org/63647>.

What would you suggest?  Simon?

Thanks for the patch, Timo!

Ludo’.

Message #14 received at 63631@debbugs.gnu.org (full text, mbox, reply):

From: "Timo Wilken" <guix@twilken.net>

To: Ludovic Courtès <ludo@gnu.org>

Cc: 63631@debbugs.gnu.org, 64036@debbugs.gnu.org, Simon Tournier <zimon.toutoune@gmail.com>, 63647@debbugs.gnu.org, 64035@debbugs.gnu.org, 63001@debbugs.gnu.org, 54097@debbugs.gnu.org, wolf@wolfsden.cz

Subject: Re: bug#63631: [PATCH] import: go: Handle subpackage versioning correctly.

Date: Sat, 17 Jun 2023 17:12:58 +0200

[Message part 1 (text/plain, inline)]

Hi Ludo', (hi everyone,)

On Wed Jun 14, 2023 at 11:09 PM CEST, Ludovic Courtès wrote:
> 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!

> 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)...

> 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...

> Thanks for the patch, Timo!

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

Cheers,
Timo

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 63631@debbugs.gnu.org (full text, mbox, reply):

From: Simon Tournier <zimon.toutoune@gmail.com>

To: Timo Wilken <guix@twilken.net>, Ludovic Courtès <ludo@gnu.org>

Cc: 63631@debbugs.gnu.org, 64036@debbugs.gnu.org, 63647@debbugs.gnu.org, 64035@debbugs.gnu.org, 63001@debbugs.gnu.org, 54097@debbugs.gnu.org, wolf@wolfsden.cz

Subject: Re: bug#63001: bug#63631: [PATCH] import: go: Handle subpackage versioning correctly.

Date: Wed, 16 Aug 2023 17:59:53 +0200

Hi Timo,

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

>> 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

Send a report that this bug log contains spam.