[bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'

[bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'

[Simon Tournier]

Hi,

Instead of that:

--8<---------------cut here---------------start------------->8---
$ guix import go software.sslmate.com/src/go-pkcs12
guix import: Importing package "software.sslmate.com/src/go-pkcs12"...
guix import: warning: Unable to determine repository root of 'software.sslmate.com/src/go-pkcs12'. Guessing 'software.sslmate.com/src/go-pkcs12'.
SWH: found revision fa70679f0f1622a2705336a97225ee8d6c555f96 with directory at 'https://archive.softwareheritage.org/api/1/directory/43d3e9f7167cdcd9f179e8d1de33d0d22e8fbd46/'
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/HEAD
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/branches/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/config
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/description
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/hooks/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/exclude
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/refs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/packs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.idx
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.pack
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/master
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/tags/
guix import: warning: Backtrace:
In ice-9/boot-9.scm:
  1752:10 12 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
          11 (apply-smob/0 #<thunk 7fa8659d9300>)
In ice-9/boot-9.scm:
    724:2 10 (call-with-prompt _ _ #<procedure default-prompt-handle?>)
In ice-9/eval.scm:
    619:8  9 (_ #(#(#<directory (guile-user) 7fa8659dcc80>)))
In guix/ui.scm:
   2330:7  8 (run-guix . _)
  2293:10  7 (run-guix-command _ . _)
In guix/scripts/import.scm:
     82:6  6 (guix-import . _)
In ice-9/boot-9.scm:
  1752:10  5 (with-exception-handler _ _ #:unwind? _ # _)
In guix/scripts/import/go.scm:
   116:29  4 (_)
In ice-9/eval.scm:
    619:8  3 (_ #(#(#<directory (guix import go) 7fa85b580d20> # ?) ?))
   174:20  2 (_ #(#(#<directory (guix import go) 7fa85b580d20> # ?) ?))
   177:32  1 (lp (#<procedure 7fa8598a1380 at ice-9/eval.scm:191:12?>))
In ice-9/boot-9.scm:
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure git-error-message: Wrong type argument: #<&compound-exception components: (#<&error> #<&irritants irritants: (#<<git-error> code: -3 message: "reference 'refs/tags/v0.5.0' not found" class: 4>)> #<&exception-with-kind-and-args kind: git-error args: (#<<git-error> code: -3 message: "reference 'refs/tags/v0.5.0' not found" class: 4>)>)>
--8<---------------cut here---------------end--------------->8---

which is annoying because it is blocking when using the importer recursively,
the patch set produces that:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import go software.sslmate.com/src/go-pkcs12
guix import: Importing package "software.sslmate.com/src/go-pkcs12"...
guix import: warning: Unable to determine repository root of 'software.sslmate.com/src/go-pkcs12'. Guessing 'software.sslmate.com/src/go-pkcs12'.
SWH: found revision fa70679f0f1622a2705336a97225ee8d6c555f96 with directory at 'https://archive.softwareheritage.org/api/1/directory/43d3e9f7167cdcd9f179e8d1de33d0d22e8fbd46/'
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/HEAD
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/branches/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/config
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/description
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/hooks/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/exclude
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/refs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/packs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.idx
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.pack
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/master
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/tags/
guix import: warning: Git cannot find the reference v0.5.0
(define-public go-software-sslmate-com-src-go-pkcs12
  (package
    (name "go-software-sslmate-com-src-go-pkcs12")
    (version "0.5.0")
    (source
     (origin
       (method git-fetch)
       (uri (git-reference
             (url "https://software.sslmate.com/src/go-pkcs12.git")
             (commit (string-append "v" version))))
       (file-name (git-file-name name version))
       (sha256
        (base32 "0000000000000000000000000000000000000000000000000000"))))
    (build-system go-build-system)
    (arguments
     (list
      #:import-path "software.sslmate.com/src/go-pkcs12"))
    (home-page "https://software.sslmate.com/src/go-pkcs12")
    (synopsis "package pkcs12")
    (description
     "Package pkcs12 implements some of PKCS#12 (also known as P12 or PFX).  It is
intended for decoding DER-encoded P12/PFX files for use with the crypto/tls
package, and for encoding P12/PFX files for use by legacy applications which do
not support newer formats.  Since PKCS#12 uses weak encryption primitives, it
SHOULD NOT be used for new applications.")
    (license license:bsd-3)))
--8<---------------cut here---------------end--------------->8---


Please double check because it tweaks 'update-cached-checkout' which is used
by "guix pull". :-)

Cheers,
simon


Simon Tournier (2):
  git: Catch Git errors when updating cached checkout.
  import: go: Warn instead of error out when reference is missing.

 guix/git.scm       | 128 ++++++++++++++++++++++++++-------------------
 guix/import/go.scm |  10 ++--
 2 files changed, 82 insertions(+), 56 deletions(-)


base-commit: 043f02462766a913080723ad286028a288b79373
-- 
2.46.0

[bug#74481] [PATCH 1/2] git: Catch Git errors when updating cached checkout.

[Simon Tournier]

* guix/git.scm (resolve-reference): Catch Git error when reference does not
exist and return #false.
(switch-to-ref): Adjust.
(update-cached-checkout)[close-and-clean!]: New helper.
Catch Git error when reference does not exist and warn.
Return #false values when reference does not exist.

Change-Id: If6e244fe40ebee978ec8de51a6a68bcbd4a2c79e
---
 guix/git.scm | 128 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 53 deletions(-)

diff --git a/guix/git.scm b/guix/git.scm
index 410cd4c153..328e2d5c8c 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -5,7 +5,7 @@
 ;;; Copyright © 2021 Marius Bakke <marius@gnu.org>
 ;;; Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2023 Tobias Geerinckx-Rice <me@tobias.gr>
-;;; Copyright © 2023 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2023, 2024 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -265,7 +265,7 @@ (define (tag->commit repository tag)
 
 (define (resolve-reference repository ref)
   "Resolve the branch, commit or tag specified by REF, and return the
-corresponding Git object."
+corresponding Git object. Return #false if REF is not found."
   (let resolve ((ref ref))
     (match ref
       (('branch . branch)
@@ -281,9 +281,12 @@ (define (resolve-reference repository ref)
          ;; can't be sure it's available.  Furthermore, 'string->oid' used to
          ;; read out-of-bounds when passed a string shorter than 40 chars,
          ;; which is why we delay calls to it below.
-         (if (< len 40)
-             (object-lookup-prefix repository (string->oid commit) len)
-             (object-lookup repository (string->oid commit)))))
+         (catch 'git-error
+           (lambda ()
+             (if (< len 40)
+                 (object-lookup-prefix repository (string->oid commit) len)
+                 (object-lookup repository (string->oid commit))))
+           (lambda _ #f))))
       (('tag-or-commit . str)
        (cond ((and (string-contains str "-g")
                    (match (string-split str #\-)
@@ -300,7 +303,10 @@ (define (resolve-reference repository ref)
               => (lambda (commit) (resolve `(commit . ,commit))))
              ((or (> (string-length str) 40)
                   (not (string-every char-set:hex-digit str)))
-              (resolve `(tag . ,str)))      ;definitely a tag
+              (catch 'git-error         ;definitely a tag
+                (lambda ()
+                  (resolve `(tag . ,str)))
+                (lambda _ #f)))
              (else
               (catch 'git-error
                 (lambda ()
@@ -336,13 +342,15 @@ (define (switch-to-ref repository ref)
   (define obj
     (resolve-reference repository ref))
 
-  (reset repository obj RESET_HARD)
+  (and obj
+       (begin
+         (reset repository obj RESET_HARD)
 
-  ;; There might still be untracked files in REPOSITORY due to an interrupted
-  ;; checkout for example; delete them.
-  (delete-untracked-files repository)
+         ;; There might still be untracked files in REPOSITORY due to an interrupted
+         ;; checkout for example; delete them.
+         (delete-untracked-files repository)
 
-  (object-id obj))
+         (object-id obj))))
 
 (define (call-with-repository directory proc)
   (let ((repository #f))
@@ -562,6 +570,31 @@ (define* (update-cached-checkout url
                    (string-append directory "/" file)))
                 (or (scandir directory) '())))
 
+  (define (close-and-clean! repository)
+    (repository-close! repository)
+
+    ;; Update CACHE-DIRECTORY's mtime to so the cache logic sees it.
+    (match (gettimeofday)
+      ((seconds . microseconds)
+       (let ((nanoseconds (* 1000 microseconds)))
+         (utime cache-directory
+                seconds seconds
+                nanoseconds nanoseconds))))
+
+    ;; Run 'git gc' if needed.
+    (maybe-run-git-gc cache-directory)
+
+    ;; When CACHE-DIRECTORY is a sub-directory of the default cache
+    ;; directory, remove expired checkouts that are next to it.
+    (let ((parent (dirname cache-directory)))
+      (when (string=? parent (%repository-cache-directory))
+        (maybe-remove-expired-cache-entries parent cache-entries
+                                            #:entry-expiration
+                                            cached-checkout-expiration
+                                            #:delete-entry delete-checkout
+                                            #:cleanup-period
+                                            %checkout-cache-cleanup-period))))
+
   (define canonical-ref
     ;; We used to require callers to specify "origin/" for each branch, which
     ;; made little sense since the cache should be transparent to them.  So
@@ -583,56 +616,45 @@ (define* (update-cached-checkout url
      ;; Only fetch remote if it has not been cloned just before.
      (when (and cache-exists?
                 (not (reference-available? repository ref)))
-       (remote-fetch (remote-lookup repository "origin")
-                     #:fetch-options (make-default-fetch-options)))
+       (catch 'git-error
+         (lambda ()
+           (remote-fetch (remote-lookup repository "origin")
+                         #:fetch-options (make-default-fetch-options)))
+         (lambda _
+           (warning (G_ "Git cannot update the cache of ~a~%") url))))
      (when recursive?
        (update-submodules repository #:log-port log-port
                           #:fetch-options (make-default-fetch-options)))
 
      ;; Note: call 'commit-relation' from here because it's more efficient
      ;; than letting users re-open the checkout later on.
-     (let* ((oid      (if check-out?
+     (let ((oid      (if check-out?
                           (switch-to-ref repository canonical-ref)
                           (object-id
-                           (resolve-reference repository canonical-ref))))
-            (new      (and starting-commit
-                           (commit-lookup repository oid)))
-            (old      (and starting-commit
-                           (false-if-git-not-found
-                            (commit-lookup repository
-                                           (string->oid starting-commit)))))
-            (relation (and starting-commit
-                           (if old
-                               (commit-relation old new)
-                               'unrelated))))
-
-       ;; Reclaim file descriptors and memory mappings associated with
-       ;; REPOSITORY as soon as possible.
-       (repository-close! repository)
-
-       ;; Update CACHE-DIRECTORY's mtime to so the cache logic sees it.
-       (match (gettimeofday)
-         ((seconds . microseconds)
-          (let ((nanoseconds (* 1000 microseconds)))
-            (utime cache-directory
-                   seconds seconds
-                   nanoseconds nanoseconds))))
-
-       ;; Run 'git gc' if needed.
-       (maybe-run-git-gc cache-directory)
-
-       ;; When CACHE-DIRECTORY is a sub-directory of the default cache
-       ;; directory, remove expired checkouts that are next to it.
-       (let ((parent (dirname cache-directory)))
-         (when (string=? parent (%repository-cache-directory))
-           (maybe-remove-expired-cache-entries parent cache-entries
-                                               #:entry-expiration
-                                               cached-checkout-expiration
-                                               #:delete-entry delete-checkout
-                                               #:cleanup-period
-                                               %checkout-cache-cleanup-period)))
-
-       (values cache-directory (oid->string oid) relation)))))
+                           (resolve-reference repository canonical-ref)))))
+       (if (not oid)
+           (begin
+             (warning (G_ "Git cannot find the reference ~a~%") (match canonical-ref
+                                                                  ((_ . ref) ref)))
+             (close-and-clean! repository)
+             (values #f #f #f))
+           (let* ((new      (and starting-commit
+                                 (commit-lookup repository oid)))
+                  (old      (and starting-commit
+                                 (false-if-git-not-found
+                                  (commit-lookup repository
+                                                 (string->oid starting-commit)))))
+                  (relation (and starting-commit
+                                 (if old
+                                     (commit-relation old new)
+                                     'unrelated))))
+
+             ;; Reclaim file descriptors and memory mappings associated with
+             ;; REPOSITORY as soon as possible.
+
+             (close-and-clean! repository)
+
+             (values cache-directory (oid->string oid) relation)))))))
 
 (define* (latest-repository-commit store url
                                    #:key
-- 
2.46.0

[bug#74481] [PATCH 1/2] git: Catch Git errors when updating cached checkout.

[Ludovic Courtès]

Hi,

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

> * guix/git.scm (resolve-reference): Catch Git error when reference does not
> exist and return #false.
> (switch-to-ref): Adjust.
> (update-cached-checkout)[close-and-clean!]: New helper.
> Catch Git error when reference does not exist and warn.
> Return #false values when reference does not exist.
>
> Change-Id: If6e244fe40ebee978ec8de51a6a68bcbd4a2c79e

[...]

>  (define (resolve-reference repository ref)
>    "Resolve the branch, commit or tag specified by REF, and return the
> -corresponding Git object."
> +corresponding Git object. Return #false if REF is not found."
>    (let resolve ((ref ref))
>      (match ref
>        (('branch . branch)
> @@ -281,9 +281,12 @@ (define (resolve-reference repository ref)
>           ;; can't be sure it's available.  Furthermore, 'string->oid' used to
>           ;; read out-of-bounds when passed a string shorter than 40 chars,
>           ;; which is why we delay calls to it below.
> -         (if (< len 40)
> -             (object-lookup-prefix repository (string->oid commit) len)
> -             (object-lookup repository (string->oid commit)))))
> +         (catch 'git-error
> +           (lambda ()
> +             (if (< len 40)
> +                 (object-lookup-prefix repository (string->oid commit) len)
> +                 (object-lookup repository (string->oid commit))))
> +           (lambda _ #f))))
>        (('tag-or-commit . str)
>         (cond ((and (string-contains str "-g")
>                     (match (string-split str #\-)
> @@ -300,7 +303,10 @@ (define (resolve-reference repository ref)
>                => (lambda (commit) (resolve `(commit . ,commit))))
>               ((or (> (string-length str) 40)
>                    (not (string-every char-set:hex-digit str)))
> -              (resolve `(tag . ,str)))      ;definitely a tag
> +              (catch 'git-error         ;definitely a tag
> +                (lambda ()
> +                  (resolve `(tag . ,str)))
> +                (lambda _ #f)))

Sorry for not replynig earlier.  I would avoid such a change: it changes
the semantics of ‘resolve-reference’ in a non-trivial and hard-to-test
way.

Instead, since the goal is to address a problem that’s specific to
importers (or to one importer), I would suggest making a local change in
the importer itself, or in code that is shared among importers only.

(This comment applies to this entire patch.)

Ludo’.

[bug#74481] [PATCH 1/2] git: Catch Git errors when updating cached checkout.

[Simon Tournier]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

Hi Ludo,

On Thu, 12 Dec 2024 at 12:35, Ludovic Courtès <ludo@gnu.org> wrote:

> Sorry for not replynig earlier.  I would avoid such a change: it changes
> the semantics of ‘resolve-reference’ in a non-trivial and hard-to-test
> way.

Could you explain more what you mean for "the semantics" of
'resolve-reference'?

> Instead, since the goal is to address a problem that’s specific to
> importers (or to one importer), I would suggest making a local change in
> the importer itself, or in code that is shared among importers only.

Well, from my understanding this suggestion is not possible because of the
way it's implemented.  Or the importer needs a rewrite.  The crash comes
from the call of 'update-cached-checkout' and it appears to me impossible
to catch the error at the level of the importer because it's too late.

If you think that's possible to have a local change somewhere in the
importers, please point me because I'm clueless. :-)

BTW, the bug of looking for an non-existent reference still remains.  In
other words, all the calls to Guile-Git as 'object-lookup repository'
should be protected with some error-catch, IMHO.

Cheers,
simon

[-- Attachment #2: Type: text/html, Size: 1510 bytes --]

[bug#74481] [PATCH 2/2] import: go: Warn instead of error out when reference is missing.

[Simon Tournier]

* guix/import/go.scm (git-checkout-hash): Return some hash although the
checkout does not contain the expected reference.

Change-Id: I40287fae29042a0ba106939a413239df6efde97d
---
 guix/import/go.scm | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 32cba25b33..25ae989378 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -6,7 +6,7 @@
 ;;; Copyright © 2021-2022 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
 ;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
-;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2021, 2024 Simon Tournier <zimon.toutoune@gmail.com>
 ;;; Copyright © 2023 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2024 Christina O'Donnell <cdo@mutix.org>
 ;;;
@@ -38,7 +38,8 @@ (define-module (guix import go)
   #:use-module (guix http-client)
   #:use-module (guix memoization)
   #:autoload   (htmlprag) (html->sxml)            ;from Guile-Lib
-  #:autoload   (guix base32) (bytevector->nix-base32-string)
+  #:autoload   (guix base32) (bytevector->nix-base32-string
+                              nix-base32-string->bytevector)
   #:autoload   (guix build utils) (mkdir-p)
   #:autoload   (guix ui) (warning)
   #:autoload   (gcrypt hash) (hash-algorithm sha256)
@@ -566,7 +567,10 @@ (define* (git-checkout-hash url reference algorithm)
                   (update-cached-checkout url
                                           #:ref
                                           `(tag-or-commit . ,reference)))))
-    (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
+    (if (and checkout commit)
+        (file-hash* checkout #:algorithm algorithm #:recursive? #true)
+        (nix-base32-string->bytevector
+         "0000000000000000000000000000000000000000000000000000"))))
 
 (define (vcs->origin vcs-type vcs-repo-url version subdir)
   "Generate the `origin' block of a package depending on what type of source
-- 
2.46.0

[bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'

[Simon Tournier]

Hi,

On Fri, 22 Nov 2024 at 20:05, Simon Tournier <zimon.toutoune@gmail.com> wrote:

> Please double check because it tweaks 'update-cached-checkout' which is used
> by "guix pull". :-)

If no more comment on that, I will push shortly.

Cheers,
simon

[bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'

[Sharlatan Hellseher]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

Hi,

If it solves import issues let's have it merged.
I've not faced with such an issue before after importing 1k packages,
maybe it's just specific for some particular ones.

I've got go-build-system modifications pending to review,
improving test coverage for subdirs and relaxing build noise.

Your patches are LGFM.

Thanks,
Oleg

[-- Attachment #2: Type: text/html, Size: 643 bytes --]