From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id OId6JU9RCWGcUgAAgWs5BA (envelope-from ) for ; Tue, 03 Aug 2021 16:23:11 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id cE4WIU9RCWE2KAAAB5/wlQ (envelope-from ) for ; Tue, 03 Aug 2021 14:23:11 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 1436518503 for ; Tue, 3 Aug 2021 16:23:11 +0200 (CEST) Received: from localhost ([::1]:50768 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mAvK1-0006jN-Vv for larch@yhetil.org; Tue, 03 Aug 2021 10:23:10 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58758) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mAvJu-0006if-Kz for guix-patches@gnu.org; Tue, 03 Aug 2021 10:23:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:57854) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mAvJu-0007zg-Ev for guix-patches@gnu.org; Tue, 03 Aug 2021 10:23:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mAvJu-0004oj-8u for guix-patches@gnu.org; Tue, 03 Aug 2021 10:23:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#49602] [PATCH] import: go: Upgrade go.mod parser. Resent-From: Maxim Cournoyer Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Tue, 03 Aug 2021 14:23:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 49602 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Sarah Morgensen Cc: 49602@debbugs.gnu.org Received: via spool by 49602-submit@debbugs.gnu.org id=B49602.162800052518443 (code B ref 49602); Tue, 03 Aug 2021 14:23:02 +0000 Received: (at 49602) by debbugs.gnu.org; 3 Aug 2021 14:22:05 +0000 Received: from localhost ([127.0.0.1]:41167 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mAvIy-0004nO-N2 for submit@debbugs.gnu.org; Tue, 03 Aug 2021 10:22:05 -0400 Received: from mail-qv1-f49.google.com ([209.85.219.49]:35565) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mAvIw-0004mq-Ub for 49602@debbugs.gnu.org; Tue, 03 Aug 2021 10:22:03 -0400 Received: by mail-qv1-f49.google.com with SMTP id 3so10648106qvd.2 for <49602@debbugs.gnu.org>; Tue, 03 Aug 2021 07:22:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=O10Iehtk/v+IRp5pzxp/Lk/Xm9j4fvzf95qIYdHtDwM=; b=QLTvBMLiTE0Twc/Y12gTHoCMVzKyf/38Qbh1BSyoKoEOza+BuuQtcXa4MZ6k5HHd+q PyySEtRWVNIB+EoiVZEYnp0v1InLsUdWTnfrEON5tF6TbTcN9z2G9PlrVD6X1+gaozDn 4SNAxTLFCzHMFm5Z6yao/OdLciiTdUv+g3i1J+dC2nd980UY3SU0o9JNCTKrOS1V09Cs 7eHWttjo+WUtyY+SwX302G0lccxkEKr99qmTqhUKjtjq31ypUxQosI79HkOF2LJOS7Kz xjCjE0msJShavwBpGQAQC6vU6+QqQLNeRgGCdw8toZIy2fx6VMfgnMjNmPeAQSghrfw0 LDTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=O10Iehtk/v+IRp5pzxp/Lk/Xm9j4fvzf95qIYdHtDwM=; b=P1W4Q6zsGICdsoRhhiAFikcQYZ2P+4Sf09hCGldLCoVdThiiWzWUgTaQNGAtf+OVlp PGlSaIQM5CIhgKyu/0r/F1SD6SwF3NGpqvqaX27FLC6XSWcOu8a9SXQmNI0Y4Ng4oK8F rq7NZnf8m+z/p1OiYszBv+ZSjYsCZFEYs+7O16+WSMgCq+pwGXYbAZSjeKf5YCr6ELVa cUv5D+0cmoMleaXL3K9ZKjkUrompvZ8LN/92LvLbdZUUFksmO2aOgGeXXc5iJOmgWXc9 k8yBnPYH+uf1X2FXMQWounuh3EZyZGCmnCedf8oI4dGwv1Zw3huuwBqWlXUQN7/PqCQT eoEQ== X-Gm-Message-State: AOAM533D21Cd/tP1bWkwKeOvVNR1VstDtU0ElXz5dqrWikIgzP0QR2X6 D5pSoOgotIuRU9Qvh29dNqkN4YfHZArF7A== X-Google-Smtp-Source: ABdhPJzzdoVTzRatFI3tN6zD7pzLCVEuU6tSwXUqJ9nXSLR87NuxcP9Yla7+vxsGJ1sPGQdUil92QQ== X-Received: by 2002:a0c:f445:: with SMTP id h5mr6901890qvm.16.1628000516551; Tue, 03 Aug 2021 07:21:56 -0700 (PDT) Received: from hurd (dsl-10-129-132.b2b2c.ca. [72.10.129.132]) by smtp.gmail.com with ESMTPSA id m19sm6061877qtx.84.2021.08.03.07.21.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Aug 2021 07:21:55 -0700 (PDT) From: Maxim Cournoyer References: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> <87tuksjh68.fsf@gmail.com> <86eebvt2o8.fsf@mgsn.dev> Date: Tue, 03 Aug 2021 10:21:53 -0400 In-Reply-To: <86eebvt2o8.fsf@mgsn.dev> (Sarah Morgensen's message of "Sun, 18 Jul 2021 20:15:03 -0700") Message-ID: <87mtpy7gn2.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1628000591; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post: dkim-signature; bh=O10Iehtk/v+IRp5pzxp/Lk/Xm9j4fvzf95qIYdHtDwM=; b=RV9+BAj1n+LRk8xo4kb02Gq9rIi0kJIjCOhrjgEhDagfIgZDTbp7+3Vt6ze07TVgkgANNi i9bP33gMeFPvAsMjFEcGaPlMvHiKeM6qXebRLN286yINLaYya/1n7AA9KkLyDzwPrTxuje sIHy+X+geUjKpBIStWuKzR5JptXebrcOMLobv0/bduiDMi+rn1/ypyhz6oBVfb+Sd2Nf1+ mqbSqVhBln/RBtJEWuhxHgX5Tu0ny85NebcUE+bGBigL9wdxGODjXRXSct/zuDVS0I18Er Pf5N6ElwAZNbGQyrV01weajPL6aY6G6EmbplLBQ2uYwwDEPO1ch6oKSpFgJxVA== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1628000591; a=rsa-sha256; cv=none; b=LBP2/gAXPOCTrt6A3GsGpjRd8s42jEP2+6V7enOsa/d/V1mqPISku960ahcmMeEq/g1u35 hkvq2isgQAAhNHnXoylcC2iul5b4ocg8BkvzVy2sAtdsh7c7iJlZwg2KnMTZCCtgydfjlq d21EbHit3/4W0U6lkFd01UBYnt1aPO//Q+1a0Oz7m0GlHSAgqRP57lf0CHGbhayL2kBxFN zn3XvGmz5K/k+eLzAmVz89MFaThCG7sEiRTYArsMRFLflJfRJWLRlsKqg4A+1vHeMa65aF /eX3UJ3jyByGONN60JJ9t4P5hEs+Ib0qnFqu58D72QYhnp7VVU9GvSJUDyTvmw== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20161025 header.b=QLTvBMLi; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Spam-Score: 0.18 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20161025 header.b=QLTvBMLi; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Queue-Id: 1436518503 X-Spam-Score: 0.18 X-Migadu-Scanner: scn1.migadu.com X-TUID: k78gdCG5x09J --=-=-= Content-Type: text/plain Hello Sarah, Sarah Morgensen 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! --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-import-go-Fix-replacement-directive-behavior.patch >From ab53cfccc4479b2fa069748c3c816376462168ae Mon Sep 17 00:00:00 2001 From: Maxim Cournoyer 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 --=-=-= Content-Type: text/plain Maxim --=-=-=--