GNU bug report logs - #49602
[PATCH] import: go: Upgrade go.mod parser.

Previous Next

Full log

View this message in rfc822 format

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Sarah Morgensen <iskarian <at> mgsn.dev>
Cc: 49602 <at> debbugs.gnu.org
Subject: [bug#49602] [PATCH] import: go: Upgrade go.mod parser.
Date: Wed, 04 Aug 2021 22:04:33 -0400

Hello Sarah,

Sarah Morgensen <iskarian <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.