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

[Sarah Morgensen]

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

>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

>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