[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Sarah Morgensen via Guix-patches via]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 20466 bytes --]

Upgrade the go.mod parser to handle the full go.mod spec, and to
gracefully handle unexpected/malformed syntax. Restructure parser usage,
making the parse tree available for other uses.

guix/import/go.scm (parse-go.mod): Parse using (ice-9 peg) instead of
regex matching for more robustness. Return a list of directives.
(go.mod-directives): New procedure.
(go.mod-requirements): New procedure.
(go-module->guix-package): Use it.
(%go.mod-require-directive-rx)
(%go.mod-replace-directive-rx): Remove unused variable.
tests/go.scm (testing-parse-mod): Adjust accordingly.
(go.mod-requirements)
(fixture-go-mod-unparseable)
(fixture-go-mod-retract)
(fixture-go-mod-strings): New variable.
("parse-go.mod: simple")
("parse-go.mod: comments and unparseable lines")
("parse-go.mod: retract")
("parse-go.mod: raw strings and quoted strings")
("parse-go.mod: complete"): New tests.
---
Hello Guix,

This one is pretty self-explanatory. This parser handles the full go.mod spec
(notably: comments, extra whitespace, retract/exclude) and gracefully handles
things it doesn't understand. It is a bit more complex, I suppose. WDYT?

--
Sarah

 guix/import/go.scm | 247 +++++++++++++++++++++++----------------------
 tests/go.scm       | 132 +++++++++++++++++++++++-
 2 files changed, 260 insertions(+), 119 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index d8f838f635..69e71d01e5 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -41,6 +41,7 @@
   #:autoload   (guix base32) (bytevector->nix-base32-string)
   #:autoload   (guix build utils) (mkdir-p)
   #:use-module (ice-9 match)
+  #:use-module (ice-9 peg)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 receive)
   #:use-module (ice-9 regex)
@@ -244,129 +245,139 @@ and VERSION and return an input port."
                      (go-path-escape version))))
     (http-fetch* url)))
 
-(define %go.mod-require-directive-rx
-  ;; A line in a require directive is composed of a module path and
-  ;; a version separated by whitespace and an optionnal '//' comment at
-  ;; the end.
-  (make-regexp
-   (string-append
-    "^[[:blank:]]*([^[:blank:]]+)[[:blank:]]+" ;the module path
-    "([^[:blank:]]+)"                          ;the version
-    "([[:blank:]]+//.*)?")))                   ;an optional comment
-
-(define %go.mod-replace-directive-rx
-  ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline
-  ;;             | ModulePath [ Version ] "=>" ModulePath Version newline .
-  (make-regexp
-   (string-append
-    "([^[:blank:]]+)"                   ;the module path
-    "([[:blank:]]+([^[:blank:]]+))?"    ;optional version
-    "[[:blank:]]+=>[[:blank:]]+"
-    "([^[:blank:]]+)"                   ;the file or module path
-    "([[:blank:]]+([^[:blank:]]+))?"))) ;the version (if a module path)
 
 (define (parse-go.mod content)
-  "Parse the go.mod file CONTENT, returning a list of requirements."
-  ;; We parse only a subset of https://golang.org/ref/mod#go-mod-file-grammar
-  ;; which we think necessary for our use case.
-  (define (toplevel requirements replaced)
-    "This is the main parser.  The results are accumulated in THE REQUIREMENTS
-and REPLACED lists."
-    (let ((line (read-line)))
-      (cond
-       ((eof-object? line)
-        ;; parsing ended, give back the result
-        (values requirements replaced))
-       ((string=? line "require (")
-        ;; a require block begins, delegate parsing to IN-REQUIRE
-        (in-require requirements replaced))
-       ((string=? line "replace (")
-        ;; a replace block begins, delegate parsing to IN-REPLACE
-        (in-replace requirements replaced))
-       ((string-prefix? "require " line)
-        ;; a require directive by itself
-        (let* ((stripped-line (string-drop line 8)))
-          (call-with-values
-              (lambda ()
-                (require-directive requirements replaced stripped-line))
-            toplevel)))
-       ((string-prefix? "replace " line)
-        ;; a replace directive by itself
-        (let* ((stripped-line (string-drop line 8)))
-          (call-with-values
-              (lambda ()
-                (replace-directive requirements replaced stripped-line))
-            toplevel)))
-       (#t
-        ;; unrecognised line, ignore silently
-        (toplevel requirements replaced)))))
-
-  (define (in-require requirements replaced)
-    (let ((line (read-line)))
-      (cond
-       ((eof-object? line)
-        ;; this should never happen here but we ignore silently
-        (values requirements replaced))
-       ((string=? line ")")
-        ;; end of block, coming back to toplevel
-        (toplevel requirements replaced))
-       (#t
-        (call-with-values (lambda ()
-                            (require-directive requirements replaced line))
-          in-require)))))
-
-  (define (in-replace requirements replaced)
-    (let ((line (read-line)))
-      (cond
-       ((eof-object? line)
-        ;; this should never happen here but we ignore silently
-        (values requirements replaced))
-       ((string=? line ")")
-        ;; end of block, coming back to toplevel
-        (toplevel requirements replaced))
-       (#t
-        (call-with-values (lambda ()
-                            (replace-directive requirements replaced line))
-          in-replace)))))
-
-  (define (replace-directive requirements replaced line)
-    "Extract replaced modules and new requirements from the replace directive
-in LINE and add them to the REQUIREMENTS and REPLACED lists."
-    (let* ((rx-match (regexp-exec %go.mod-replace-directive-rx line))
-           (module-path (match:substring rx-match 1))
-           (version (match:substring rx-match 3))
-           (new-module-path (match:substring rx-match 4))
-           (new-version (match:substring rx-match 6))
-           (new-replaced (cons (list module-path version) replaced))
-           (new-requirements
-            (if (string-match "^\\.?\\./" new-module-path)
-                requirements
-                (cons (list new-module-path new-version) requirements))))
-      (values new-requirements new-replaced)))
-
-  (define (require-directive requirements replaced line)
-    "Extract requirement from LINE and augment the REQUIREMENTS and REPLACED
-lists."
-    (let* ((rx-match (regexp-exec %go.mod-require-directive-rx line))
-           (module-path (match:substring rx-match 1))
-           ;; Double-quoted strings were seen in the wild without escape
-           ;; sequences; trim the quotes to be on the safe side.
-           (module-path (string-trim-both module-path #\"))
-           (version (match:substring rx-match 2)))
-      (values (cons (list module-path version) requirements) replaced)))
-
-  (with-input-from-string content
-    (lambda ()
-      (receive (requirements replaced)
-          (toplevel '() '())
-        ;; At last remove the replaced modules from the requirements list.
-        (remove (lambda (r)
-                  (assoc (car r) replaced))
-                requirements)))))
+  "Parse the go.mod file CONTENT, returning a list of directives, comments,
+and unknown lines. Each sublist begins with a symbol (go, module, require,
+replace, exclude, retract, comment, or unknown) and is followed by one or more
+sublists. Each sublist begins with a symbol (module-path, version, file-path,
+comment, or unknown) and is followed by the indicated data."
+  ;; https://golang.org/ref/mod#go-mod-file-grammar
+  (define-peg-pattern NL none "\n")
+  (define-peg-pattern WS none (or " " "\t" "\r"))
+  (define-peg-pattern => none (and (* WS) "=>"))
+  (define-peg-pattern punctuation none (or "," "=>" "[" "]" "(" ")"))
+  (define-peg-pattern comment all
+    (and (ignore "//") (* WS) (* (and (not-followed-by NL) peg-any))))
+  (define-peg-pattern EOL body (and (* WS) (? comment) NL))
+  (define-peg-pattern block-start none (and (* WS) "(" EOL))
+  (define-peg-pattern block-end none (and (* WS) ")" EOL))
+  (define-peg-pattern any-line body
+    (and (* WS) (* (and (not-followed-by NL) peg-any)) EOL))
+
+  ;; Strings and identifiers
+  (define-peg-pattern identifier body
+    (+ (and (not-followed-by (or NL WS punctuation)) peg-any)))
+  (define-peg-pattern string-raw body
+     (and (ignore "`") (+ (and (not-followed-by "`") peg-any)) (ignore "`")))
+  (define-peg-pattern string-quoted body
+    (and (ignore "\"")
+         (+ (or (and (ignore "\\") peg-any)
+                (and (not-followed-by "\"") peg-any)))
+         (ignore "\"")))
+  (define-peg-pattern string-or-ident body
+    (and (* WS) (or string-raw string-quoted identifier)))
+
+  (define-peg-pattern version all string-or-ident)
+  (define-peg-pattern module-path all string-or-ident)
+  (define-peg-pattern file-path all string-or-ident)
+
+  ;; Non-directive lines
+  (define-peg-pattern unknown all any-line)
+  (define-peg-pattern block-line body
+    (or EOL (and (not-followed-by block-end) unknown)))
+
+  ;; GoDirective = "go" GoVersion newline .
+  (define-peg-pattern go all (and (ignore "go") version EOL))
+
+  ;; ModuleDirective = "module" ( ModulePath | "(" newline ModulePath newline ")" ) newline .
+  (define-peg-pattern module all
+    (and (ignore "module") (or (and block-start module-path EOL block-end)
+                               (and module-path EOL))))
+
+  ;; The following directives may all be used solo or in a block
+  ;; RequireSpec = ModulePath Version newline .
+  (define-peg-pattern require all (and module-path version EOL))
+  (define-peg-pattern require-top body
+    (and (ignore "require")
+         (or (and block-start (* (or require block-line)) block-end) require)))
+
+  ;; ExcludeSpec = ModulePath Version newline .
+  (define-peg-pattern exclude all (and module-path version EOL))
+  (define-peg-pattern exclude-top body
+    (and (ignore "exclude")
+         (or (and block-start (* (or exclude block-line)) block-end) exclude)))
+
+  ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline
+  ;;             | ModulePath [ Version ] "=>" ModulePath Version newline .
+  (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") 
+         (or (and block-start (* (or replace block-line)) block-end) replace)))
+
+  ;; RetractSpec = ( Version | "[" Version "," Version "]" ) newline .
+  (define-peg-pattern range all
+    (and (* WS) (ignore "[") version
+         (* WS) (ignore ",") version (* WS) (ignore "]")))
+  (define-peg-pattern retract all (and (or range version) EOL))
+  (define-peg-pattern retract-top body
+    (and (ignore "retract")
+         (or (and block-start (* (or retract block-line)) block-end) retract)))
+
+  (define-peg-pattern go-mod body
+    (* (and (* WS) (or go module require-top exclude-top replace-top
+                       retract-top EOL unknown))))
+
+  (let ((tree (peg:tree (match-pattern go-mod content)))
+        (keywords '(go module require replace exclude retract comment unknown)))
+    (keyword-flatten keywords tree)))
 
 ;; Prevent inlining of this procedure, which is accessed by unit tests.
 (set! parse-go.mod parse-go.mod)
 
+(define (go.mod-directives go.mod directive)
+  "Return the list of top-level directive bodies in GO.MOD matching the symbol
+DIRECTIVE."
+  (filter-map (match-lambda
+                (((? (cut eq? <> directive) head) . rest) rest)
+                (_ #f))
+              go.mod))
+
+(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
+      ;; TODO: Is this correct behavior? It's in the go.mod for a reason...
+      (if (and (equal? module-path (first new-requirement))
+               (not (assoc-ref requirements module-path)))
+          requirements
+          (cons new-requirement (alist-delete module-path requirements))))
+
+    (match directive
+      ((('original ('module-path module-path) . _) with . _)
+       (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)))))))
+
+  (define (require directive requirements)
+    (match directive
+      ((('module-path module-path) ('version version) . _)
+       (cons (list module-path version) requirements))))
+
+  (let* ((requires (go.mod-directives go.mod 'require))
+         (replaces (go.mod-directives go.mod 'replace))
+         (requirements (fold require '() requires)))
+    (fold replace requirements replaces)))
+
+;; Prevent inlining of this procedure, which is accessed by unit tests.
+(set! go.mod-requirements go.mod-requirements)
+
 (define-record-type <vcs>
   (%make-vcs url-prefix root-regex type)
   vcs?
@@ -588,7 +599,7 @@ When VERSION is unspecified, the latest version available is used."
 hint: use one of the following available versions ~a\n"
                              version* available-versions))))
          (content (fetch-go.mod goproxy module-path version*))
-         (dependencies+versions (parse-go.mod content))
+         (dependencies+versions (go.mod-requirements (parse-go.mod content)))
          (dependencies (if pin-versions?
                            dependencies+versions
                            (map car dependencies+versions)))
diff --git a/tests/go.scm b/tests/go.scm
index b088ab50d2..b16965d941 100644
--- a/tests/go.scm
+++ b/tests/go.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2021 François Joulaud <francois.joulaud@radiofrance.com>
+;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -31,6 +32,9 @@
   #:use-module (srfi srfi-64)
   #:use-module (web response))
 
+(define go.mod-requirements
+  (@@ (guix import go) go.mod-requirements))
+
 (define parse-go.mod
   (@@ (guix import go) parse-go.mod))
 
@@ -96,6 +100,40 @@ replace (
 
 ")
 
+(define fixture-go-mod-unparseable
+  "module my/thing
+go 1.12 // avoid feature X
+require other/thing v1.0.2
+// Security issue: CVE-XXXXX
+exclude old/thing v1.2.3
+new-directive another/thing yet-another/thing
+replace (
+        bad/thing v1.4.5 => good/thing v1.4.5
+        // Unparseable
+        bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0
+)
+")
+
+(define fixture-go-mod-retract
+  "retract v0.9.1
+
+retract (
+	v1.9.2
+	[v1.0.0, v1.7.9]
+)
+")
+
+(define fixture-go-mod-strings
+  "require `example.com/\"some-repo\"` v1.9.3
+require (
+        `example.com/\"another.repo\"` v1.0.0
+        \"example.com/special!repo\" v9.3.1
+)
+replace \"example.com/\\\"some-repo\\\"\" => `launchpad.net/some-repo` v1.9.3
+replace (
+        \"example.com/\\\"another.repo\\\"\" => launchpad.net/another-repo v1.0.0
+)
+")
 
 
 (define fixture-latest-for-go-check
@@ -185,7 +223,7 @@ require github.com/kr/pretty v0.2.1
     (string<? (car p1) (car p2)))
   (test-equal name
     (sort expected inf?)
-    (sort ((@@ (guix import go) parse-go.mod) input) inf?)))
+    (sort (go.mod-requirements (parse-go.mod input)) inf?)))
 
 (testing-parse-mod "parse-go.mod-simple"
                    '(("good/thing" "v1.4.5")
@@ -221,6 +259,98 @@ require github.com/kr/pretty v0.2.1
    ("github.com/go-check/check" "v0.0.0-20140225173054-eb6ee6f84d0a"))
  fixture-go-mod-complete)
 
+(test-equal "parse-go.mod: simple"
+  `((module (module-path "my/thing"))
+    (go (version "1.12"))
+    (require (module-path "other/thing") (version "v1.0.2"))
+    (require (module-path "new/thing/v2") (version "v2.3.4"))
+    (exclude (module-path "old/thing") (version "v1.2.3"))
+    (replace (original (module-path "bad/thing") (version "v1.4.5"))
+      (with (module-path "good/thing") (version "v1.4.5"))))
+  (parse-go.mod fixture-go-mod-simple))
+
+(test-equal "parse-go.mod: comments and unparseable lines"
+  `((module (module-path "my/thing"))
+    (go (version "1.12") (comment "avoid feature X"))
+    (require (module-path "other/thing") (version "v1.0.2"))
+    (comment "Security issue: CVE-XXXXX")
+    (exclude (module-path "old/thing") (version "v1.2.3"))
+    (unknown "new-directive another/thing yet-another/thing")
+    (replace (original (module-path "bad/thing") (version "v1.4.5"))
+      (with (module-path "good/thing") (version "v1.4.5")))
+    (comment "Unparseable")
+    (unknown "bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0"))
+  (parse-go.mod fixture-go-mod-unparseable))
+
+(test-equal "parse-go.mod: retract"
+  `((retract (version "v0.9.1"))
+    (retract (version "v1.9.2"))
+    (retract (range (version "v1.0.0") (version "v1.7.9"))))
+  (parse-go.mod fixture-go-mod-retract))
+
+(test-equal "parse-go.mod: raw strings and quoted strings"
+  `((require (module-path "example.com/\"some-repo\"") (version "v1.9.3"))
+    (require (module-path "example.com/\"another.repo\"") (version "v1.0.0"))
+    (require (module-path "example.com/special!repo") (version "v9.3.1"))
+    (replace (original (module-path "example.com/\"some-repo\""))
+      (with (module-path "launchpad.net/some-repo") (version "v1.9.3")))
+    (replace (original (module-path "example.com/\"another.repo\""))
+      (with (module-path "launchpad.net/another-repo") (version "v1.0.0"))))
+  (parse-go.mod fixture-go-mod-strings))
+
+(test-equal "parse-go.mod: complete"
+  `((module (module-path "M"))
+    (go (version "1.13"))
+    (replace (original (module-path "github.com/myname/myproject/myapi"))
+      (with (file-path "./api")))
+    (replace (original (module-path "github.com/mymname/myproject/thissdk"))
+      (with (file-path "../sdk")))
+    (replace (original (module-path "launchpad.net/gocheck"))
+      (with (module-path "github.com/go-check/check")
+            (version "v0.0.0-20140225173054-eb6ee6f84d0a")))
+    (require (module-path "github.com/user/project")
+             (version "v1.1.11"))
+    (require (module-path "github.com/user/project/sub/directory")
+             (version "v1.1.12"))
+    (require (module-path "bitbucket.org/user/project")
+             (version "v1.11.20"))
+    (require (module-path "bitbucket.org/user/project/sub/directory")
+             (version "v1.11.21"))
+    (require (module-path "launchpad.net/project")
+             (version "v1.1.13"))
+    (require (module-path "launchpad.net/project/series")
+             (version "v1.1.14"))
+    (require (module-path "launchpad.net/project/series/sub/directory")
+             (version "v1.1.15"))
+    (require (module-path "launchpad.net/~user/project/branch")
+             (version "v1.1.16"))
+    (require (module-path "launchpad.net/~user/project/branch/sub/directory")
+             (version "v1.1.17"))
+    (require (module-path "hub.jazz.net/git/user/project")
+             (version "v1.1.18"))
+    (require (module-path "hub.jazz.net/git/user/project/sub/directory")
+             (version "v1.1.19"))
+    (require (module-path "k8s.io/kubernetes/subproject")
+             (version "v1.1.101"))
+    (require (module-path "one.example.com/abitrary/repo")
+             (version "v1.1.111"))
+    (require (module-path "two.example.com/abitrary/repo")
+             (version "v0.0.2"))
+    (require (module-path "quoted.example.com/abitrary/repo")
+             (version "v0.0.2"))
+    (replace (original (module-path "two.example.com/abitrary/repo"))
+      (with (module-path "github.com/corp/arbitrary-repo")
+            (version "v0.0.2")))
+    (replace (original (module-path "golang.org/x/sys"))
+      (with (module-path "golang.org/x/sys")
+            (version "v0.0.0-20190813064441-fde4db37ae7a"))
+      (comment "pinned to release-branch.go1.13"))
+    (replace (original (module-path "golang.org/x/tools"))
+      (with (module-path "golang.org/x/tools")
+            (version "v0.0.0-20190821162956-65e3620a7ae7"))
+      (comment "pinned to release-branch.go1.13")))
+  (parse-go.mod fixture-go-mod-complete))
+
 ;;; End-to-end tests for (guix import go)
 (define (mock-http-fetch testcase)
   (lambda (url . rest)

base-commit: 3ee0f170c8bd883728d8abb2c2e00f445c13f17d
-- 
2.31.1

bug#49602: [PATCH] import: go: Upgrade go.mod parser.

[Maxim Cournoyer]

Hi!

Sarah Morgensen <iskarian@mgsn.dev> writes:

> Upgrade the go.mod parser to handle the full go.mod spec, and to
> gracefully handle unexpected/malformed syntax. Restructure parser
                                                ^ There is a convention
in Guix to add two spaces to separate the the ending (period) of a
sentence from the beginning of the next one (which was inherited from
GNU Standards, AFAICT: "Please put two spaces after the end of a
sentence in your comments, so that the Emacs sentence commands will
work.", c.f. info "(standards) Comments").

> usage, making the parse tree available for other uses.
>
> guix/import/go.scm (parse-go.mod): Parse using (ice-9 peg) instead of
> regex matching for more robustness. Return a list of directives.
> (go.mod-directives): New procedure.
> (go.mod-requirements): New procedure.
                         ^Likewise.
                         
> (go-module->guix-package): Use it.
> (%go.mod-require-directive-rx)

The above line can be removed, as it is duplicated below.

> (%go.mod-replace-directive-rx): Remove unused variable.
> tests/go.scm (testing-parse-mod): Adjust accordingly.
> (go.mod-requirements)
> (fixture-go-mod-unparseable)
> (fixture-go-mod-retract)
> (fixture-go-mod-strings): New variable.
                                        ^s.
> ("parse-go.mod: simple")
> ("parse-go.mod: comments and unparseable lines")
> ("parse-go.mod: retract")
> ("parse-go.mod: raw strings and quoted strings")
> ("parse-go.mod: complete"): New tests.
> ---
> Hello Guix,
>
> This one is pretty self-explanatory. This parser handles the full go.mod spec
> (notably: comments, extra whitespace, retract/exclude) and gracefully handles
> things it doesn't understand. It is a bit more complex, I suppose. WDYT?

This looks really neat!  I'm happy to see more use of (ice-9 peg).  I
think the added complexity is worth it, given the benefits it provides.

> -- Sarah
>
>  guix/import/go.scm | 247 +++++++++++++++++++++++----------------------
>  tests/go.scm       | 132 +++++++++++++++++++++++-
>  2 files changed, 260 insertions(+), 119 deletions(-)
>
> diff --git a/guix/import/go.scm b/guix/import/go.scm
> index d8f838f635..69e71d01e5 100644
> --- a/guix/import/go.scm
> +++ b/guix/import/go.scm
> @@ -41,6 +41,7 @@
>    #:autoload   (guix base32) (bytevector->nix-base32-string)
>    #:autoload   (guix build utils) (mkdir-p)
>    #:use-module (ice-9 match)
> +  #:use-module (ice-9 peg)
>    #:use-module (ice-9 rdelim)
>    #:use-module (ice-9 receive)
>    #:use-module (ice-9 regex)
> @@ -244,129 +245,139 @@ and VERSION and return an input port."
>                       (go-path-escape version))))
>      (http-fetch* url)))
>
> -(define %go.mod-require-directive-rx
> -  ;; A line in a require directive is composed of a module path and
> -  ;; a version separated by whitespace and an optionnal '//' comment at
> -  ;; the end.
> -  (make-regexp
> -   (string-append
> -    "^[[:blank:]]*([^[:blank:]]+)[[:blank:]]+" ;the module path
> -    "([^[:blank:]]+)"                          ;the version
> -    "([[:blank:]]+//.*)?")))                   ;an optional comment
> -
> -(define %go.mod-replace-directive-rx
> -  ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline
> -  ;;             | ModulePath [ Version ] "=>" ModulePath Version newline .
> -  (make-regexp
> -   (string-append
> -    "([^[:blank:]]+)"                   ;the module path
> -    "([[:blank:]]+([^[:blank:]]+))?"    ;optional version
> -    "[[:blank:]]+=>[[:blank:]]+"
> -    "([^[:blank:]]+)"                   ;the file or module path
> -    "([[:blank:]]+([^[:blank:]]+))?"))) ;the version (if a module path)
>
>  (define (parse-go.mod content)
> -  "Parse the go.mod file CONTENT, returning a list of requirements."
> -  ;; We parse only a subset of https://golang.org/ref/mod#go-mod-file-grammar
> -  ;; which we think necessary for our use case.
> -  (define (toplevel requirements replaced)
> -    "This is the main parser.  The results are accumulated in THE REQUIREMENTS
> -and REPLACED lists."
> -    (let ((line (read-line)))
> -      (cond
> -       ((eof-object? line)
> -        ;; parsing ended, give back the result
> -        (values requirements replaced))
> -       ((string=? line "require (")
> -        ;; a require block begins, delegate parsing to IN-REQUIRE
> -        (in-require requirements replaced))
> -       ((string=? line "replace (")
> -        ;; a replace block begins, delegate parsing to IN-REPLACE
> -        (in-replace requirements replaced))
> -       ((string-prefix? "require " line)
> -        ;; a require directive by itself
> -        (let* ((stripped-line (string-drop line 8)))
> -          (call-with-values
> -              (lambda ()
> -                (require-directive requirements replaced stripped-line))
> -            toplevel)))
> -       ((string-prefix? "replace " line)
> -        ;; a replace directive by itself
> -        (let* ((stripped-line (string-drop line 8)))
> -          (call-with-values
> -              (lambda ()
> -                (replace-directive requirements replaced stripped-line))
> -            toplevel)))
> -       (#t
> -        ;; unrecognised line, ignore silently
> -        (toplevel requirements replaced)))))
> -
> -  (define (in-require requirements replaced)
> -    (let ((line (read-line)))
> -      (cond
> -       ((eof-object? line)
> -        ;; this should never happen here but we ignore silently
> -        (values requirements replaced))
> -       ((string=? line ")")
> -        ;; end of block, coming back to toplevel
> -        (toplevel requirements replaced))
> -       (#t
> -        (call-with-values (lambda ()
> -                            (require-directive requirements replaced line))
> -          in-require)))))
> -
> -  (define (in-replace requirements replaced)
> -    (let ((line (read-line)))
> -      (cond
> -       ((eof-object? line)
> -        ;; this should never happen here but we ignore silently
> -        (values requirements replaced))
> -       ((string=? line ")")
> -        ;; end of block, coming back to toplevel
> -        (toplevel requirements replaced))
> -       (#t
> -        (call-with-values (lambda ()
> -                            (replace-directive requirements replaced line))
> -          in-replace)))))
> -
> -  (define (replace-directive requirements replaced line)
> -    "Extract replaced modules and new requirements from the replace directive
> -in LINE and add them to the REQUIREMENTS and REPLACED lists."
> -    (let* ((rx-match (regexp-exec %go.mod-replace-directive-rx line))
> -           (module-path (match:substring rx-match 1))
> -           (version (match:substring rx-match 3))
> -           (new-module-path (match:substring rx-match 4))
> -           (new-version (match:substring rx-match 6))
> -           (new-replaced (cons (list module-path version) replaced))
> -           (new-requirements
> -            (if (string-match "^\\.?\\./" new-module-path)
> -                requirements
> -                (cons (list new-module-path new-version) requirements))))
> -      (values new-requirements new-replaced)))
> -
> -  (define (require-directive requirements replaced line)
> -    "Extract requirement from LINE and augment the REQUIREMENTS and REPLACED
> -lists."
> -    (let* ((rx-match (regexp-exec %go.mod-require-directive-rx line))
> -           (module-path (match:substring rx-match 1))
> -           ;; Double-quoted strings were seen in the wild without escape
> -           ;; sequences; trim the quotes to be on the safe side.
> -           (module-path (string-trim-both module-path #\"))
> -           (version (match:substring rx-match 2)))
> -      (values (cons (list module-path version) requirements) replaced)))
> -
> -  (with-input-from-string content
> -    (lambda ()
> -      (receive (requirements replaced)
> -          (toplevel '() '())
> -        ;; At last remove the replaced modules from the requirements list.
> -        (remove (lambda (r)
> -                  (assoc (car r) replaced))
> -                requirements)))))
> +  "Parse the go.mod file CONTENT, returning a list of directives, comments,
> +and unknown lines. Each sublist begins with a symbol (go, module, require,
> +replace, exclude, retract, comment, or unknown) and is followed by one or more
> +sublists. Each sublist begins with a symbol (module-path, version, file-path,
> +comment, or unknown) and is followed by the indicated data."
> +  ;; https://golang.org/ref/mod#go-mod-file-grammar
> +  (define-peg-pattern NL none "\n")
> +  (define-peg-pattern WS none (or " " "\t" "\r"))

> +  (define-peg-pattern => none (and (* WS) "=>"))
> +  (define-peg-pattern punctuation none (or "," "=>" "[" "]" "(" ")"))
> +  (define-peg-pattern comment all
> +    (and (ignore "//") (* WS) (* (and (not-followed-by NL) peg-any))))
> +  (define-peg-pattern EOL body (and (* WS) (? comment) NL))
> +  (define-peg-pattern block-start none (and (* WS) "(" EOL))
> +  (define-peg-pattern block-end none (and (* WS) ")" EOL))w
> +  (define-peg-pattern any-line body
> +    (and (* WS) (* (and (not-followed-by NL) peg-any)) EOL))
> +
> +  ;; Strings and identifiers
> +  (define-peg-pattern identifier body
> +    (+ (and (not-followed-by (or NL WS punctuation)) peg-any)))
> +  (define-peg-pattern string-raw body
> +     (and (ignore "`") (+ (and (not-followed-by "`") peg-any)) (ignore "`")))
> +  (define-peg-pattern string-quoted body
> +    (and (ignore "\"")
> +         (+ (or (and (ignore "\\") peg-any)
> +                (and (not-followed-by "\"") peg-any)))
> +         (ignore "\"")))
> +  (define-peg-pattern string-or-ident body
                                   ^
Perhaps prefer the fully spelled out 'string-or-identifier' variable name.

> +    (and (* WS) (or string-raw string-quoted identifier)))
> +
> +  (define-peg-pattern version all string-or-ident)
> +  (define-peg-pattern module-path all string-or-ident)
> +  (define-peg-pattern file-path all string-or-ident)
> +
> +  ;; Non-directive lines
> +  (define-peg-pattern unknown all any-line)
> +  (define-peg-pattern block-line body
> +    (or EOL (and (not-followed-by block-end) unknown)))
> +
> +  ;; GoDirective = "go" GoVersion newline .
> +  (define-peg-pattern go all (and (ignore "go") version EOL))
> +
> +  ;; ModuleDirective = "module" ( ModulePath | "(" newline ModulePath newline ")" ) newline .
> +  (define-peg-pattern module all
> +    (and (ignore "module") (or (and block-start module-path EOL block-end)
> +                               (and module-path EOL))))
> +
> +  ;; The following directives may all be used solo or in a block
> +  ;; RequireSpec = ModulePath Version newline .
> +  (define-peg-pattern require all (and module-path version EOL))
> +  (define-peg-pattern require-top body
> +    (and (ignore "require")
> +         (or (and block-start (* (or require block-line)) block-end) require)))
> +
> +  ;; ExcludeSpec = ModulePath Version newline .
> +  (define-peg-pattern exclude all (and module-path version EOL))
> +  (define-peg-pattern exclude-top body
> +    (and (ignore "exclude")
> +         (or (and block-start (* (or exclude block-line)) block-end) exclude)))
> +
> +  ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline
> +  ;;             | ModulePath [ Version ] "=>" ModulePath Version newline .
> +  (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")
> +         (or (and block-start (* (or replace block-line)) block-end) replace)))
> +
> +  ;; RetractSpec = ( Version | "[" Version "," Version "]" ) newline .
> +  (define-peg-pattern range all
> +    (and (* WS) (ignore "[") version
> +         (* WS) (ignore ",") version (* WS) (ignore "]")))
> +  (define-peg-pattern retract all (and (or range version) EOL))
> +  (define-peg-pattern retract-top body
> +    (and (ignore "retract")
> +         (or (and block-start (* (or retract block-line)) block-end) retract)))
> +
> +  (define-peg-pattern go-mod body
> +    (* (and (* WS) (or go module require-top exclude-top replace-top
> +                       retract-top EOL unknown))))
> +
> +  (let ((tree (peg:tree (match-pattern go-mod content)))
> +        (keywords '(go module require replace exclude retract comment unknown)))
> +    (keyword-flatten keywords tree)))

Fun!

>  ;; Prevent inlining of this procedure, which is accessed by unit tests.
>  (set! parse-go.mod parse-go.mod)
>
> +(define (go.mod-directives go.mod directive)
> +  "Return the list of top-level directive bodies in GO.MOD matching the symbol
> +DIRECTIVE."
> +  (filter-map (match-lambda
> +                (((? (cut eq? <> directive) head) . rest) rest)
> +                (_ #f))
> +              go.mod))
> +
> +(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
> +      ;; TODO: Is this correct behavior? It's in the go.mod for a
> reason...

According to [1], it seems that yes: "replace directives only apply in
the main module's go.mod file and are ignored in other modules.", IIUC.

[1]  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))))
> +
> +    (match directive
> +      ((('original ('module-path module-path) . _) with . _)
> +       (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)))))))
> +
> +  (define (require directive requirements)
> +    (match directive
> +      ((('module-path module-path) ('version version) . _)
> +       (cons (list module-path version) requirements))))
> +
> +  (let* ((requires (go.mod-directives go.mod 'require))
> +         (replaces (go.mod-directives go.mod 'replace))
> +         (requirements (fold require '() requires)))
> +    (fold replace requirements replaces)))
> +
> +;; Prevent inlining of this procedure, which is accessed by unit tests.
> +(set! go.mod-requirements go.mod-requirements)
> +
>  (define-record-type <vcs>
>    (%make-vcs url-prefix root-regex type)
>    vcs?
> @@ -588,7 +599,7 @@ When VERSION is unspecified, the latest version available is used."
>  hint: use one of the following available versions ~a\n"
>                               version* available-versions))))
>           (content (fetch-go.mod goproxy module-path version*))
> -         (dependencies+versions (parse-go.mod content))
> +         (dependencies+versions (go.mod-requirements (parse-go.mod content)))
>           (dependencies (if pin-versions?
>                             dependencies+versions
>                             (map car dependencies+versions)))
> diff --git a/tests/go.scm b/tests/go.scm
> index b088ab50d2..b16965d941 100644
> --- a/tests/go.scm
> +++ b/tests/go.scm
> @@ -1,5 +1,6 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright  2021 Franois Joulaud <francois.joulaud@radiofrance.com>
> +;;; Copyright  2021 Sarah Morgensen <iskarian@mgsn.dev>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -31,6 +32,9 @@
>    #:use-module (srfi srfi-64)
>    #:use-module (web response))
>
> +(define go.mod-requirements
> +  (@@ (guix import go) go.mod-requirements))
> +
>  (define parse-go.mod
>    (@@ (guix import go) parse-go.mod))
>
> @@ -96,6 +100,40 @@ replace (
>
>  ")
>
> +(define fixture-go-mod-unparseable
> +  "module my/thing
> +go 1.12 // avoid feature X
> +require other/thing v1.0.2
> +// Security issue: CVE-XXXXX
> +exclude old/thing v1.2.3
> +new-directive another/thing yet-another/thing
> +replace (
> +        bad/thing v1.4.5 => good/thing v1.4.5
> +        // Unparseable
> +        bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0
> +)
> +")
> +
> +(define fixture-go-mod-retract
> +  "retract v0.9.1
> +
> +retract (
> +	v1.9.2
> +	[v1.0.0, v1.7.9]
> +)
> +")
> +
> +(define fixture-go-mod-strings
> +  "require `example.com/\"some-repo\"` v1.9.3
> +require (
> +        `example.com/\"another.repo\"` v1.0.0
> +        \"example.com/special!repo\" v9.3.1
> +)
> +replace \"example.com/\\\"some-repo\\\"\" => `launchpad.net/some-repo` v1.9.3
> +replace (
> +        \"example.com/\\\"another.repo\\\"\" => launchpad.net/another-repo v1.0.0
> +)
> +")
>
>
>  (define fixture-latest-for-go-check
> @@ -185,7 +223,7 @@ require github.com/kr/pretty v0.2.1
>      (string<? (car p1) (car p2)))
>    (test-equal name
>      (sort expected inf?)
> -    (sort ((@@ (guix import go) parse-go.mod) input) inf?)))
> +    (sort (go.mod-requirements (parse-go.mod input)) inf?)))
>
>  (testing-parse-mod "parse-go.mod-simple"
>                     '(("good/thing" "v1.4.5")
> @@ -221,6 +259,98 @@ require github.com/kr/pretty v0.2.1
>     ("github.com/go-check/check" "v0.0.0-20140225173054-eb6ee6f84d0a"))
>   fixture-go-mod-complete)
>
> +(test-equal "parse-go.mod: simple"
> +  `((module (module-path "my/thing"))
> +    (go (version "1.12"))
> +    (require (module-path "other/thing") (version "v1.0.2"))
> +    (require (module-path "new/thing/v2") (version "v2.3.4"))
> +    (exclude (module-path "old/thing") (version "v1.2.3"))
> +    (replace (original (module-path "bad/thing") (version "v1.4.5"))
> +      (with (module-path "good/thing") (version "v1.4.5"))))
> +  (parse-go.mod fixture-go-mod-simple))
> +
> +(test-equal "parse-go.mod: comments and unparseable lines"
> +  `((module (module-path "my/thing"))
> +    (go (version "1.12") (comment "avoid feature X"))
> +    (require (module-path "other/thing") (version "v1.0.2"))
> +    (comment "Security issue: CVE-XXXXX")
> +    (exclude (module-path "old/thing") (version "v1.2.3"))
> +    (unknown "new-directive another/thing yet-another/thing")
> +    (replace (original (module-path "bad/thing") (version "v1.4.5"))
> +      (with (module-path "good/thing") (version "v1.4.5")))
> +    (comment "Unparseable")
> +    (unknown "bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0"))
> +  (parse-go.mod fixture-go-mod-unparseable))
> +
> +(test-equal "parse-go.mod: retract"
> +  `((retract (version "v0.9.1"))
> +    (retract (version "v1.9.2"))
> +    (retract (range (version "v1.0.0") (version "v1.7.9"))))
> +  (parse-go.mod fixture-go-mod-retract))
> +
> +(test-equal "parse-go.mod: raw strings and quoted strings"
> +  `((require (module-path "example.com/\"some-repo\"") (version "v1.9.3"))
> +    (require (module-path "example.com/\"another.repo\"") (version "v1.0.0"))
> +    (require (module-path "example.com/special!repo") (version "v9.3.1"))
> +    (replace (original (module-path "example.com/\"some-repo\""))
> +      (with (module-path "launchpad.net/some-repo") (version "v1.9.3")))
> +    (replace (original (module-path "example.com/\"another.repo\""))
> +      (with (module-path "launchpad.net/another-repo") (version "v1.0.0"))))
> +  (parse-go.mod fixture-go-mod-strings))
> +
> +(test-equal "parse-go.mod: complete"
> +  `((module (module-path "M"))
> +    (go (version "1.13"))
> +    (replace (original (module-path "github.com/myname/myproject/myapi"))
> +      (with (file-path "./api")))
> +    (replace (original (module-path "github.com/mymname/myproject/thissdk"))
> +      (with (file-path "../sdk")))
> +    (replace (original (module-path "launchpad.net/gocheck"))
> +      (with (module-path "github.com/go-check/check")
> +            (version "v0.0.0-20140225173054-eb6ee6f84d0a")))
> +    (require (module-path "github.com/user/project")
> +             (version "v1.1.11"))
> +    (require (module-path "github.com/user/project/sub/directory")
> +             (version "v1.1.12"))
> +    (require (module-path "bitbucket.org/user/project")
> +             (version "v1.11.20"))
> +    (require (module-path "bitbucket.org/user/project/sub/directory")
> +             (version "v1.11.21"))
> +    (require (module-path "launchpad.net/project")
> +             (version "v1.1.13"))
> +    (require (module-path "launchpad.net/project/series")
> +             (version "v1.1.14"))
> +    (require (module-path "launchpad.net/project/series/sub/directory")
> +             (version "v1.1.15"))
> +    (require (module-path "launchpad.net/~user/project/branch")
> +             (version "v1.1.16"))
> +    (require (module-path "launchpad.net/~user/project/branch/sub/directory")
> +             (version "v1.1.17"))
> +    (require (module-path "hub.jazz.net/git/user/project")
> +             (version "v1.1.18"))
> +    (require (module-path "hub.jazz.net/git/user/project/sub/directory")
> +             (version "v1.1.19"))
> +    (require (module-path "k8s.io/kubernetes/subproject")
> +             (version "v1.1.101"))
> +    (require (module-path "one.example.com/abitrary/repo")
> +             (version "v1.1.111"))
> +    (require (module-path "two.example.com/abitrary/repo")
> +             (version "v0.0.2"))
> +    (require (module-path "quoted.example.com/abitrary/repo")
> +             (version "v0.0.2"))
> +    (replace (original (module-path "two.example.com/abitrary/repo"))
> +      (with (module-path "github.com/corp/arbitrary-repo")
> +            (version "v0.0.2")))
> +    (replace (original (module-path "golang.org/x/sys"))
> +      (with (module-path "golang.org/x/sys")
> +            (version "v0.0.0-20190813064441-fde4db37ae7a"))
> +      (comment "pinned to release-branch.go1.13"))
> +    (replace (original (module-path "golang.org/x/tools"))
> +      (with (module-path "golang.org/x/tools")
> +            (version "v0.0.0-20190821162956-65e3620a7ae7"))
> +      (comment "pinned to release-branch.go1.13")))
> +  (parse-go.mod fixture-go-mod-complete))
> +
>  ;;; End-to-end tests for (guix import go)
>  (define (mock-http-fetch testcase)
>    (lambda (url . rest)
>
> base-commit: 3ee0f170c8bd883728d8abb2c2e00f445c13f17d

LGTM!

Pushed with commit 793ba333c6, after fixing up some indents and
substituting the TODO for an explanatory comment with a reference to the
replace directive, and a few more cosmetic changes.

Thanks for the continued improvements to the Go importer :-).

Closing.

Maxim

[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Sarah Morgensen]

Hello,

Thanks for the review :) I just knew I wouldn't get that long commit
message right the first time!

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

>> (%go.mod-require-directive-rx)
>
> The above line can be removed, as it is duplicated below.
>
>> (%go.mod-replace-directive-rx): Remove unused variable.

Those are actually two distinct variables ("...-require-..." vs
"...-replace-..."), though I should've written "variables."

>> +  (define-peg-pattern string-or-ident body
>                                    ^
> Perhaps prefer the fully spelled out 'string-or-identifier' variable name.
>

Agree.

>> +(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
>> +      ;; TODO: Is this correct behavior? It's in the go.mod for a
>> reason...
>
> According to [1], it seems that yes: "replace directives only apply in
> the main module's go.mod file and are ignored in other modules.", IIUC.
>
> [1]  https://golang.org/ref/mod#go-mod-file-replace
>

My read of it is that if module A requires module B, replace directives
in B's go.mod are ignored, but A's go.mod can replace any module, direct
or indirect. For example, if module B hasn't been updated, but the
version of module C it depends on has a bug that affects it, module A
can use a replace in it's go.mod without requiring an upstream update of
module B. To be fair, this is usually handled by specifying the indirect
dependency with a specific version as a requirement in module A's
go.mod, but the replace should be valid as well. 

The reason it was skipped before, I think (if it was intentional), is
that since we only have the one version of a module in Guix at a time,
it's not necessary to make the indirect dependency explicitly an input,
so don't include it. On the other hand, if it *was* used to avoid a bug
in a version used by an indirect dependency, wouldn't we want to make
sure the Guix package was the fixed version? This is why I was
questioning whether leaving it out was correct. 

> Pushed with commit 793ba333c6, after fixing up some indents and
> substituting the TODO for an explanatory comment with a reference to the
> replace directive, and a few more cosmetic changes.

Thanks for the final touches.

> Thanks for the continued improvements to the Go importer :-).

Happy to help :) Originally, I was just trying to package a Go program,
and look what happened...

--
Sarah

[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Maxim Cournoyer]

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

Hello Sarah,

Sarah Morgensen <iskarian@mgsn.dev> writes:

[...]

>>> +(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
>>> +      ;; TODO: Is this correct behavior? It's in the go.mod for a
>>> reason...
>>
>> According to [1], it seems that yes: "replace directives only apply in
>> the main module's go.mod file and are ignored in other modules.", IIUC.
>>
>> [1]  https://golang.org/ref/mod#go-mod-file-replace
>>
>
> My read of it is that if module A requires module B, replace directives
> in B's go.mod are ignored, but A's go.mod can replace any module, direct
> or indirect. For example, if module B hasn't been updated, but the
> version of module C it depends on has a bug that affects it, module A
> can use a replace in it's go.mod without requiring an upstream update of
> module B. To be fair, this is usually handled by specifying the indirect
> dependency with a specific version as a requirement in module A's
> go.mod, but the replace should be valid as well. 

Thank you for explaining.  It makes sense.  So it seems that we should
honor the replacement for any dependency.

> The reason it was skipped before, I think (if it was intentional), is
> that since we only have the one version of a module in Guix at a time,
> it's not necessary to make the indirect dependency explicitly an input,
> so don't include it.

Sounds plausible!

> On the other hand, if it *was* used to avoid a bug
> in a version used by an indirect dependency, wouldn't we want to make
> sure the Guix package was the fixed version? This is why I was
> questioning whether leaving it out was correct. 

Now that I have a better understanding (I think!), I'd like to propose
the attached patch; it should make the replacement logic more in line
with the upstream specification.

Thanks for the discussion!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-import-go-Fix-replacement-directive-behavior.patch --]
[-- Type: text/x-patch, Size: 7213 bytes --]

From ab53cfccc4479b2fa069748c3c816376462168ae Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Tue, 3 Aug 2021 00:46:02 -0400
Subject: [PATCH] import: go: Fix replacement directive behavior.

Per upstream specifications, the 'replace' directive should be able to replace
any dependency found in its requirements (transitively).  It should also
discriminate based on the source module version, if present.

* guix/import/go.scm (go.mod-requirements): Handle version from the
replacement specification and no longer skip replacements to self.
* tests/go.scm (fixture-go-mod-simple): Add two new requirements and a replace
directive.
("parse-go.mod-simple"): Adjust expected result.
("parse-go.mod-complete"): Correct expected result.  Since nothing requires
the "github.com/go-check/check" module, the replacement directive should not
add it to the requirements.
("parse-go.mod: simple"): Adjust expected result.
---
 guix/import/go.scm | 61 +++++++++++++++++++++++++++++++---------------
 tests/go.scm       | 15 +++++++++---
 2 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 617a0d0e23..77eba56f57 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -46,6 +46,7 @@
   #:use-module (ice-9 receive)
   #:use-module (ice-9 regex)
   #:use-module (ice-9 textual-ports)
+  #:use-module ((rnrs base) #:select (assert))
   #:use-module ((rnrs io ports) #:select (call-with-port))
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-2)
@@ -348,31 +349,51 @@ 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))))
+    ;; Refer to https://golang.org/ref/mod#go-mod-file-replace for the
+    ;; specifications.
+    (define* (replace* module-path version
+                       to-module-path to-version
+                       requirements)
+      "Do the replacement, if a matching MODULE-PATH and VERSION is found in
+the requirements.  When TO-MODULE-PATH is #f, delete the MODULE-PATH from
+REQUIREMENTS.  Return the updated requirements."
+      (when to-module-path
+        (assert to-version))
+
+      (let* ((requirement-version (and=> (assoc-ref requirements module-path)
+                                         first))
+             (replacement-match? (if version
+                                     (and=> requirement-version
+                                            (cut string=? version <>))
+                                     requirement-version)))
+        (if replacement-match?
+            (begin
+              (if to-module-path
+                  (cons (list to-module-path to-version)
+                        (alist-delete module-path requirements))
+                  (alist-delete module-path requirements))) ;file-path case
+            requirements)))
 
     (match directive
-      ((('original ('module-path module-path) . _) with . _)
-       (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)))))))
-
-  (define (require directive requirements)
-    (match directive
-      ((('module-path module-path) ('version version) . _)
-       (cons (list module-path version) requirements))))
+      ((('original ('module-path module-path))
+        ('with ('file-path _)) . _)
+       (replace* module-path #f #f #f requirements))
+      ((('original ('module-path module-path) ('version version))
+        ('with ('file-path _)) . _)
+       (replace* module-path version #f #f requirements))
+      ((('original ('module-path module-path))
+        ('with ('module-path new-module-path) ('version new-version)) . _)
+       (replace* module-path #f new-module-path new-version requirements))
+      ((('original ('module-path module-path) ('version version))
+        ('with ('module-path new-module-path) ('version new-version)) . _)
+       (replace* module-path version new-module-path new-version requirements))))
 
   (let* ((requires (go.mod-directives go.mod 'require))
          (replaces (go.mod-directives go.mod 'replace))
-         (requirements (fold require '() requires)))
+         (requirements (map (match-lambda
+                              ((('module-path module-path) ('version version))
+                               (list module-path version)))
+                            requires)))
     (fold replace requirements replaces)))
 
 ;; Prevent inlining of this procedure, which is accessed by unit tests.
diff --git a/tests/go.scm b/tests/go.scm
index 6749f4585f..8c30f20c1e 100644
--- a/tests/go.scm
+++ b/tests/go.scm
@@ -43,8 +43,11 @@
 go 1.12
 require other/thing v1.0.2
 require new/thing/v2 v2.3.4
+require bad/thing v1.4.5
+require extra/thing v2
 exclude old/thing v1.2.3
 replace bad/thing v1.4.5 => good/thing v1.4.5
+replace extra/thing v1 => replaced/extra v1
 ")
 
 (define fixture-go-mod-with-block
@@ -220,7 +223,8 @@ require github.com/kr/pretty v0.2.1
     (sort (go.mod-requirements (parse-go.mod input)) inf?)))
 
 (testing-parse-mod "parse-go.mod-simple"
-                   '(("good/thing" "v1.4.5")
+                   '(("extra/thing" "v2")
+                     ("good/thing" "v1.4.5")
                      ("new/thing/v2" "v2.3.4")
                      ("other/thing" "v1.0.2"))
                    fixture-go-mod-simple)
@@ -249,8 +253,7 @@ require github.com/kr/pretty v0.2.1
    ("bitbucket.org/user/project" "v1.11.20")
    ("k8s.io/kubernetes/subproject" "v1.1.101")
    ("github.com/user/project/sub/directory" "v1.1.12")
-   ("github.com/user/project" "v1.1.11")
-   ("github.com/go-check/check" "v0.0.0-20140225173054-eb6ee6f84d0a"))
+   ("github.com/user/project" "v1.1.11"))
  fixture-go-mod-complete)
 
 (test-equal "parse-go.mod: simple"
@@ -258,9 +261,13 @@ require github.com/kr/pretty v0.2.1
     (go (version "1.12"))
     (require (module-path "other/thing") (version "v1.0.2"))
     (require (module-path "new/thing/v2") (version "v2.3.4"))
+    (require (module-path "bad/thing") (version "v1.4.5"))
+    (require (module-path "extra/thing") (version "v2"))
     (exclude (module-path "old/thing") (version "v1.2.3"))
     (replace (original (module-path "bad/thing") (version "v1.4.5"))
-      (with (module-path "good/thing") (version "v1.4.5"))))
+      (with (module-path "good/thing") (version "v1.4.5")))
+    (replace (original (module-path "extra/thing") (version "v1"))
+      (with (module-path "replaced/extra") (version "v1"))))
   (parse-go.mod fixture-go-mod-simple))
 
 (test-equal "parse-go.mod: comments and unparseable lines"
-- 
2.32.0


[-- Attachment #3: Type: text/plain, Size: 7 bytes --]


Maxim

[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Sarah Morgensen]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hello Sarah,
>
> Sarah Morgensen <iskarian@mgsn.dev> writes:
>
> [...]
>
>>>> +(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
>>>> +      ;; TODO: Is this correct behavior? It's in the go.mod for a
>>>> reason...
>>>
>>> According to [1], it seems that yes: "replace directives only apply in
>>> the main module's go.mod file and are ignored in other modules.", IIUC.
>>>
>>> [1]  https://golang.org/ref/mod#go-mod-file-replace
>>>
>>
>> My read of it is that if module A requires module B, replace directives
>> in B's go.mod are ignored, but A's go.mod can replace any module, direct
>> or indirect. For example, if module B hasn't been updated, but the
>> version of module C it depends on has a bug that affects it, module A
>> can use a replace in it's go.mod without requiring an upstream update of
>> module B. To be fair, this is usually handled by specifying the indirect
>> dependency with a specific version as a requirement in module A's
>> go.mod, but the replace should be valid as well. 
>
> Thank you for explaining.  It makes sense.  So it seems that we should
> honor the replacement for any dependency.
>

[...]

>
> Now that I have a better understanding (I think!), I'd like to propose
> the attached patch; it should make the replacement logic more in line
> with the upstream specification.

If I'm reading your patch correctly, the proposed behavior is that
replace directives have an effect iff the module (with matching version,
if specified) is present in the requirements list, yes?

This indeed how it should work, *if* the requirements list was populated
with the requirements from dependencies first (like Go does). However,
the way the importer is structured right now, we only deal with a single
go.mod at a time [0]. So with this proposed behavior, if module A has a
replace directive for module C => module D, but module C is not listed
in module A's requirements, the replacement would not be processed.

So the current behavior for such replacements is to unconditionally
perform the replacement, adding module D even if module C is not in the
requirements. (A "module C => module C" replacement would not be
performed.)

I think the best we can do is to use the greatest referenced version of
any module and remove replaced modules, which almost satisfied minimal
version selection [1]:

Case 1:
  module A
  require C v1
  replace C v1 => C v2

-> requirements: (("C" "v2"))

Case 2:
  module A
  require C v1
  replace C v0.96 => C v0.99c

-> requirements: (("C" "v1"))

Case 3:
  module A
  require C v1
  replace C v1.2 => C v1.1

-> requirements: (("C" "v1.1"))

Case 4:
  module A
  replace <anything> => C v1.1
  
-> requirements: (("C" "v1.1"))

Case 5:
  module A
  require D v3.5
  replace D => F v2

-> requirements: (("F" "v2"))

The only wrench in the works is this:

Case 4:
  module A
  require B v1
  replace C v1.2 => C v1.1

  module B
  require C v1.2

In this case, the importer would see that the greatest version of C
required (and also latest available) is v1.2, and package only C
v1.2. Use of --pin-versions would be required, but insufficient, to
address this issue. In our current build system, I believe that even if
there are two versions of module C required by a package, one shadows
the other, but this means that essentially the replace is ignored.

However, if we attempt to only keep the "best version" of anything
packaged at a time, this shouldn't really be an issue, right?

It also looks like for go 1.17 and above, the "go.mod file includes an
explicit require directive for each modulle that provides any package
transitively imported by a package or test in the main module." [2]
(They finally realized the mess they made!) This should eventually make
things easier in the future.

--
Sarah

[0] It would be difficult to process all dependent go.mods, because we
    might not be doing a recursive import, or packages may already be in
    Guix, in which case we wouldn't be fetching their go.mods. I also
    think it would make things more brittle, as all it would take is an
    issue with a single dependency to break it.

[1] <https://golang.org/ref/mod/#minimal-version-selection> Minimal
    version selection

[2] <https://golang.org/ref/mod/#go-mod-file-go> go directive


>
> Thanks for the discussion!
>
>>From ab53cfccc4479b2fa069748c3c816376462168ae Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Tue, 3 Aug 2021 00:46:02 -0400
> Subject: [PATCH] import: go: Fix replacement directive behavior.
>
> Per upstream specifications, the 'replace' directive should be able to replace
> any dependency found in its requirements (transitively).  It should also
> discriminate based on the source module version, if present.
>
> * guix/import/go.scm (go.mod-requirements): Handle version from the
> replacement specification and no longer skip replacements to self.
> * tests/go.scm (fixture-go-mod-simple): Add two new requirements and a replace
> directive.
> ("parse-go.mod-simple"): Adjust expected result.
> ("parse-go.mod-complete"): Correct expected result.  Since nothing requires
> the "github.com/go-check/check" module, the replacement directive should not
> add it to the requirements.
> ("parse-go.mod: simple"): Adjust expected result.
> ---
>  guix/import/go.scm | 61 +++++++++++++++++++++++++++++++---------------
>  tests/go.scm       | 15 +++++++++---
>  2 files changed, 52 insertions(+), 24 deletions(-)
>
> diff --git a/guix/import/go.scm b/guix/import/go.scm
> index 617a0d0e23..77eba56f57 100644
> --- a/guix/import/go.scm
> +++ b/guix/import/go.scm
> @@ -46,6 +46,7 @@
>    #:use-module (ice-9 receive)
>    #:use-module (ice-9 regex)
>    #:use-module (ice-9 textual-ports)
> +  #:use-module ((rnrs base) #:select (assert))
>    #:use-module ((rnrs io ports) #:select (call-with-port))
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-2)
> @@ -348,31 +349,51 @@ 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))))
> +    ;; Refer to https://golang.org/ref/mod#go-mod-file-replace for the
> +    ;; specifications.
> +    (define* (replace* module-path version
> +                       to-module-path to-version
> +                       requirements)
> +      "Do the replacement, if a matching MODULE-PATH and VERSION is found in
> +the requirements.  When TO-MODULE-PATH is #f, delete the MODULE-PATH from
> +REQUIREMENTS.  Return the updated requirements."
> +      (when to-module-path
> +        (assert to-version))
> +
> +      (let* ((requirement-version (and=> (assoc-ref requirements module-path)
> +                                         first))
> +             (replacement-match? (if version
> +                                     (and=> requirement-version
> +                                            (cut string=? version <>))
> +                                     requirement-version)))
> +        (if replacement-match?
> +            (begin
> +              (if to-module-path
> +                  (cons (list to-module-path to-version)
> +                        (alist-delete module-path requirements))
> +                  (alist-delete module-path requirements))) ;file-path case
> +            requirements)))
>  
>      (match directive
> -      ((('original ('module-path module-path) . _) with . _)
> -       (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)))))))
> -
> -  (define (require directive requirements)
> -    (match directive
> -      ((('module-path module-path) ('version version) . _)
> -       (cons (list module-path version) requirements))))
> +      ((('original ('module-path module-path))
> +        ('with ('file-path _)) . _)
> +       (replace* module-path #f #f #f requirements))
> +      ((('original ('module-path module-path) ('version version))
> +        ('with ('file-path _)) . _)
> +       (replace* module-path version #f #f requirements))
> +      ((('original ('module-path module-path))
> +        ('with ('module-path new-module-path) ('version new-version)) . _)
> +       (replace* module-path #f new-module-path new-version requirements))
> +      ((('original ('module-path module-path) ('version version))
> +        ('with ('module-path new-module-path) ('version new-version)) . _)
> +       (replace* module-path version new-module-path new-version requirements))))
>  
>    (let* ((requires (go.mod-directives go.mod 'require))
>           (replaces (go.mod-directives go.mod 'replace))
> -         (requirements (fold require '() requires)))
> +         (requirements (map (match-lambda
> +                              ((('module-path module-path) ('version version))
> +                               (list module-path version)))
> +                            requires)))
>      (fold replace requirements replaces)))
>  
>  ;; Prevent inlining of this procedure, which is accessed by unit tests.
> diff --git a/tests/go.scm b/tests/go.scm
> index 6749f4585f..8c30f20c1e 100644
> --- a/tests/go.scm
> +++ b/tests/go.scm
> @@ -43,8 +43,11 @@
>  go 1.12
>  require other/thing v1.0.2
>  require new/thing/v2 v2.3.4
> +require bad/thing v1.4.5
> +require extra/thing v2
>  exclude old/thing v1.2.3
>  replace bad/thing v1.4.5 => good/thing v1.4.5
> +replace extra/thing v1 => replaced/extra v1
>  ")
>  
>  (define fixture-go-mod-with-block
> @@ -220,7 +223,8 @@ require github.com/kr/pretty v0.2.1
>      (sort (go.mod-requirements (parse-go.mod input)) inf?)))
>  
>  (testing-parse-mod "parse-go.mod-simple"
> -                   '(("good/thing" "v1.4.5")
> +                   '(("extra/thing" "v2")
> +                     ("good/thing" "v1.4.5")
>                       ("new/thing/v2" "v2.3.4")
>                       ("other/thing" "v1.0.2"))
>                     fixture-go-mod-simple)
> @@ -249,8 +253,7 @@ require github.com/kr/pretty v0.2.1
>     ("bitbucket.org/user/project" "v1.11.20")
>     ("k8s.io/kubernetes/subproject" "v1.1.101")
>     ("github.com/user/project/sub/directory" "v1.1.12")
> -   ("github.com/user/project" "v1.1.11")
> -   ("github.com/go-check/check" "v0.0.0-20140225173054-eb6ee6f84d0a"))
> +   ("github.com/user/project" "v1.1.11"))
>   fixture-go-mod-complete)
>  
>  (test-equal "parse-go.mod: simple"
> @@ -258,9 +261,13 @@ require github.com/kr/pretty v0.2.1
>      (go (version "1.12"))
>      (require (module-path "other/thing") (version "v1.0.2"))
>      (require (module-path "new/thing/v2") (version "v2.3.4"))
> +    (require (module-path "bad/thing") (version "v1.4.5"))
> +    (require (module-path "extra/thing") (version "v2"))
>      (exclude (module-path "old/thing") (version "v1.2.3"))
>      (replace (original (module-path "bad/thing") (version "v1.4.5"))
> -      (with (module-path "good/thing") (version "v1.4.5"))))
> +      (with (module-path "good/thing") (version "v1.4.5")))
> +    (replace (original (module-path "extra/thing") (version "v1"))
> +      (with (module-path "replaced/extra") (version "v1"))))
>    (parse-go.mod fixture-go-mod-simple))
>  
>  (test-equal "parse-go.mod: comments and unparseable lines"

[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Maxim Cournoyer]

Hello Sarah,

Sarah Morgensen <iskarian@mgsn.dev> writes:

[...]

>> Now that I have a better understanding (I think!), I'd like to propose
>> the attached patch; it should make the replacement logic more in line
>> with the upstream specification.
>
> If I'm reading your patch correctly, the proposed behavior is that
> replace directives have an effect iff the module (with matching version,
> if specified) is present in the requirements list, yes?
>
> This indeed how it should work, *if* the requirements list was populated
> with the requirements from dependencies first (like Go does). However,
> the way the importer is structured right now, we only deal with a single
> go.mod at a time [0]. So with this proposed behavior, if module A has a
> replace directive for module C => module D, but module C is not listed
> in module A's requirements, the replacement would not be processed.
>
> So the current behavior for such replacements is to unconditionally
> perform the replacement, adding module D even if module C is not in the
> requirements. (A "module C => module C" replacement would not be
> performed.)

Oh, indeed!  So sticking to the upstream spec is not a good fit for our
current design choices, where we only deal with the data of a single
go.mod at a time.  I could 'feel' something was not right, but failed to
see it; thanks for pointing it out.

> I think the best we can do is to use the greatest referenced version of
> any module and remove replaced modules, which almost satisfied minimal
> version selection [1]:

That's what is currently being done, right?

So, I was going to propose to just add a  comment, like so:

--8<---------------cut here---------------start------------->8---
modified   guix/import/go.scm
@@ -347,12 +347,16 @@ 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).
+      ;; Since only one go.mod is considered at a time and hence not all the
+      ;; transitive requirements are known, we honor all the replacements,
+      ;; contrary to the upstream specification where only dependencies
+      ;; actually *required* get replaced.  Also, do not allow version updates
+      ;; for indirect dependencies, as other modules know best about their
+      ;; requirements.
       (if (and (equal? module-path (first new-requirement))
                (not (assoc-ref requirements module-path)))
           requirements
           (cons new-requirement (alist-delete module-path requirements))))
--8<---------------cut here---------------end--------------->8---

But the last sentence I'm unsure about, as while it would not update a
same named module replacement that is not in the requirements (thus an
indirect dependency, correct?), it *would* replace a module that had its
name changed.  Compare for example in tests/go.scm, for the
fixture-go-mod-complete, the two indirect dependencies:

replace launchpad.net/gocheck => github.com/go-check/check
v0.0.0-20140225173054-eb6ee6f84d0a is honored, but

golang.org/x/sys => golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a
is not (because the module name is the same).

What is the reasoning behind this?  Can you expound/correct my draft
comment so that it accurately describes the desired outcome?


> Case 1:
>   module A
>   require C v1
>   replace C v1 => C v2
>
> -> requirements: (("C" "v2"))
>
> Case 2:
>   module A
>   require C v1
>   replace C v0.96 => C v0.99c
>
> -> requirements: (("C" "v1"))
>
> Case 3:
>   module A
>   require C v1
>   replace C v1.2 => C v1.1
>
> -> requirements: (("C" "v1.1"))
>
> Case 4:
>   module A
>   replace <anything> => C v1.1
>   
> -> requirements: (("C" "v1.1"))
>
> Case 5:
>   module A
>   require D v3.5
>   replace D => F v2
>
> -> requirements: (("F" "v2"))
>
> The only wrench in the works is this:
>
> Case 4:
>   module A
>   require B v1
>   replace C v1.2 => C v1.1
>
>   module B
>   require C v1.2
>
> In this case, the importer would see that the greatest version of C
> required (and also latest available) is v1.2, and package only C
> v1.2. Use of --pin-versions would be required, but insufficient, to
> address this issue. In our current build system, I believe that even if
> there are two versions of module C required by a package, one shadows
> the other, but this means that essentially the replace is ignored.
>
> However, if we attempt to only keep the "best version" of anything
> packaged at a time, this shouldn't really be an issue, right?

It might cause problems if the great version we carry is not a backward
compatible with the replacement requested ?  But then we collapse
everything in the GOPATH currently, so it could break something else if
we honored it.  I believe with newer packages using Go modules they can
have their own sand-boxed dependency graph, but we're not there yet (and
perhaps don't want to go there ? :-)).

> It also looks like for go 1.17 and above, the "go.mod file includes an
> explicit require directive for each modulle that provides any package
> transitively imported by a package or test in the main module." [2]
> (They finally realized the mess they made!) This should eventually make
> things easier in the future.

I see!  So we'd have all the information at hand even we consider only a
single go.mod at a time.

I'm withdrawing my patch for the time being; it may be of use if we'd
like to refine the replacement strategy for the --pin-versions mode, but
for now what we have seems sufficient (especially since there will be
changes in Go 1.17 as you mentioned).

Thanks,

Maxim

[bug#49602] [PATCH] import: go: Upgrade go.mod parser.

[Sarah Morgensen]

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

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

[...]

>
>> I think the best we can do is to use the greatest referenced version of
>> any module and remove replaced modules, which almost satisfied minimal
>> version selection [1]:
>
> That's what is currently being done, right?

Not quite, as you explain below (same-named modules are currently not
being replaced).

>
> So, I was going to propose to just add a  comment, like so:
>
> modified   guix/import/go.scm
> @@ -347,12 +347,16 @@ 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).
> +      ;; Since only one go.mod is considered at a time and hence not all the
> +      ;; transitive requirements are known, we honor all the replacements,
> +      ;; contrary to the upstream specification where only dependencies
> +      ;; actually *required* get replaced.  Also, do not allow version updates
> +      ;; for indirect dependencies, as other modules know best about their
> +      ;; requirements.
>        (if (and (equal? module-path (first new-requirement))
>                 (not (assoc-ref requirements module-path)))
>            requirements
>            (cons new-requirement (alist-delete module-path requirements))))
>
> But the last sentence I'm unsure about, as while it would not update a
> same named module replacement that is not in the requirements (thus an
> indirect dependency, correct?), it *would* replace a module that had its
> name changed.  Compare for example in tests/go.scm, for the
> fixture-go-mod-complete, the two indirect dependencies:
>
> replace launchpad.net/gocheck => github.com/go-check/check
> v0.0.0-20140225173054-eb6ee6f84d0a is honored, but
>
> golang.org/x/sys => golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a
> is not (because the module name is the same).
>
> What is the reasoning behind this?  Can you expound/correct my draft
> comment so that it accurately describes the desired outcome?

As I did not write the original Go importer, I can only guess, as I did
earlier:

> The reason it was skipped before, I think (if it was intentional), is
> that since we only have the one version of a module in Guix at a time,
> it's not necessary to make the indirect dependency explicitly an input,
> so don't include it. On the other hand, if it *was* used to avoid a bug
> in a version used by an indirect dependency, wouldn't we want to make
> sure the Guix package was the fixed version? This is why I was
> questioning whether leaving it out was correct. 

As I suggested there, I would honor the same-named replacement. I've
attached an updated draft comment and code to match. I got carried away,
so there's a second one which riffs off of your previous version check
patch.

(One solution, I think, would be to discriminate between direct and
indirect dependencies, and only include an indirect dependency in inputs
if its version is different than the one already used. But this would
require restructuring the importer for arguably little benefit.)

[...]

>> In this case, the importer would see that the greatest version of C
>> required (and also latest available) is v1.2, and package only C
>> v1.2. Use of --pin-versions would be required, but insufficient, to
>> address this issue. In our current build system, I believe that even if
>> there are two versions of module C required by a package, one shadows
>> the other, but this means that essentially the replace is ignored.
>>
>> However, if we attempt to only keep the "best version" of anything
>> packaged at a time, this shouldn't really be an issue, right?
>
> It might cause problems if the great version we carry is not a backward
> compatible with the replacement requested ?  But then we collapse
> everything in the GOPATH currently, so it could break something else if
> we honored it.  I believe with newer packages using Go modules they can
> have their own sand-boxed dependency graph, but we're not there yet (and
> perhaps don't want to go there ? :-)).

*sigh* Yes, this is frustrating. IIUC the Guix way is to have two
separate packages/variables if there is a version incompatibility. But
like you say, because of the way the Go build system uses GOPATH, only
one version of any module is used at a time. We may indeed want to go
down the path of recreating $GOPATH/pkg/mod (or even a GOPROXY mock?),
but I don't think it will be easy, and I think there will be issues with
Guix not having whatever exact version some module wants.

>
>> It also looks like for go 1.17 and above, the "go.mod file includes an
>> explicit require directive for each modulle that provides any package
>> transitively imported by a package or test in the main module." [2]
>> (They finally realized the mess they made!) This should eventually make
>> things easier in the future.
>
> I see!  So we'd have all the information at hand even we consider only a
> single go.mod at a time.
>
> I'm withdrawing my patch for the time being; it may be of use if we'd
> like to refine the replacement strategy for the --pin-versions mode, but
> for now what we have seems sufficient (especially since there will be
> changes in Go 1.17 as you mentioned).
>
> Thanks,
>
> Maxim

--
Sarah


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-import-go-Allow-version-pinning-for-indirect-depende.patch --]
[-- Type: text/x-patch, Size: 1861 bytes --]

From e32521de5b0badf8172058364611db147d562522 Mon Sep 17 00:00:00 2001
Message-Id: <e32521de5b0badf8172058364611db147d562522.1628223778.git.iskarian@mgsn.dev>
From: Sarah Morgensen <iskarian@mgsn.dev>
Date: Thu, 5 Aug 2021 21:15:26 -0700
Subject: [PATCH 1/2] import: go: Allow version pinning for indirect
 dependencies.

---
 guix/import/go.scm | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 617a0d0e23..5c1a251677 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -349,12 +349,15 @@ DIRECTIVE."
   "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))))
+      ;; Since only one go.mod is considered at a time and hence not all the
+      ;; transitive requirements are known, we honor all the replacements,
+      ;; contrary to the upstream specification where only dependencies actually
+      ;; *required* get replaced.
+      ;;
+      ;; Notably, allow version pinning/updating for indirect dependencies.  It
+      ;; is rare in practice, may be useful with --pin-versions, and at worst
+      ;; adds an extra direct input (which would be transitively included anyway).
+      (cons new-requirement (alist-delete module-path requirements)))
 
     (match directive
       ((('original ('module-path module-path) . _) with . _)

base-commit: d0d3bcc615f1d521ea60a8b2e085767e0adb05b6
-- 
2.31.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-import-go-Check-version-for-replacements.patch --]
[-- Type: text/x-patch, Size: 4575 bytes --]

From c1c898c1df14f931be31151713ec4204adee04eb Mon Sep 17 00:00:00 2001
Message-Id: <c1c898c1df14f931be31151713ec4204adee04eb.1628223778.git.iskarian@mgsn.dev>
In-Reply-To: <e32521de5b0badf8172058364611db147d562522.1628223778.git.iskarian@mgsn.dev>
References: <e32521de5b0badf8172058364611db147d562522.1628223778.git.iskarian@mgsn.dev>
From: Sarah Morgensen <iskarian@mgsn.dev>
Date: Thu, 5 Aug 2021 21:19:58 -0700
Subject: [PATCH 2/2] import: go: Check version for replacements.

---
 guix/import/go.scm | 48 +++++++++++++++++++++++++++++-----------------
 tests/go.scm       |  2 ++
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 5c1a251677..04546cb9e9 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -348,25 +348,37 @@ 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)
-      ;; Since only one go.mod is considered at a time and hence not all the
-      ;; transitive requirements are known, we honor all the replacements,
-      ;; contrary to the upstream specification where only dependencies actually
-      ;; *required* get replaced.
-      ;;
-      ;; Notably, allow version pinning/updating for indirect dependencies.  It
-      ;; is rare in practice, may be useful with --pin-versions, and at worst
-      ;; adds an extra direct input (which would be transitively included anyway).
-      (cons new-requirement (alist-delete module-path requirements)))
-
+    ;; Since only one go.mod is considered at a time and hence not all the
+    ;; transitive requirements are known, we honor all the replacements,
+    ;; contrary to the upstream specification where only dependencies actually
+    ;; *required* get replaced.  However, if a version is specified and the
+    ;; module is required in this go.mod, the version must match in order to
+    ;; replace.
+    ;;
+    ;; Notably, allow version pinning/updating for indirect dependencies.  It
+    ;; is rare in practice, may be useful with --pin-versions, and at worst
+    ;; adds an extra direct input (which would be transitively included anyway).
+    (define (replace* module-path version
+                      to-module-path to-version
+                      requirements)
+      "Perform the replacement unless VERSION is non-false, MODULE-PATH is found
+in REQUIREMENTS, and its version does not match VERSION. Return the updated
+REQUIREMENTS."
+      (let* ((current-version (and=> (assoc-ref requirements module-path) first))
+             (version-matches? (equal? version current-version)))
+        (if (and version current-version (not version-matches?))
+            requirements
+            (cons (list to-module-path to-version)
+                  (alist-delete module-path requirements)))))
     (match directive
-      ((('original ('module-path module-path) . _) with . _)
-       (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)))))))
+      ((('original ('module-path module-path) . _) ('with ('file-path _)) . _)
+       (alist-delete module-path requirements))
+      ((('original ('module-path module-path) ('version version) . _)
+        ('with ('module-path new-module-path) ('version new-version) . _) . _)
+       (replace* module-path version new-module-path new-version requirements))
+      ((('original ('module-path module-path) . _)
+        ('with ('module-path new-module-path) ('version new-version) . _) . _)
+       (replace* module-path #f new-module-path new-version requirements))))
 
   (define (require directive requirements)
     (match directive
diff --git a/tests/go.scm b/tests/go.scm
index 6749f4585f..ec92e3fce4 100644
--- a/tests/go.scm
+++ b/tests/go.scm
@@ -238,6 +238,8 @@ require github.com/kr/pretty v0.2.1
  '(("github.com/corp/arbitrary-repo" "v0.0.2")
    ("quoted.example.com/abitrary/repo" "v0.0.2")
    ("one.example.com/abitrary/repo" "v1.1.111")
+   ("golang.org/x/sys" "v0.0.0-20190813064441-fde4db37ae7a")
+   ("golang.org/x/tools" "v0.0.0-20190821162956-65e3620a7ae7")
    ("hub.jazz.net/git/user/project/sub/directory" "v1.1.19")
    ("hub.jazz.net/git/user/project" "v1.1.18")
    ("launchpad.net/~user/project/branch/sub/directory" "v1.1.17")
-- 
2.31.1