[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Attila Lendvai]

---

this will be a series of patches that were needed to be able to
(mostly) successfully run these two imports (go-ethereum and
ethersphere/bee):

RUN=12
clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethersphere/bee@v1.5.1 > >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.log >&2)

RUN=36
clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethereum/go-ethereum@v1.10.17 > >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.log >&2)

note that i only have a mediocre knowledge of the golang
infrastructure, so this should be reviewed by someone who
more deeply understands the golang build process, and its
implementation within guix.

i think most of it is not very controversial, maybe except
the last commit.

i'm willing to reshape this under the guidance of someone who
has a better bird's eye view perspective on this all.

 guix/scripts/import.scm | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/guix/scripts/import.scm b/guix/scripts/import.scm
index 40fa6759ae..891f558b97 100644
--- a/guix/scripts/import.scm
+++ b/guix/scripts/import.scm
@@ -127,10 +127,14 @@ (define-command (guix-import . args)
                             ('define-public _ ...)))
               (print expr))
              ((? list? expressions)
-              (for-each (lambda (expr)
-                          (print expr)
-                          (newline))
-                        expressions))
+              (let ((count 0))
+                (for-each (lambda (expr)
+                            (print expr)
+                            (set! count (1+ count))
+                            (newline))
+                          expressions)
+                (format (current-warning-port)
+                        (G_ "Imported ~a packages~%") count)))
              (x
               (leave (G_ "'~a' import failed~%") importer))))
          (let ((hint (string-closest importer importers #:threshold 3)))
-- 
2.35.1

[bug#55242] [PATCH 02/10] guix: import: go: Rename go.pkg.dev-info to pkg.go.dev-info.

[Attila Lendvai]

To properly mimic the name of the website. Also employ it at one more place.
---
 guix/import/go.scm | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index d00c13475a..bb4bb7bb7b 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -120,8 +120,11 @@ (define (escape occurrence)
 ;; Prevent inlining of this procedure, which is accessed by unit tests.
 (set! go-path-escape go-path-escape)
 
-(define (go.pkg.dev-info name)
-  (http-fetch* (string-append "https://pkg.go.dev/" name)))
+(define (pkg.go.dev-info module-path)
+  ;; FIXME if MODULE-PATH is a fork on github, then it will try to get the
+  ;; info of the fork (instead of the upstream), which most probably does not
+  ;; exist in pkg.go.dev.  Example: https://github.com/bonitoo-io/oapi-codegen
+  (http-fetch* (string-append "https://pkg.go.dev/" module-path)))
 
 (define* (go-module-version-string goproxy name #:key version)
   "Fetch the version string of the latest version for NAME from the given
@@ -147,7 +150,7 @@ (define* (go-module-available-versions goproxy name)
 (define (go-package-licenses name)
   "Retrieve the list of licenses that apply to NAME, a Go package or module
 name (e.g. \"github.com/golang/protobuf/proto\")."
-  (let* ((body (go.pkg.dev-info (string-append name "?tab=licenses")))
+  (let* ((body (pkg.go.dev-info (string-append name "?tab=licenses")))
          ;; Extract the text contained in a h2 child node of any
          ;; element marked with a "License" class attribute.
          (select (sxpath `(// (* (@ (equal? (class "License"))))
@@ -169,7 +172,7 @@ (define (sxml->texi sxml-node)
 (define (go-package-description name)
   "Retrieve a short description for NAME, a Go package name,
 e.g. \"google.golang.org/protobuf/proto\"."
-  (let* ((body (go.pkg.dev-info name))
+  (let* ((body (pkg.go.dev-info name))
          (sxml (html->sxml body #:strict? #t))
          (overview ((sxpath
                      `(//
@@ -206,8 +209,7 @@ (define (go-package-synopsis module-name)
 the https://pkg.go.dev/ web site."
   ;; Note: Only the *module* (rather than package) page has the README title
   ;; used as a synopsis on the https://pkg.go.dev web site.
-  (let* ((url (string-append "https://pkg.go.dev/" module-name))
-         (body (http-fetch* url))
+  (let* ((body (pkg.go.dev-info module-name))
          ;; Extract the text contained in a h2 child node of any
          ;; element marked with a "License" class attribute.
          (select-title (sxpath
-- 
2.35.1

[bug#55242] [PATCH 03/10] guix: import: go: Add mockup logging facility.

[Attila Lendvai]

Introduce a (local) mockup logger, so that we don't need to keep adding and
deleting format's when debugging the codebase.

* guix/import/go.scm (log.info) (log.debug): New macros.
---
 guix/import/go.scm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index bb4bb7bb7b..0af5e4b5e2 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -100,6 +100,17 @@ (define-module (guix import go)
 
 ;;; Code:
 
+;; FIXME set up logging for the entire project, and replace this poor man's
+;; logger with the proper one.
+(define-syntax-rule (log.info format-string ...)
+  (let ((port (current-warning-port)))
+    (format port format-string ...)
+    (newline port)))
+
+(define-syntax-rule (log.debug format-string ...)
+  ;;(log.info format-string ...)
+  '())
+
 (define http-fetch*
   ;; Like http-fetch, but memoized and returning the body as a string.
   (memoize (lambda args
-- 
2.35.1

[bug#55242] [PATCH 04/10] guix: import: go: Fix the interpretation of the replace directive.

[Attila Lendvai]

When a version is specified in a replace directive, then we must also match
the version against any potential replacements.

* guix/import/go.scm (go.mod-requirements): Fix by also matching the version.
---
 guix/import/go.scm | 53 +++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 0af5e4b5e2..a115c61adc 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -331,7 +331,7 @@ (define-peg-pattern original all (or (and module-path version) module-path))
   (define-peg-pattern with all (or (and module-path version) file-path))
   (define-peg-pattern replace all (and original => with EOL))
   (define-peg-pattern replace-top body
-    (and (ignore "replace") 
+    (and (ignore "replace")
          (or (and block-start (* (or replace block-line)) block-end) replace)))
 
   ;; RetractSpec = ( Version | "[" Version "," Version "]" ) newline .
@@ -365,22 +365,39 @@ (define (go.mod-directives go.mod directive)
 (define (go.mod-requirements go.mod)
   "Compute and return the list of requirements specified by GO.MOD."
   (define (replace directive requirements)
-    (define (maybe-replace module-path new-requirement)
-      ;; Do not allow version updates for indirect dependencies (see:
-      ;; https://golang.org/ref/mod#go-mod-file-replace).
-      (if (and (equal? module-path (first new-requirement))
-               (not (assoc-ref requirements module-path)))
-          requirements
-          (cons new-requirement (alist-delete module-path requirements))))
-
+    ;; Specification: https://golang.org/ref/mod#go-mod-file-replace
+    ;; Interesting test cases:
+    ;;   github.com/influxdata/influxdb-client-go/v2@v2.4.0
+    (log.debug "inside replace, directive ~S, requirements ~S" directive requirements)
+    (define (maybe-replace old-module-path old-version new-module-path new-version)
+      (let ((matched-entry (assoc-ref requirements old-module-path)))
+        (log.debug "inside maybe-replace, ~S ~S => ~S ~S, matched-entry ~S"
+                   old-module-path old-version new-module-path new-version matched-entry)
+        (cond ((and (equal? old-module-path new-module-path)
+                    (not matched-entry))
+               ;; "A replace directive has no effect if the module version on the left
+               ;; side is not required."
+               ;; Do not allow version updates for indirect dependencies.
+               requirements)
+              ((and matched-entry
+                    (or (not old-version)
+                        (equal? old-version new-version)))
+               (cons (list new-module-path new-version)
+                     (alist-delete old-module-path requirements)))
+              (else
+               requirements))))
+    (log.debug "toplevel directive is ~S" directive)
     (match directive
-      ((('original ('module-path module-path) . _) with . _)
+      ((('original ('module-path old-module-path) ('version old-version) ...) with)
+       (unless (null? old-version)
+         (set! old-version (first old-version)))
        (match with
-         (('with ('file-path _) . _)
-          (alist-delete module-path requirements))
-         (('with ('module-path new-module-path) ('version new-version) . _)
-          (maybe-replace module-path
-                         (list new-module-path new-version)))))))
+         (('with ('file-path _))
+          ;; Superseded by a module in a local path, so let's delete it.
+          (alist-delete old-module-path requirements))
+         (('with ('module-path new-module-path) ('version new-version))
+          (maybe-replace old-module-path old-version
+                         new-module-path new-version))))))
 
   (define (require directive requirements)
     (match directive
@@ -389,8 +406,10 @@ (define (require directive requirements)
 
   (let* ((requires (go.mod-directives go.mod 'require))
          (replaces (go.mod-directives go.mod 'replace))
-         (requirements (fold require '() requires)))
-    (fold replace requirements replaces)))
+         (requirements (fold require '() requires))
+         (result (fold replace requirements replaces)))
+    (log.debug "requires:~% ~S~%replaces:~% ~S~%result:~% ~S" requires replaces result)
+    result))
 
 ;; Prevent inlining of this procedure, which is accessed by unit tests.
 (set! go.mod-requirements go.mod-requirements)
-- 
2.35.1

[bug#55242] [PATCH 05/10] guix: import: go: Harden sxml->texi conversion.

[Attila Lendvai]

* guix/import/go.scm (sxml->texi): Escape @ as @@, and only emit the text part
of URLs if they are plain strings.
---
 guix/import/go.scm | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index a115c61adc..6b0fbaa8b6 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -172,13 +172,26 @@ (define (sxml->texi sxml-node)
   "A very basic SXML to Texinfo converter which attempts to preserve HTML
 formatting and links as text."
   (sxml-match sxml-node
-              ((strong ,text)
-               (format #f "@strong{~a}" text))
-              ((a (@ (href ,url)) ,text)
-               (format #f "@url{~a,~a}" url text))
-              ((code ,text)
-               (format #f "@code{~a}" text))
-              (,something-else something-else)))
+    ((strong ,text)
+     (format #f "@strong{~a}" text))
+    ((a (@ (href ,url)) ,body)
+     ;; Examples: image in the url: github.com/go-openapi/jsonpointer
+     ;; (code ...) in the URL body: github.com/mwitkow/go-conntrack
+     (if (string? body)
+         (format #f "@url{~a,~a}" url body)
+         (sxml-match body
+                     ((code ,text)
+                      (format #f "@url{~a,~a}" url (sxml->texi body)))
+                     (,_
+                      (format #f "@url{~a}" url)))))
+    ((code ,text)
+     (format #f "@code{~a}" text))
+    (,something-else
+     ;; Example: @ in the description: github.com/ethersphere/langos
+     (if (string? something-else)
+         (string-replace-substring something-else
+                                   "@" "@@")
+         something-else))))
 
 (define (go-package-description name)
   "Retrieve a short description for NAME, a Go package name,
-- 
2.35.1

[bug#55242] [PATCH 06/10] guix: import: go: Add a local duplicate of http-fetch.

[Attila Lendvai]

This is needed when dealing with golang packages, as per:
https://golang.org/ref/mod#vcs-find

A page may return 404, but at the same time also contain the sought after
`go-import` meta tag.  An example for such a project/page is:
https://www.gonum.org/v1/gonum?go-get=1

It's not enough to just handle the thrown exception, because we need to be
able to get hold of the fetched content, too.

Discussion why it's duplicated here: https://issues.guix.gnu.org/54836

* guix/import/go.scm (http-fetch): Add a copy and extend it with the
accept-all-response-codes? param.
---
 guix/import/go.scm | 107 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 6b0fbaa8b6..a08005d090 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -33,13 +33,22 @@ (define-module (guix import go)
   #:use-module (guix import json)
   #:use-module (guix packages)
   #:use-module ((guix utils) #:select (string-replace-substring))
-  #:use-module (guix http-client)
+  ;; FIXME? We use a local copy of http-fetch.
+  ;; See https://issues.guix.gnu.org/54836
+  #:use-module ((guix http-client) #:hide (http-fetch))
+  #:use-module (guix base64)
+  #:use-module (rnrs bytevectors)
+  #:use-module ((guix build download)
+                #:select (open-socket-for-uri
+                          (open-connection-for-uri
+                           . guix:open-connection-for-uri)
+                          resolve-uri-reference))
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix memoization)
   #:autoload   (htmlprag) (html->sxml)            ;from Guile-Lib
   #:autoload   (guix serialization) (write-file)
   #:autoload   (guix base32) (bytevector->nix-base32-string)
-  #:autoload   (guix build utils) (mkdir-p)
+  #:autoload   (guix build utils) (mkdir-p dump-port)
   #:autoload   (gcrypt hash) (hash-algorithm sha256)
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
@@ -69,6 +78,100 @@ (define-module (guix import go)
             go-module->guix-package*
             go-module-recursive-import))
 
+;; This is a duplicate from (guix http-client) with the addition of a
+;; #:accept-all-response-codes? param. See https://issues.guix.gnu.org/54836
+(define* (http-fetch uri #:key port (text? #f) (buffered? #t)
+                     (open-connection guix:open-connection-for-uri)
+                     (keep-alive? #f)
+                     (verify-certificate? #t)
+                     (headers '((user-agent . "GNU Guile")))
+                     (log-port (current-error-port))
+                     timeout
+                     (accept-all-response-codes? #f))
+  "Return an input port containing the data at URI, and the expected number of
+bytes available or #f.  If TEXT? is true, the data at URI is considered to be
+textual.  Follow any HTTP redirection.  When BUFFERED? is #f, return an
+unbuffered port, suitable for use in `filtered-port'.  HEADERS is an alist of
+extra HTTP headers.
+
+When KEEP-ALIVE? is true, the connection is marked as 'keep-alive' and PORT is
+not closed upon completion.
+
+When VERIFY-CERTIFICATE? is true, verify HTTPS server certificates.
+
+TIMEOUT specifies the timeout in seconds for connection establishment; when
+TIMEOUT is #f, connection establishment never times out.
+
+Write information about redirects to LOG-PORT.
+
+When ACCEPT-ALL-RESPONSE-CODES? is false then raise an '&http-get-error'
+condition if downloading fails, otherwise return the response regardless
+of the reponse code."
+  (define parsed-initial-uri
+    (if (string? uri) (string->uri uri) uri))
+
+  (define (open-connection* uri)
+    (open-connection uri
+                     #:verify-certificate? verify-certificate?
+                     #:timeout timeout))
+
+  (let loop ((current-uri parsed-initial-uri)
+             (current-port (or port (open-connection parsed-initial-uri))))
+    (let ((headers (match (uri-userinfo current-uri)
+                     ((? string? str)
+                      (cons (cons 'Authorization
+                                  (string-append "Basic "
+                                                 (base64-encode
+                                                  (string->utf8 str))))
+                            headers))
+                     (_ headers))))
+      (unless (or buffered? (not (file-port? current-port)))
+        (setvbuf current-port 'none))
+      (let*-values (((resp data)
+                     (http-get current-uri #:streaming? #t #:port current-port
+                               #:keep-alive? keep-alive?
+                               #:headers headers))
+                    ((code)
+                     (response-code resp)))
+        (case code
+          ((200)
+           (values data (response-content-length resp)))
+          ((301                         ; moved permanently
+            302                         ; found (redirection)
+            303                         ; see other
+            307                         ; temporary redirection
+            308)                        ; permanent redirection
+           (let ((host (uri-host current-uri))
+                 (new-uri (resolve-uri-reference (response-location resp)
+                                                 current-uri)))
+             (if keep-alive?
+                 (dump-port data (%make-void-port "w0")
+                            (response-content-length resp))
+                 (close-port current-port))
+             (format log-port (G_ "following redirection to `~a'...~%")
+                     (uri->string new-uri))
+             (loop new-uri
+                   (or (and keep-alive?
+                            (or (not (uri-host new-uri))
+                                (string=? host (uri-host new-uri)))
+                            current-port)
+                       (open-connection* new-uri)))))
+          (else
+           (if accept-all-response-codes?
+               (values data (response-content-length resp))
+               (raise (condition (&http-get-error
+                                  (uri current-uri)
+                                  (code code)
+                                  (reason (response-reason-phrase resp))
+                                  (headers (response-headers resp)))
+                                 (&message
+                                  (message
+                                   (format
+                                    #f
+                                    (G_ "~a: HTTP download failed: ~a (~s)")
+                                    (uri->string current-uri) code
+                                    (response-reason-phrase resp)))))))))))))
+
 ;;; Commentary:
 ;;;
 ;;; (guix import go) attempts to make it easier to create Guix package
-- 
2.35.1

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Attila Lendvai]

This also adds logging statements to various parts of the go importer.

* guix/import/go.scm (go-module->guix-package*): Catch exceptions and retry in
case of network errors. Add ability to enable printing a full backtrace when
debugging.
(go-module->guix-package): Tolerate the inability to fetch the synopsis and
description.
---
 guix/import/go.scm | 104 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 86 insertions(+), 18 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index a08005d090..6871132a70 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -23,7 +23,15 @@
 ;;; You should have received a copy of the GNU General Public License
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
+;;; goproxy protocol:
+;;;   https://golang.org/ref/mod#module-proxy
+;;;   https://docs.gomods.io/intro/protocol/
+;;;   https://roberto.selbach.ca/go-proxies/
+
 (define-module (guix import go)
+  #:use-module (git)
+  #:use-module (git structs)
+  #:use-module (git errors)
   #:use-module (guix build-system go)
   #:use-module (guix git)
   #:use-module (guix hash)
@@ -51,6 +59,8 @@ (define-module (guix import go)
   #:autoload   (guix build utils) (mkdir-p dump-port)
   #:autoload   (gcrypt hash) (hash-algorithm sha256)
   #:use-module (ice-9 format)
+  #:use-module (ice-9 control)
+  #:use-module (ice-9 exceptions)
   #:use-module (ice-9 match)
   #:use-module (ice-9 peg)
   #:use-module (ice-9 rdelim)
@@ -629,10 +639,14 @@ (define (go-import->module-meta content-text)
        (make-module-meta root-path (string->symbol vcs)
                          (strip-.git-suffix/maybe repo-url)))))
   ;; <meta name="go-import" content="import-prefix vcs repo-root">
-  (let* ((meta-data (http-fetch* (format #f "https://~a?go-get=1" module-path)))
+  (let* ((url (format #f "https://~a?go-get=1" module-path))
+         (meta-data (http-fetch* url #:accept-all-response-codes? #true))
          (select (sxpath `(// (meta (@ (equal? (name "go-import"))))
-                              // content))))
-    (match (select (html->sxml meta-data #:strict? #t))
+                              // content)))
+         (sxml (html->sxml meta-data #:strict? #t))
+         (selected (select sxml)))
+    (log.debug "The fetched meta-data from ~A is:~%~S" url selected)
+    (match selected
       (() #f)                           ;nothing selected
       ((('content content-text) ..1)
        (or
@@ -809,22 +823,76 @@ (define* (go-module->guix-package module-path #:key
          dependencies+versions
          dependencies))))
 
-(define go-module->guix-package*
-  (lambda args
-    ;; Disable output buffering so that the following warning gets printed
-    ;; consistently.
-    (setvbuf (current-error-port) 'none)
-    (let ((package-name (match args ((name _ ...) name))))
-      (guard (c ((http-get-error? c)
-                 (warning (G_ "Failed to import package ~s.
-reason: ~s could not be fetched: HTTP error ~a (~s).
+(define (go-module->guix-package* . args)
+  ;; Disable output buffering so that the following warning gets printed
+  ;; consistently.
+  (setvbuf (current-error-port) 'none)
+  (setvbuf (current-warning-port) 'none)
+  (let* ((package-name (match args ((name _ ...) name)))
+         ;; Use report-all-errors? for debugging purposes only, because
+         ;; e.g. getaddrinfo is not reentrant and therefore we must unwind
+         ;; before retrying.
+         (report-all-errors? #false)
+         (report-network-error
+          (lambda (reason)
+            (warning (G_ "Failing to import package ~S.
+reason: ~A.~%")
+                     package-name reason))))
+    (let loop ((attempts 0))
+      (when (> attempts 0)
+        (sleep 3)
+        (log.info "~%Retrying, attempt ~s." attempts))
+      (cond
+       ((> attempts 60)
+        (warning (G_ "Giving up on importing package ~s.
 This package and its dependencies won't be imported.~%")
-                          package-name
-                          (uri->string (http-get-error-uri c))
-                          (http-get-error-code c)
-                          (http-get-error-reason c))
-                 (values #f '())))
-        (apply go-module->guix-package args)))))
+                 package-name)
+        (values #f '()))
+       (else
+        (guard (c ((http-get-error? c)
+                   (report-network-error
+                    (format #f "~s could not be fetched: HTTP error ~a (~s)"
+                            (uri->string (http-get-error-uri c))
+                            (http-get-error-code c)
+                            (http-get-error-reason c)))
+                   (loop (+ 1 attempts)))
+                  ((eq? (exception-kind c)
+                        'getaddrinfo-error)
+                   (report-network-error "DNS lookup failed")
+                   (loop (+ 1 attempts)))
+                  ((and (eq? (exception-kind c)
+                             'git-error)
+                        (eq? (git-error-class (first (exception-args c)))
+                             GITERR_NET))
+                   (report-network-error "network error coming from git")
+                   (loop (+ 1 attempts)))
+                  (else
+                   (let ((port (current-warning-port)))
+                     (format port "Unexpected error, will skip ~S.~%reason: "
+                             package-name)
+                     ;; Printing a backtrace here is not very useful: it is
+                     ;; cut off because GUARD unwinds.
+                     (print-exception port (stack-ref (make-stack #t) 1)
+                                      c (exception-args c))
+                     (display-backtrace (make-stack #t) port))
+                   ;; give up on this entry
+                   (values #f '())))
+          (with-exception-handler
+              (lambda (c)
+                (when report-all-errors?
+                  (let ((port (current-warning-port)))
+                    (format port "*** exception while importing:~%")
+                    (print-exception port (stack-ref (make-stack #t) 1)
+                                     c (exception-args c))
+                    (format port "*** printing backtrace:~%")
+                    (display-backtrace (make-stack #t) port)
+                    ;; DISPLAY-BACKTRACE can fail, so it's better to make its
+                    ;; exit also visible.
+                    (format port "*** done printing backtrace~%")))
+                (raise-continuable c))
+            (lambda ()
+              (apply go-module->guix-package args))
+            #:unwind? (not report-all-errors?))))))))
 
 (define* (go-module-recursive-import package-name
                                      #:key (goproxy "https://proxy.golang.org")
-- 
2.35.1

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Attila Lendvai]

Handle golang projects that are located in a subdir relative to the repo's
root directory, and whose tags are also prefixed accordingly with the relative
path.

An example: guix import go github.com/aws/aws-sdk-go-v2/service/route53@v1.1.1

Also be more resilient while dealing with licences.

* guix/import/go.scm (go-module->guix-package): Attempt to split comma
separated lists of licences, and tolerate the inability to fetch any license
information.  Handle go modules that are a subdirectory of a vcs repo,
including the special naming of their tags (it includes the subdir as a prefix).
(list->licenses): Warn when the conversion fails.
(+known-vcs+): Renamed from known-vcs to use naming convention for constants.
---
 guix/import/go.scm | 130 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 102 insertions(+), 28 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 6871132a70..ce6463cc51 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -346,6 +346,7 @@ (define (go-package-synopsis module-name)
 the https://pkg.go.dev/ web site."
   ;; Note: Only the *module* (rather than package) page has the README title
   ;; used as a synopsis on the https://pkg.go.dev web site.
+  (log.debug "Getting synopsis for ~S" module-name)
   (let* ((body (pkg.go.dev-info module-name))
          ;; Extract the text contained in a h2 child node of any
          ;; element marked with a "License" class attribute.
@@ -377,7 +378,12 @@ (define (list->licenses licenses)
                             ("GPL3" "GPL-3.0")
                             ("NIST" "NIST-PD")
                             (_ license)))
-                         'unknown-license!)))
+                         (begin
+                           (warning (G_ "Failed to identify license ~S.~%")
+                                    license)
+                           ;; This will put the license there as a string that
+                           ;; will error at compilation.
+                           license))))
               licenses))
 
 (define (fetch-go.mod goproxy module-path version)
@@ -550,7 +556,8 @@ (define-record-type <vcs>
 (define (make-vcs prefix regexp type)
   (%make-vcs prefix (make-regexp regexp) type))
 
-(define known-vcs
+;; TODO use define-constant
+(define +known-vcs+
   ;; See the following URL for the official Go equivalent:
   ;; https://github.com/golang/go/blob/846dce9d05f19a1f53465e62a304dea21b99f910/src/cmd/go/internal/vcs/vcs.go#L1026-L1087
   (list
@@ -587,16 +594,17 @@ (define vcs-qualifiers '(".bzr" ".fossil" ".git" ".hg" ".svn"))
 
   (define (vcs-qualified-module-path->root-repo-url module-path)
     (let* ((vcs-qualifiers-group (string-join vcs-qualifiers "|"))
-           (pattern (format #f "^(.*(~a))(/|$)" vcs-qualifiers-group))
-           (m (string-match pattern module-path)))
-      (and=> m (cut match:substring <> 1))))
+           (pattern (format #f "^(.*(~a))(/|$)" vcs-qualifiers-group)))
+      (and=> (string-match pattern module-path)
+             (cut match:substring <> 1))))
 
   (or (and=> (find (lambda (vcs)
                      (string-prefix? (vcs-url-prefix vcs) module-path))
-                   known-vcs)
+                   +known-vcs+)
              (lambda (vcs)
                (match:substring (regexp-exec (vcs-root-regex vcs)
-                                             module-path) 1)))
+                                             module-path)
+                                1)))
       (vcs-qualified-module-path->root-repo-url module-path)
       module-path))
 
@@ -666,6 +674,7 @@ (define (module-meta-data-repo-url meta-data goproxy)
 (define* (git-checkout-hash url reference algorithm)
   "Return the ALGORITHM hash of the checkout of URL at REFERENCE, a commit or
 tag."
+  (log.info "Fetching git repo at ~S, reference ~S" url reference)
   (define cache
     (string-append (or (getenv "TMPDIR") "/tmp")
                    "/guix-import-go-"
@@ -681,6 +690,7 @@ (define cache
                   (update-cached-checkout url
                                           #:ref
                                           `(tag-or-commit . ,reference)))))
+    (log.debug " hashing at checkout ~S, commit ~S, reference ~S" checkout commit reference)
     (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
 
 (define (vcs->origin vcs-type vcs-repo-url version)
@@ -688,8 +698,10 @@ (define (vcs->origin vcs-type vcs-repo-url version)
 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* ((git-ref        (go-version->git-ref version))
+            (plain-version? (string=? version git-ref))
+            (v-prefixed?    (string-prefix? "v" version)))
+       (log.debug "Version ~S converted to git reference ~S" version git-ref)
        `(origin
           (method git-fetch)
           (uri (git-reference
@@ -699,13 +711,12 @@ (define (vcs->origin vcs-type vcs-repo-url version)
                 ;; stripped of any 'v' prefixed.
                 (commit ,(if (and plain-version? v-prefixed?)
                              '(string-append "v" version)
-                             '(go-version->git-ref version)))))
+                             git-ref))))
           (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))))))))
+              (git-checkout-hash vcs-repo-url git-ref (hash-algorithm sha256))))))))
     ((hg)
      `(origin
         (method hg-fetch)
@@ -768,41 +779,104 @@ (define* (go-module->guix-package module-path #:key
   "Return the package S-expression corresponding to MODULE-PATH at VERSION, a Go package.
 The meta-data is fetched from the GOPROXY server and https://pkg.go.dev/.
 When VERSION is unspecified, the latest version available is used."
+  (log.info "~%Processing go module ~A@~A" module-path version)
   (let* ((available-versions (go-module-available-versions goproxy module-path))
          (version* (validate-version
                     (or (and version (ensure-v-prefix version))
                         (go-module-version-string goproxy module-path)) ;latest
                     available-versions
                     module-path))
-         (content (fetch-go.mod goproxy module-path version*))
-         (dependencies+versions (go.mod-requirements (parse-go.mod content)))
+         (go.mod (fetch-go.mod goproxy module-path version*))
+         (dependencies+versions (go.mod-requirements (parse-go.mod go.mod)))
          (dependencies (if pin-versions?
                            dependencies+versions
                            (map car dependencies+versions)))
-         (module-path-sans-suffix
-          (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))
+         (repo-root (module-path->repository-root module-path))
          ;; 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))
+         (meta-data (fetch-module-meta-data repo-root))
+         (module-root-path (module-meta-import-prefix meta-data))
          (vcs-type (module-meta-vcs meta-data))
          (vcs-repo-url (module-meta-data-repo-url meta-data goproxy))
-         (synopsis (go-package-synopsis module-path))
-         (description (go-package-description module-path))
-         (licenses (go-package-licenses module-path)))
+         (raw-subdir (if (< (string-length module-root-path)
+                            (string-length module-path))
+                         (substring module-path
+                                    (+ 1 (string-length module-root-path)))
+                         #false))
+         (module-subdirectory raw-subdir)
+         (major-version (and=>
+                         (and raw-subdir
+                              (string-match ".*v([0-9]+)$" raw-subdir))
+                         (lambda (m)
+                           (let* ((v-postfix (match:substring m 1))
+                                  (ver (string->number v-postfix)))
+                             (log.debug " ~A matched as having a version-postfix: ~A"
+                                        raw-subdir v-postfix)
+                             (set! module-subdirectory
+                                   (substring raw-subdir 0
+                                              ;; Don't go negative when
+                                              ;; raw-subdir is a simple "v2".
+                                              (max 0
+                                                   (- (string-length raw-subdir)
+                                                      (string-length v-postfix)
+                                                      ;; Drop two slashes.
+                                                      2))))
+                             (when (string-null? module-subdirectory)
+                               (set! module-subdirectory #false))
+                             (unless (integer? ver)
+                               (raise
+                                (formatted-message (G_ "failed to parse version postfix from '~a' for package '~a'")
+                                                   raw-subdir module-path)))
+                             ;; TODO assert that major-version matches the first number in version
+                             ;; TODO assert that only version 1.x.y is allowed without a /v[major-version] postfix
+                             ver))))
+         (vcs-tag (if module-subdirectory
+                      (string-append module-subdirectory "/" version*)
+                      version*))
+         (synopsis (or (false-if-exception
+                        (go-package-synopsis module-path))
+                       (begin
+                         (warning (G_ "Failed to fetch synopsis for ~S.~%")
+                                  module-path)
+                         "TODO FIXME")))
+         (description (or (false-if-exception
+                           (go-package-description module-path))
+                          (begin
+                            (warning (G_ "Failed to fetch description for ~S.~%")
+                                     module-path)
+                            "TODO FIXME")))
+         (licenses (or (false-if-exception
+                        (go-package-licenses module-path))
+                       (begin
+                         (warning (G_ "Failed to fetch license for ~S.~%")
+                                  module-path)
+                         '("unknown-license!")))))
+    ;; Maybe split comma separated list of licenses in a single string
+    (when (and (= 1 (length licenses))
+               (string? (first licenses)))
+      (let ((pieces (map string-trim-both
+                         (remove! string-null?
+                                  (string-split (first licenses) #\,)))))
+        (when (< 1 (length pieces))
+          (set! licenses pieces))))
+    (log.debug " meta-data: ~S~% raw-subdir: ~S, module-subdirectory: ~S, \
+major-version: ~S"
+               meta-data raw-subdir module-subdirectory major-version)
+    (log.debug " dependencies:~%~S" dependencies+versions)
     (values
      `(package
         (name ,guix-name)
         (version ,(strip-v-prefix version*))
         (source
-         ,(vcs->origin vcs-type vcs-repo-url version*))
+         ,(vcs->origin vcs-type vcs-repo-url vcs-tag))
         (build-system go-build-system)
         (arguments
-         '(#:import-path ,module-path
-           ,@(if (string=? module-path-sans-suffix root-module-path)
-                 '()
-                 `(#:unpack-path ,root-module-path))))
+         '(#:tests? #false ; some packages have unrecorded dependencies needed only by their tests
+           #:import-path ,module-path
+           ,@(if module-subdirectory
+                 `(#:unpack-path ,module-root-path)
+                 '())))
         ,@(maybe-propagated-inputs
            (map (match-lambda
                   ((name version)
@@ -810,7 +884,7 @@ (define* (go-module->guix-package module-path #:key
                   (name
                    (go-module->guix-package-name name)))
                 dependencies))
-        (home-page ,(format #f "https://~a" root-module-path))
+        (home-page ,(format #f "https://~a" module-root-path))
         (synopsis ,synopsis)
         (description ,(and=> description beautify-description))
         (license ,(match (list->licenses licenses)
@@ -898,7 +972,7 @@ (define* (go-module-recursive-import package-name
                                      #:key (goproxy "https://proxy.golang.org")
                                      version
                                      pin-versions?)
-
+  (log.info "Initiating recursive import of ~a, version ~a" package-name version)
   (recursive-import
    package-name
    #:repo->guix-package
-- 
2.35.1

[bug#55242] [PATCH 09/10] guix: import: go: module-name -> module-path to be consistent

[Attila Lendvai]

* guix/import/go.scm (go-package-description): Renamed first parameter.
---
 guix/import/go.scm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index ce6463cc51..25d424c1ac 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -340,14 +340,14 @@ (define (go-package-description name)
       ((p elements ...)
        (apply string-append (filter string? (map sxml->texi elements)))))))
 
-(define (go-package-synopsis module-name)
-  "Retrieve a short synopsis for a Go module named MODULE-NAME,
+(define (go-package-synopsis module-path)
+  "Retrieve a short synopsis for a Go module named MODULE-PATH,
 e.g. \"google.golang.org/protobuf\".  The data is scraped from
 the https://pkg.go.dev/ web site."
   ;; Note: Only the *module* (rather than package) page has the README title
   ;; used as a synopsis on the https://pkg.go.dev web site.
-  (log.debug "Getting synopsis for ~S" module-name)
-  (let* ((body (pkg.go.dev-info module-name))
+  (log.debug "Getting synopsis for ~S" module-path)
+  (let* ((body (pkg.go.dev-info module-path))
          ;; Extract the text contained in a h2 child node of any
          ;; element marked with a "License" class attribute.
          (select-title (sxpath
-- 
2.35.1

[bug#55242] [PATCH 10/10] guix: import: go: Better handling of /v2 in the module path.

[Attila Lendvai]

* guix/import/go.scm (module-path->repository-root): Delete version param.
(*module-path->import-path*): New mapping, initialized with a small db of
projects.
(go-module->guix-package): Return packages with #:unpack-path when needed.
---
 guix/import/go.scm | 93 +++++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 39 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 25d424c1ac..fbed3ca88b 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -75,6 +75,7 @@ (define-module (guix import go)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
+  #:use-module (srfi srfi-71) ; let*
   #:use-module (sxml match)
   #:use-module ((sxml xpath) #:renamer (lambda (s)
                                          (if (eq? 'filter s)
@@ -608,19 +609,15 @@ (define (vcs-qualified-module-path->root-repo-url module-path)
       (vcs-qualified-module-path->root-repo-url module-path)
       module-path))
 
-(define* (go-module->guix-package-name module-path #:optional version)
-  "Converts a module's path to the canonical Guix format for Go packages.
-Optionally include a VERSION string to append to the name."
+(define* (go-module->guix-package-name module-path)
+  "Converts a module's path to the canonical Guix format for Go packages."
   ;; Map dot, slash, underscore and tilde characters to hyphens.
   (let ((module-path* (string-map (lambda (c)
                                     (if (member c '(#\. #\/ #\_ #\~))
                                         #\-
                                         c))
                                   module-path)))
-    (string-downcase (string-append "go-" module-path*
-                                    (if version
-                                        (string-append "-" version)
-                                        "")))))
+    (string-downcase (string-append "go-" module-path*))))
 
 (define (strip-.git-suffix/maybe repo-url)
   "Strip a repository URL '.git' suffix from REPO-URL if hosted at GitHub."
@@ -771,6 +768,14 @@ (define (validate-version version available-versions module-path)
 available versions:~{ ~a~}.")
                                   (map strip-v-prefix
                                        available-versions)))))))))
+(define *module-path->import-path*
+  ;; There's no other way to derive this information, at least that I know of.
+  '(("github.com/google/go-cmp" . "github.com/google/go-cmp/cmp")
+    ("github.com/sergi/go-diff" . "github.com/sergi/go-diff/diffmatchpatch")
+    ("github.com/davecgh/go-spew" . "github.com/davecgh/go-spew/spew")
+    ("github.com/beorn7/perks" . "github.com/beorn7/perks/quantile")
+    ("github.com/census-instrumentation/opencensus-proto" .
+     "github.com/census-instrumentation/opencensus-proto/gen-go")))
 
 (define* (go-module->guix-package module-path #:key
                                   (goproxy "https://proxy.golang.org")
@@ -864,38 +869,48 @@ (define* (go-module->guix-package module-path #:key
 major-version: ~S"
                meta-data raw-subdir module-subdirectory major-version)
     (log.debug " dependencies:~%~S" dependencies+versions)
-    (values
-     `(package
-        (name ,guix-name)
-        (version ,(strip-v-prefix version*))
-        (source
-         ,(vcs->origin vcs-type vcs-repo-url vcs-tag))
-        (build-system go-build-system)
-        (arguments
-         '(#:tests? #false ; some packages have unrecorded dependencies needed only by their tests
-           #:import-path ,module-path
-           ,@(if module-subdirectory
-                 `(#:unpack-path ,module-root-path)
-                 '())))
-        ,@(maybe-propagated-inputs
-           (map (match-lambda
-                  ((name version)
-                   (go-module->guix-package-name name (strip-v-prefix version)))
-                  (name
-                   (go-module->guix-package-name name)))
-                dependencies))
-        (home-page ,(format #f "https://~a" module-root-path))
-        (synopsis ,synopsis)
-        (description ,(and=> description beautify-description))
-        (license ,(match (list->licenses licenses)
-                    (() #f)                       ;unknown license
-                    ((license)                    ;a single license
-                     license)
-                    ((license ...)                ;a list of licenses
-                     `(list ,@license)))))
-     (if pin-versions?
-         dependencies+versions
-         dependencies))))
+    (let* ((import-path unpack-path
+            (if module-subdirectory
+                (values module-path module-root-path)
+                (let ((import-path (assoc-ref *module-path->import-path*
+                                              module-path)))
+                  (if import-path
+                      (begin
+                        (log.debug "matched as an import-path exception: ~S" import-path)
+                        (values import-path module-path))
+                      (values module-path #f))))))
+      (values
+       `(package
+          (name ,guix-name)
+          (version ,(strip-v-prefix version*))
+          (source
+           ,(vcs->origin vcs-type vcs-repo-url vcs-tag))
+          (build-system go-build-system)
+          (arguments
+           '(#:import-path ,import-path
+             ,@(if unpack-path
+                   `(#:unpack-path ,unpack-path)
+                   '())))
+          ,@(maybe-propagated-inputs
+             (map (match-lambda
+                    ((name version)
+                     (list (go-module->guix-package-name name)
+                           (strip-v-prefix version)))
+                    (name
+                     (go-module->guix-package-name name)))
+                  dependencies))
+          (home-page ,(format #f "https://~a" module-root-path))
+          (synopsis ,synopsis)
+          (description ,(and=> description beautify-description))
+          (license ,(match (list->licenses licenses)
+                      (() #f)                       ;unknown license
+                      ((license)                    ;a single license
+                       license)
+                      ((license ...)                ;a list of licenses
+                       `(list ,@license)))))
+       (if pin-versions?
+           dependencies+versions
+           dependencies)))))
 
 (define (go-module->guix-package* . args)
   ;; Disable output buffering so that the following warning gets printed
-- 
2.35.1

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +                   (let ((port (current-warning-port)))
> +                     (format port "Unexpected error, will skip ~S.~%reason: "
> +                             package-name)
> +                     ;; Printing a backtrace here is not very useful: it is
> +                     ;; cut off because GUARD unwinds.
> +                     (print-exception port (stack-ref (make-stack #t) 1)
> +                                      c (exception-args c))
> +                     (display-backtrace (make-stack #t) port))

Why are unknown errors being catched here?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +          (with-exception-handler
> +              (lambda (c)
> +                (when report-all-errors?
> +                  (let ((port (current-warning-port)))
> +                    (format port "*** exception while importing:~%")
> +                    (print-exception port (stack-ref (make-stack #t) 1)
> +                                     c (exception-args c))
> +                    (format port "*** printing backtrace:~%")
> +                    (display-backtrace (make-stack #t) port)
> +                    ;; DISPLAY-BACKTRACE can fail, so it's better to make its
> +                    ;; exit also visible.
> +                    (format port "*** done printing backtrace~%")))
> +                (raise-continuable c))

What's the '-continuable' for here?  Is it to avoid extra 'raise-
exception' entries in the backtrace, or does this actually make use of
Scheme's continuable exceptions?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:16 [+0200]:
> [bug#55242] [PATCH 01/10] guix: import: Print the number of packages
> at the end.
> +                (format (current-warning-port)
> +                        (G_ "Imported ~a packages~%") count)))

What's the use case for counting the number of packages?

Why only for the Go importer, and why a warning? 

The 'info' macro from (guix diagnostics) seems to fit here (it also
uses 'current-warning-port', but only internally).

The plural form of 'packages' is incorrect when count=1.

Greetings,
Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 02/10] guix: import: go: Rename go.pkg.dev-info to pkg.go.dev-info.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> github

Standard capitalisation is "GitHub" here, not that it really matters
much.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 03/10] guix: import: go: Add mockup logging facility.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +;; FIXME set up logging for the entire project, and replace this poor man's
> +;; logger with the proper one.
> +(define-syntax-rule (log.info format-string ...)
> +  (let ((port (current-warning-port)))
> +    (format port format-string ...)
> +    (newline port)))
> +
> +(define-syntax-rule (log.debug format-string ...)
> +  ;;(log.info format-string ...)
> +  '())

We already have a logging functionality: 'info' from
guix/diagnostics.scm.  No 'debug' yet though ...

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 04/10] guix: import: go: Fix the interpretation of the replace directive.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +    ;; Specification: https://golang.org/ref/mod#go-mod-file-replace
> +    ;; Interesting test cases:
> +    ;;   github.com/influxdata/influxdb-client-go/v2@v2.4.0
> +    (log.debug "inside replace, directive ~S, requirements ~S" directive requirements)

You can use 'pk' for this:

  #;(pk "inside replace, directive" directive "requirements" requirements)

(delete 'pk' when debugging).

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 04/10] guix: import: go: Fix the interpretation of the replace directive.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +    (define (maybe-replace old-module-path old-version new-module-path new-version)
> +      (let ((matched-entry (assoc-ref requirements old-module-path)))
> +        (log.debug "inside maybe-replace, ~S ~S => ~S ~S, matched-entry ~S"
> +                   old-module-path old-version new-module-path new-version matched-entry)
> +        (cond ((and (equal? old-module-path new-module-path)
> +                    (not matched-entry))
> +               ;; "A replace directive has no effect if the module version on the left
> +               ;; side is not required."
> +               ;; Do not allow version updates for indirect dependencies.
> +               requirements)
> +              ((and matched-entry
> +                    (or (not old-version)
> +                        (equal? old-version new-version)))
> +               (cons (list new-module-path new-version)
> +                     (alist-delete old-module-path requirements)))
> +              (else
> +               requirements))))

There's some extra complexity here, so I recommend some corresponding
extra tests, to avoid it breaking in the future.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 04/10] guix: import: go: Fix the interpretation of the replace directive.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +       (unless (null? old-version)

How can a version be a list?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 04/10] guix: import: go: Fix the interpretation of the replace directive.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +       (unless (null? old-version)
> +         (set! old-version (first old-version)))

Nevermind, there's some ...

Still, why choose the first old-version instead of the latest old-
version?  How can we know which one is correct?.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 05/10] guix: import: go: Harden sxml->texi conversion.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
>    (sxml-match sxml-node
> -              ((strong ,text)
> -               (format #f "@strong{~a}" text))
> -              ((a (@ (href ,url)) ,text)
> -               (format #f "@url{~a,~a}" url text))
> -              ((code ,text)
> -               (format #f "@code{~a}" text))
> -              (,something-else something-else)))
> +    ((strong ,text)
> +     (format #f "@strong{~a}" text))

Do we know for sure here that 'text' is actually a string?  What if the
input was (strong (em (a (href "http://super") "Super") emphasis")?

> +    ((a (@ (href ,url)) ,body)
> +     ;; Examples: image in the url: github.com/go-openapi/jsonpointer
> +     ;; (code ...) in the URL body: github.com/mwitkow/go-conntrack
> +     (if (string? body)
> +         (format #f "@url{~a,~a}" url body)
> +         (sxml-match body
> +                     ((code ,text)
> +                      (format #f "@url{~a,~a}" url (sxml->texi body)))
> +                     (,_

I'm not familiar enough with sxml to be sure, but maybe the , can be
removed here.

> +                      (format #f "@url{~a}" url)))))
> +    ((code ,text)
> +     (format #f "@code{~a}" text))
> +    (,something-else
> +     ;; Example: @ in the description: github.com/ethersphere/langos
> +     (if (string? something-else)
> +         (string-replace-substring something-else
> +                                   "@" "@@")
> +         something-else))))

Anyway, more cases are nice, but I recommend tests.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> * guix/import/go.scm (go-module->guix-package*): Catch exceptions and retry in
> case of network errors. Add ability to enable printing a full backtrace when
> debugging.
> (go-module->guix-package): Tolerate the inability to fetch the synopsis and
> description.

Ok, but couldn't this be done more generally, for all importers using
HTTP?  E.g., could we have a variant 'http-fetch/retrying' of 'http-
fetch' that automatically retries a number of times?  Then the retrying
functionality could almost trivially be used in other HTTP-using
importers as well.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +            (warning (G_ "Failing to import package ~S.
> +reason: ~A.~%")

Why a warning instead of a 'report-error'?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> -                         'unknown-license!)))
> +                         (begin
> +                           (warning (G_ "Failed to identify license ~S.~%")
> +                                    license)

Why only for the Go importer and not other importers?
Maybe the behaviour of other importers in presence of unidentifiable
licenses could be unified, with some common support procedure or
something.  Then only a single location needs to be modified to change
things, and all importers would benefit.

> +                           ;; This will put the license there as a string that
> +                           ;; will error at compilation.
> +                           license))))

I don't think this is correct, in the past I've put '(foobar) and
#false in 'license' fields of packages and those packages could be
compiled, without any compilation issues.  I assume the same holds for
strings as well.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +;; TODO use define-constant
> +(define +known-vcs+

The naming convention in (Guile) Scheme is '%known-vcs' or 'known-vcs'.
Also, 'define-constant' doesn't exist in Guile, and the Lisp macro
'defconstant' and 'defmacro' appears to have some complications w.r.t.
phasing.  What would be the benefit of 'define-constant' here?


Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +  (log.info "~%Processing go module ~A@~A" module-path version)

Why a newline at the beginning instead of the end?  Also, why the
logging?  If I do "guix import go foobar", wouldn't I know in advance
it will process the foobar module?  And in case of --recursive,
wouldn't I know after the fact which modules were processed?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +         (raw-subdir (if (< (string-length module-root-path)
> +                            (string-length module-path))
> +                         (substring module-path
> +                                    (+ 1 (string-length module-root-path)))
> +                         #false))

You can use (and COND (substring ...)) here.
Also, the usual extra complexity --> extra tests.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +                              (string-match ".*v([0-9]+)$" raw-subdir))

Why a $ at the end but no "^" at the beginning?  What's the point of
the leading ".*"?  Could the regexp be constructed in advance, or only
once (maybe 'delay/force' and a global variable) -- turning a regexp
into a NFA into a DFA can be expensive.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +         (synopsis (or (false-if-exception
> +                        (go-package-synopsis module-path))

Are you sure you want to ignore _all_ exceptions?  Including stack
overflows, out-of-memories, unbound variable errors, type errors ...?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +                         (warning (G_ "Failed to fetch license for ~S.~%")
> +                                  module-path)
> +                         '("unknown-license!")))))


In some other location, the symbol 'unknonw-license!' is used.  Here, a
string is used.  Why the inconsistency?  Also, given the "unknown-
license!", the warning seems mostly redundant to me.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +    ;; Maybe split comma separated list of licenses in a single string
> +    (when (and (= 1 (length licenses))
> +               (string? (first licenses)))
> +      (let ((pieces (map string-trim-both
> +                         (remove! string-null?

When can it be null?  Also, tests.

> +                                  (string-split (first licenses) #\,)))))

Is there a need for speed here?  If not, I'd keep things simple by
using the non-mutating version (I expect the speed gain to be neglible
here).  If there is, I would also use the mutating version of map --
'map!' from srfi-1.


Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +         '(#:tests? #false ; some packages have unrecorded dependencies needed only by their tests

Then they need to be recorded, and the reviewer will ask why tests are
being skipped here.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Attila Lendvai]

> > +                   (let ((port (current-warning-port)))
> > +                     (format port "Unexpected error, will skip ~S.~%reason: "
> > +                             package-name)
> > +                     ;; Printing a backtrace here is not very useful: it is
> > +                     ;; cut off because GUARD unwinds.
> > +                     (print-exception port (stack-ref (make-stack #t) 1)
> > +                                      c (exception-args c))
> > +                     (display-backtrace (make-stack #t) port))
>
>
> Why are unknown errors being catched here?

*all* errors are caught, displayed, and swallowed, so that the importing can proceed to the rest of the packages.

a possible scenario: a large import can take several minutes. if there's a transient network error, then this way it may finish with 99% of the packages, and the rest can be restarted by hand after inspecting the log output.

another scenario is that the importer is simply buggy, and the user gets 99% of the packages imported, and has to do only one package by hand.


> What's the '-continuable' for here?  Is it to avoid extra 'raise-
> exception' entries in the backtrace, or does this actually make use
> of Scheme's continuable exceptions?


honestly, i can't remember. which translates to: i should have
commented on that!

*shakes head and makes a mental note*

from guile's manual:

"If continuable? is true, the handler is invoked in tail position
relative to the raise-exception call. Otherwise if the handler
returns, a non-continuable exception of type &non-continuable is
raised in the same dynamic environment as the handler."

i.e. it should rather be called raise-continuably not
raise-continuable.

and i think the reason is that the stack is not unwound that way, so
that the handlers in the guard can print a backtrace showing the
entire stack.

does this make sense? i'm still learning scheme's stack/exception
handling...

or maybe what i wanted is that the handlers in the guard can return
with (values #f '())? but i think that should work with vanilla RAISE,
too.


> Why a warning instead of a 'report-error'?


because i want the importer to be able to deal with a transitive
closures of dependencies with > 400 entries. if it fails at the first
error, then a `guix import go -r` becomes a hopeless endeavor for
larger go modules.

thanks for the review Maxime! i'll address the rest, too.

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“'Emergencies' have always been the pretext on which the safeguards of individual liberty have been eroded.”
	— F. A. Hayek (1899–1992)

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:42 [+0200]:
> +  (log.info "Initiating recursive import of ~a, version ~a" package-name version)

This seems implied by typing "guix import go --recursive foo@bar"?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:16 [+0200]:
> note that i only have a mediocre knowledge of the golang
> infrastructure, so this should be reviewed by someone who
> more deeply understands the golang build process, and its
> implementation within guix.
> 
> i think most of it is not very controversial, maybe except
> the last commit.

FWIW (given that I don't know the Go packaging system well), the last
patch (10/10) seems the least controversial to me.  I'm not seeing why
it would be controversial.

Greetings,
Maxime.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 17:00 [+0000]:
> a possible scenario: a large import can take several minutes.
> if there's a transient network error, then this way it may finish
> with 99% of the packages, and the rest can be restarted by hand after
> inspecting the log output.

Catching (presumably) transient network errors and reporting them with
'report-network-error' and the like: no problem.

> another scenario is that the importer is simply buggy,

... but if there's some kind of bug, why not just report and fix it?

> and the user gets 99% of the packages imported, and has to do only
> one package by hand.

If the bug is fixed, then no packages need to be done by hand and other
people will benefit as well.  Whereas if it's only for 1% and the
backtrace only happens for recursive import and not for non-recursive
or manual import, then I'd think that the chance that the user actually
reports or fixes the bug decreases a lot.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Attila Lendvai]

> ... but if there's some kind of bug, why not just report and fix it?


realistically? because there are years old issues, and long bitrotten
patches in every project's issue tracker, Guix included.

i think an importer, that does proper logging, and finishes with a 99%
correct result, is worth much more than one that barks at the first
error, and then refuses to continue processing the 100+ remaining
entries.

this was true even in my own case, when i also had the maintainer hat
on, i.e. i was already neck deep into fixing the importer.


> > and the user gets 99% of the packages imported, and has to do only
> > one package by hand.
>
>
> If the bug is fixed, then no packages need to be done by hand and other
> people will benefit as well. Whereas if it's only for 1% and the
> backtrace only happens for recursive import and not for non-recursive
> or manual import, then I'd think that the chance that the user actually
> reports or fixes the bug decreases a lot.


the tradeoff is this: what leads to a lower level of overall annoyance of all the parties involved:

1) have a tool that is less useful, and through that it annoys the
   users to report issues, which then (hopefully) thrusts some people
   towards improving the tool... or

2) have a tool that does its best to produce the most useful output.

short of hard data, this can be argued forever. my intuition is clearly pointing towards 2), because i doubt that the choking point of improving the importer is due to not having enough examples on which it breaks.

does it make enough sense to justify my proposed behavior?

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“Communism doesn’t work because people like to own stuff.”
	— Frank Zappa (1940–1993)

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Attila Lendvai]

> > note that i only have a mediocre knowledge of the golang
> > infrastructure, so this should be reviewed by someone who
> > more deeply understands the golang build process, and its
> > implementation within guix.
> >
> > i think most of it is not very controversial, maybe except
> > the last commit.
>
>
> FWIW (given that I don't know the Go packaging system well), the last
> patch (10/10) seems the least controversial to me. I'm not seeing why
> it would be controversial.


because it introduces an in-source database of golang projects that
require special-casing (*module-path->import-path*).

i suspect that whatever info i'm storing in that db of exceptions is
probably available somewhere, somehow.

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“Nobody in the world, nobody in history, has ever gotten their freedom by appealing to the moral sense of the people who were oppressing them.”
	— Assata Shakur (1947–)

[bug#55242] [PATCH 08/10] guix: import: go: Modules in a subdir and prefixed tags.

[Attila Lendvai]

> > +  (log.info "~%Processing go module ~A@~A" module-path version)
>
>
> Why a newline at the beginning instead of the end? Also, why the


it makes the log more readable.


> logging? If I do "guix import go foobar", wouldn't I know in advance
> it will process the foobar module? And in case of --recursive,
> wouldn't I know after the fact which modules were processed?


here's how i was working on this codebase:

RUN=12
clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethersphere/bee@v1.5.1 > >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/bee-run-${RUN}.log >&2) && beep

RUN=36
clear && ./pre-inst-env guix import go -r --pin-versions github.com/ethereum/go-ethereum@v1.10.17 > >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.scm) 2> >(tee -a ~/workspace/guix/importing/go-ethereum-run-${RUN}.log >&2) && beep


IOW, i was repeatedly firing long-running imports whose output i stored in versioned files. when a run was finished, i inspected the log and the output file (containing hundreds of packages).

whenever i had to understand what went wrong -- which often manifested in successfull runs producing broken output (i.e. no errors/backtraces) --, i added log statements to see what's happening. i could have deleted the printfs, and then re-add them again a gazillion times, or just leave them there for the upcoming programmers (which includes me), conditionally compiled in with the help of a macro.

background: in my lisp codebase emacs colors the log statement gray, and the logging lib i'm using in CL has a separate compile-time and a runtime log level. IOW, i can eliminate the entire runtime logging overhead, and most of the code readability overhead.

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“Associate yourself with people of good quality, for it is better to be alone than in bad company.”
	— Booker T. Washington (1856–1915)

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op ma 09-05-2022 om 12:34 [+0000]:
> [...] does it make enough sense to justify my proposed behavior?

(For a positive note, skip to the last paragraph!)

If we do something like this (*) (this = catching errors for
dependencies and skipping those packages), I don't want this to be
something Go-specific.  Could this be done in 'recursive-import' or
something like that instead, such that all importers benefit and Go
isn't some weird special case?

Also, given that we appear to have a number of people effectively
maintaining the Go importer ("git log guix/import.go.scm" mentions a
few ‘Maxim Cournoyer’, ‘Ludovic Courtès’ ‘Xinglu Chen’ and ‘Sarah
Morgensen’, and more recently maybe you?) and I don't see any open old
Go importer bugs/patches on issues.guix.gnu.org, I don't think we have
to worry about

> realistically? because there are years old issues, and long bitrotten
> patches in every project's issue tracker, Guix included.

in this case.

> short of hard data, this can be argued forever. my intuition is
> clearly pointing towards 2), because i doubt that the choking point
> of improving the importer is due to not having enough examples on
> which it breaks.

If you know of some examples on which the Go importer broke, could you
add them to tests/go, to make sure it won't break again later
(regression), in case of future changes?

Anyway, let's take a look at for which kind of things I think of in
case of ‘buggy importer’:

1. (Maybe [PATCH 04/10]?)  The parser for a file format cannot parse 
   <something>, so it throws an exception.

   (I consider this more a ‘limitation’ than a ‘bug’.)

   For this kind of thing, maybe we can just throw a ‘oops I don't
   understand the file format’ exception, report an appropriate error
   message (package name + maybe line number, but no need for a
   backtrace), skip that package and continue.  That way, the Go
   importer will be to some degree robust to future changes.

   That's already done in this patch series, except for the ‘no need
   for a backtrace’ part and this patch series catching all exceptions
   instead of just networking errors, ‘I don't understand the file
   format’ errors, ...

2. The Go importer misinterprets some kind of field or such.

   These kind of things don't (typically?) cause errors, so skipping.
   (Still a bug though!)

3. The Go importer cannot produce a package definition because it
   doesn't support <some Go packaging method>.

   Similar (1).

4. The Go importer has some kind of type error or such.

   This case looks like an actual bug to me, where I think that
   catching the resulting exceptions is just sweeping things under
   the rug, implicitly teaching other people that it's ok to just
   ignore problems without making any attempt of fixing them or
   reporting them.  Vaguely similar to ‘broken windows’
   (<https://en.wikipedia.org/wiki/Broken_windows_theory>).

   Sure, for a single particular user and short term this makes the
   importer a bit less useful, but knowing bugs is a prerequisite for
   fixing them, and long term, by actually knowing about the bugs and
   fixing them, more users will benefit.

That said, like "guix build" has a "--keep-going" option to temporarily
ignore to some degree packages that fail to build, maybe "guix import"
(when using --recursive) can have a "--keep-going" option.

Greetings,
Maxime.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Attila Lendvai]

> Also, given that we appear to have a number of people effectively
> maintaining the Go importer ("git log guix/import.go.scm" mentions a
> few ‘Maxim Cournoyer’, ‘Ludovic Courtès’ ‘Xinglu Chen’ and ‘Sarah
> Morgensen’, and more recently maybe you?) and I don't see any open


note that the list of people *using* the go importer is probably not all that much larger.


> old Go importer bugs/patches on issues.guix.gnu.org, I don't think
> we have to worry about


the fact that the go importer bugs were not even reported into the issue tracker doesn't change much about the fact that i have experienced several problems when i first tried to recursively import a non-trivial go project (which even included issues with http-fetch). it means that all those bug were there, not hidden by exception handling, and yet they have *not* been reported into the issue tracker.

or to put it another way, i think there are just too many unverified assumptions in this discussion about actual user behavior. my strategy in those situations is to expose as much info as feasible, and let the users respond intelligently to the situation.


> 4. The Go importer has some kind of type error or such.
>
> This case looks like an actual bug to me, where I think that
> catching the resulting exceptions is just sweeping things under
> the rug, implicitly teaching other people that it's ok to just
> ignore problems without making any attempt of fixing them or
> reporting them. Vaguely similar to ‘broken windows’
> (https://en.wikipedia.org/wiki/Broken_windows_theory).


note that this patch does not just catch errors, but handles them by emitting useful information about them, and continues only after that.

i consider silently swallowing errors a major sin (read: code that will certainly eat up some portion of someone's life in the future).

the only place where this patch silently swallows errors is in the extraction of descriprions and such, i.e. data that the user must review anyway.


> Sure, for a single particular user and short term this makes the
> importer a bit less useful, but knowing bugs is a prerequisite for
> fixing them, and long term, by actually knowing about the bugs and
> fixing them, more users will benefit.


this patch does not hide any bugs, only enables the code to continue by skipping a package.

and as a bonus: i wouldn't be surprised if it emitted *more* useful information than when allowing the exception to pass through to the caller, because error handling in the guix codebase could use a bit more love.


> That said, like "guix build" has a "--keep-going" option to temporarily
> ignore to some degree packages that fail to build, maybe "guix import"
> (when using --recursive) can have a "--keep-going" option.


ok, i'll look into putting this feature behind a CLI argument, and generalize it to all importers. as time allows i'll get back to the rest of the questions/suggestions, too.

thanks for the feedback Maxime!

--
• attila lendvai
• PGP: 963F 5D5F 45C7 DFCD 0A39
--
“A great civilization is not conquered from without until it destroys itself from within.”
	— Will Durant

[bug#55242] [PATCH 07/10] guix: import: go: More resilience wrt network errors; add logging.

[Maxime Devos]

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

Attila Lendvai schreef op ma 09-05-2022 om 20:02 [+0000]:
> note that this patch does not just catch errors, but handles them by emitting useful information about them, and continues only after that.

I think such errors are easily overlooked, but yes, and I don't have
any actual data on that.

> i consider silently swallowing errors a major sin (read: code that will certainly eat up some portion of someone's life in the future).
> 
> the only place where this patch silently swallows errors is in the extraction of descriprions and such, i.e. data that the user must review anyway.
> [...]

I think I've overstated the ‘swallows errors etc.’ a bit, given that
if "guix import go --recursive <foo>" skips a package <bar>, then the
user will have to package its dependency <bar> anyway, and for that
the user could first try "guix import go <bar>" (reporting errors on
bug-guix@ or #guix) before fixing/extending the importer or doing
things manually.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 10/10] guix: import: go: Better handling of /v2 in the module path.

[Maxime Devos]

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

Attila Lendvai schreef op di 03-05-2022 om 13:43 [+0200]:
> +(define *module-path->import-path*

In (Guile) Scheme (at least as used in the Scheme code I've seen),
*earmuffs* are typically used for mutable variables, not for global
constants.  Though guix/self.scm uses *system-modules* for not-really-
constant-but-not-mutated lexical variables so maybe ...

FWIW, I would write (define %module-path->import-path ...) here, not
that it really matters.

> +  ;; There's no other way to derive this information, at least that I know of.
> +  '(("github.com/google/go-cmp" . "github.com/google/go-cmp/cmp")
> +    ("github.com/sergi/go-diff" . "github.com/sergi/go-diff/diffmatchpatch")
> +    ("github.com/davecgh/go-spew" . "github.com/davecgh/go-spew/spew")
> +    ("github.com/beorn7/perks" . "github.com/beorn7/perks/quantile")
> +    ("github.com/census-instrumentation/opencensus-proto" .
> +     "github.com/census-instrumentation/opencensus-proto/gen-go")))

About the ‘is this controversional’ bit: these kind of mappings and
sets have precedent, see e.g. 'known-vcs' in (guix import cran),
%builtin-mods in (guix import minetest), default-r-packages in (guix
import cran).

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Maxime Devos]

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

Hi,

I forgot that "guix build" already does some debug logging when passed
"--verbose" or "--debug".  Maybe "guix import ..." can gain a similar 
system.  I don't know if the internals are reusable though ...
(I'm not convinced that we need logging for "guix import" but I'm not
convinced of the contrary either, and there is already some kind of
debug logging system elsewhere.)

> here's how i was working on this codebase:
> 
> RUN=12
> clear && ./pre-inst-env guix import go -r --pin-versions  
> github.com/ethersphere/bee@v1.5.1 > >(tee -a
> ~/workspace/guix/importing/bee-run-${RUN}.scm) 2> >(tee -a
> ~/workspace/guix/importing/bee-run-${RUN}.log >&2) && beep
> 

FWIW, for these kind of things I do

  while ./pre-inst-env guix some-command; do echo 'Next'; done

and for your case (broken output without errors), maybe

  while true; do echo 'Next run' >> the-log-file && ./pre-inst-env guix some-command >> the-log-file; done

Greetings,
Maxime.




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Maxime Devos]

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

> > That said, like "guix build" has a "--keep-going" option to
> temporarily
> > ignore to some degree packages that fail to build, maybe "guix
> import"
> > (when using --recursive) can have a "--keep-going" option.
> 
> 
> ok, i'll look into putting this feature behind a CLI argument, and
> generalize it to all importers. as time allows i'll get back to the
> rest of the questions/suggestions, too.

I'm not sure if I mentioned it previously, but it looks like some
patches are uncontroversial and usable mostly as-is (e.g. 04/10 without
the logging, 02/10, 05/10 with verifying types, 06/10 without the
overly-general false-if-exception and logging, 9/10, 10/10, 04/10). So
you could try the non-controversial bits first and then see if some
kind of consensus or such could be found for the remainder.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55242] [PATCH 01/10] guix: import: Print the number of packages at the end.

[Maxim Cournoyer]

Hello Attila,

Attila Lendvai <attila@lendvai.name> writes:

> Introduce a (local) mockup logger, so that we don't need to keep adding and
> deleting format's when debugging the codebase.
>
> * guix/import/go.scm (log.info) (log.debug): New macros.
> ---
>  guix/import/go.scm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/guix/import/go.scm b/guix/import/go.scm
> index bb4bb7bb7b..0af5e4b5e2 100644
> --- a/guix/import/go.scm
> +++ b/guix/import/go.scm
> @@ -100,6 +100,17 @@ (define-module (guix import go)
>  
>  ;;; Code:
>  
> +;; FIXME set up logging for the entire project, and replace this poor man's
> +;; logger with the proper one.
> +(define-syntax-rule (log.info format-string ...)
> +  (let ((port (current-warning-port)))
> +    (format port format-string ...)
> +    (newline port)))
> +
> +(define-syntax-rule (log.debug format-string ...)
> +  ;;(log.info format-string ...)
> +  '())
> +

We alreading have 'warning' and 'info' in (guix diagnostics); perhaps we
could also have 'debug', enabled when GUIX_DEBUG is set or something.
Alternatively, there's a full blown logging library available in the
Guile-Lib project, as (logging logger) [0].

[0]  https://www.nongnu.org/guile-lib/doc/ref/logging.logger/

Maxim