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 6I2zCD7eCmF+UQAAgWs5BA (envelope-from ) for ; Wed, 04 Aug 2021 20:36:46 +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 YOcnBD7eCmEZLwAAB5/wlQ (envelope-from ) for ; Wed, 04 Aug 2021 18:36:46 +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 8F70C12EC7 for ; Wed, 4 Aug 2021 20:36:45 +0200 (CEST) Received: from localhost ([::1]:56114 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mBLkx-0005Um-OY for larch@yhetil.org; Wed, 04 Aug 2021 14:36:43 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:49552) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBLSs-0005Rb-UR for guix-patches@gnu.org; Wed, 04 Aug 2021 14:18:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:33548) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mBLSs-0002dX-Lv for guix-patches@gnu.org; Wed, 04 Aug 2021 14:18:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mBLSs-0004PJ-8j for guix-patches@gnu.org; Wed, 04 Aug 2021 14:18:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#49602] [PATCH] import: go: Upgrade go.mod parser. Resent-From: Sarah Morgensen Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Wed, 04 Aug 2021 18:18: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: Maxim Cournoyer Cc: 49602@debbugs.gnu.org Received: via spool by 49602-submit@debbugs.gnu.org id=B49602.162810105416893 (code B ref 49602); Wed, 04 Aug 2021 18:18:02 +0000 Received: (at 49602) by debbugs.gnu.org; 4 Aug 2021 18:17:34 +0000 Received: from localhost ([127.0.0.1]:45094 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mBLSQ-0004OO-48 for submit@debbugs.gnu.org; Wed, 04 Aug 2021 14:17:34 -0400 Received: from out2.migadu.com ([188.165.223.204]:38983) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mBLSM-0004OE-Lj for 49602@debbugs.gnu.org; Wed, 04 Aug 2021 14:17:32 -0400 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mgsn.dev; s=key1; t=1628101048; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=1bz5DY6DKzyFR20C+u/EV0CB9dTnh7VTtNFNvLA4KXw=; b=S7e8bXsNu1lOOUlANUAADvG09iCDEsoEPWnEXvfHhW2hMvqDBh/tekTm8tVqMFq7NyMkAW BTkanL25XG4RuSQLpCPb+P5gZDTsRsV487c/An+RI+jsPEuDEWuWrA5+7Alyqbowr1rdLq iy3820MTt/IZmff80QQjJHl1L6FkIPc= From: Sarah Morgensen References: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> <87tuksjh68.fsf@gmail.com> <86eebvt2o8.fsf@mgsn.dev> <87mtpy7gn2.fsf@gmail.com> Date: Wed, 04 Aug 2021 11:17:25 -0700 In-Reply-To: <87mtpy7gn2.fsf@gmail.com> (Maxim Cournoyer's message of "Tue, 03 Aug 2021 10:21:53 -0400") Message-ID: <86v94l2hxm.fsf_-_@mgsn.dev> MIME-Version: 1.0 Content-Type: text/plain X-Migadu-Auth-User: iskarian@mgsn.dev 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=1628102205; 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=1bz5DY6DKzyFR20C+u/EV0CB9dTnh7VTtNFNvLA4KXw=; b=jisqcoaFQLRL/cAExXkNPf6QtATTtzTY+b683UivuX8BuUHbF0xVYROvyKGLodQc9eo/8s 2RrJ1NlJGnKx5dPjp4REzxeU2TpSOjE1p1VsOiHgjj6+r4flpcYM+mL5oImmDer1ThoZS6 QqdME3npxbhRmv8zfyRKezRg3IUeGMHkhj2mSPH7LJn9w9YuNyrE1AEpT7sRBXgO2D9BRa w2wJ8tF67AMBLo9iqqXJA5ArJp0eyZxIL/mxX259Ct/VuCWoR1yUfkym5xS+VU+lgZUTNG GdAR4Ib/lNsXq9SfN2TdsbIWTl+vUz976fLWpwLRo2HnhVyMi983Izxi4cKlvA== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1628102205; a=rsa-sha256; cv=none; b=ifo5lJD1MrQ+62PMfIoxsxdL2JzvExMWguYuiQ/aUdQrw0Gc7HsTdogIzRouku6/5YiWzt yTeCM9DCc4JAvuOPswFn8/xUJS4nBbEK/8+FA+eB0awF68WeOOGUi2y6AZZsWu2+oi5tch tMUgMx8HL46CE1Qtg/cPYXbgv+9+CBzVBSlaz0dZu+xmPTA001Jq4DhfiGTEppzJYpQ6UL W/lTDvliMkw/+skG75cha8ba3YEynK4pS+noN2m2bnqhF/Ylq3alxe6WolkMLUyVrBrmVV BDU9M0BwClfdpUkRIlzXL8EJp4ALfNjh7skHJSSRusSSQVbfctkpW0EuPnMVNw== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=mgsn.dev header.s=key1 header.b=S7e8bXsN; dmarc=fail reason="SPF not aligned (relaxed)" header.from=mgsn.dev (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.19 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=mgsn.dev header.s=key1 header.b=S7e8bXsN; dmarc=fail reason="SPF not aligned (relaxed)" header.from=mgsn.dev (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: 8F70C12EC7 X-Spam-Score: 0.19 X-Migadu-Scanner: scn0.migadu.com X-TUID: I3EC9Oa3ODuk Hi Maxim, Maxim Cournoyer writes: > 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. > [...] > > 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 => 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] Minimal version selection [2] go directive > > Thanks for the discussion! > >>>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"