[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