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

[wolf]

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
Description: PGP signature