[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"