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 VY5WHeS5DGGXyAAAgWs5BA (envelope-from ) for ; Fri, 06 Aug 2021 06:26:12 +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 KHFdGOS5DGE/EQAAB5/wlQ (envelope-from ) for ; Fri, 06 Aug 2021 04:26:12 +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 1A579115C9 for ; Fri, 6 Aug 2021 06:26:08 +0200 (CEST) Received: from localhost ([::1]:50626 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mBrQs-0008Ed-TI for larch@yhetil.org; Fri, 06 Aug 2021 00:26:06 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:51260) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBrQo-0008EV-VR for guix-patches@gnu.org; Fri, 06 Aug 2021 00:26:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:37300) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mBrQo-0006bx-Kf for guix-patches@gnu.org; Fri, 06 Aug 2021 00:26:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mBrQo-0001Un-GZ for guix-patches@gnu.org; Fri, 06 Aug 2021 00:26: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: Fri, 06 Aug 2021 04:26: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.16282239225693 (code B ref 49602); Fri, 06 Aug 2021 04:26:02 +0000 Received: (at 49602) by debbugs.gnu.org; 6 Aug 2021 04:25:22 +0000 Received: from localhost ([127.0.0.1]:48846 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mBrQ9-0001Tk-Ou for submit@debbugs.gnu.org; Fri, 06 Aug 2021 00:25:22 -0400 Received: from out2.migadu.com ([188.165.223.204]:58744) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mBrQ7-0001TU-7w for 49602@debbugs.gnu.org; Fri, 06 Aug 2021 00:25:21 -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=1628223917; 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=cEs2n83tBEtELptitXiYiFY2fqqg26Jstzm7XDkVinA=; b=RzS8akZ8Ma0CArxJG4VhdTS4/e9s8bWWeGUznuLEkxKxXwOXbm92gLmwPn8Ke1XuTn1rXK t4511FlhOLYcXCPXet796a67IOwKM2pMZ8NVxjhu54n6JaHPHHw0CeEaWo2Elt/OpwGZKo n1kcjhjo+yH+kq+JTmTSc7BGUYfeQzc= From: Sarah Morgensen References: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> <87tuksjh68.fsf@gmail.com> <86eebvt2o8.fsf@mgsn.dev> <87mtpy7gn2.fsf@gmail.com> <86v94l2hxm.fsf_-_@mgsn.dev> <87v94kmytq.fsf@gmail.com> Date: Thu, 05 Aug 2021 21:25:14 -0700 In-Reply-To: <87v94kmytq.fsf@gmail.com> (Maxim Cournoyer's message of "Wed, 04 Aug 2021 22:04:33 -0400") Message-ID: <86sfzn19p1.fsf_-_@mgsn.dev> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" 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=1628223968; 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=cEs2n83tBEtELptitXiYiFY2fqqg26Jstzm7XDkVinA=; b=J6ZNJZVPgrANIqRsUDunEVdVyypXLhujdcR8i0G0e5PLrgc3bO+hDdTDMJJ25Z8FhkERxc tM0/73jlH+S9Yc0tADJGXT8W1N2WQ/f2uBuLokpHzcz7/5Q4vgEgl9gz1qGB6Ac5P0KSjb cYEW6atFJ7l1TpC2ZO0OJaxzOfZfEU/gNEVNSwDCVqsSKAMfsGwmCAJt9hTc3s8uq19syw 9K8+N0EPhHGvrwmLGIrq+hFq8BxsVD2YDd571mwS9I9zmARMO5sd0SR2ev+E4K4lodJx21 pB69R2CKvALHwn/zaA0hGo/7+sW+0jbRpCRqRrMeYDIEszJ3SR2mB1AHrVRTlA== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1628223968; a=rsa-sha256; cv=none; b=LnNCqeY2Of1ZmWmFExNPs4KbZ59fLxfe9bHDAHnu0rpEEBmiz/Lo4ozuPaWSq02ygrZCeh CEPaJzm2n471scAnrC7Qyu6E6uw3HtUXSYb7VHbEPQfCS6Geofn7knBezPG2rpjwapojOh /DUMRqns6dffIUuJMil/Mqjo+qXzEiQqevbIN6mWKxN2iyenpSxSYyBipdeFacc8F4p6dr kWjKTXIWUm+xJ66aiIH8d/pOwhrYQzMADkKQ4yq3iuZNb9KbhiP+ZDGYFzhJHGETwZyUBG J5RvPBO8rfaWsdH1G1/qZDnTyOj/30nRWZC0JjY6YS3dVbDSs/N7CD2Mk21AEQ== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=mgsn.dev header.s=key1 header.b=RzS8akZ8; 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: -1.32 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=mgsn.dev header.s=key1 header.b=RzS8akZ8; 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: 1A579115C9 X-Spam-Score: -1.32 X-Migadu-Scanner: scn1.migadu.com X-TUID: HqrWMdHbhCL6 --=-=-= Content-Type: text/plain Hi Maxim, Maxim Cournoyer writes: [...] > >> 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? Not quite, as you explain below (same-named modules are currently not being replaced). > > So, I was going to propose to just add a comment, like so: > > 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)))) > > 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? As I did not write the original Go importer, I can only guess, as I did earlier: > 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. 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. As I suggested there, I would honor the same-named replacement. I've attached an updated draft comment and code to match. I got carried away, so there's a second one which riffs off of your previous version check patch. (One solution, I think, would be to discriminate between direct and indirect dependencies, and only include an indirect dependency in inputs if its version is different than the one already used. But this would require restructuring the importer for arguably little benefit.) [...] >> 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 ? :-)). *sigh* Yes, this is frustrating. IIUC the Guix way is to have two separate packages/variables if there is a version incompatibility. But like you say, because of the way the Go build system uses GOPATH, only one version of any module is used at a time. We may indeed want to go down the path of recreating $GOPATH/pkg/mod (or even a GOPROXY mock?), but I don't think it will be easy, and I think there will be issues with Guix not having whatever exact version some module wants. > >> 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 -- Sarah --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-import-go-Allow-version-pinning-for-indirect-depende.patch Content-Description: 0001-import-go-Allow-version-pinning-for-indirect-depende.patch >From e32521de5b0badf8172058364611db147d562522 Mon Sep 17 00:00:00 2001 Message-Id: From: Sarah Morgensen Date: Thu, 5 Aug 2021 21:15:26 -0700 Subject: [PATCH 1/2] import: go: Allow version pinning for indirect dependencies. --- guix/import/go.scm | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/guix/import/go.scm b/guix/import/go.scm index 617a0d0e23..5c1a251677 100644 --- a/guix/import/go.scm +++ b/guix/import/go.scm @@ -349,12 +349,15 @@ DIRECTIVE." "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)))) + ;; 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. + ;; + ;; Notably, allow version pinning/updating for indirect dependencies. It + ;; is rare in practice, may be useful with --pin-versions, and at worst + ;; adds an extra direct input (which would be transitively included anyway). + (cons new-requirement (alist-delete module-path requirements))) (match directive ((('original ('module-path module-path) . _) with . _) base-commit: d0d3bcc615f1d521ea60a8b2e085767e0adb05b6 -- 2.31.1 --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0002-import-go-Check-version-for-replacements.patch Content-Description: 0002-import-go-Check-version-for-replacements.patch >From c1c898c1df14f931be31151713ec4204adee04eb Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Sarah Morgensen Date: Thu, 5 Aug 2021 21:19:58 -0700 Subject: [PATCH 2/2] import: go: Check version for replacements. --- guix/import/go.scm | 48 +++++++++++++++++++++++++++++----------------- tests/go.scm | 2 ++ 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/guix/import/go.scm b/guix/import/go.scm index 5c1a251677..04546cb9e9 100644 --- a/guix/import/go.scm +++ b/guix/import/go.scm @@ -348,25 +348,37 @@ 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) - ;; 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. - ;; - ;; Notably, allow version pinning/updating for indirect dependencies. It - ;; is rare in practice, may be useful with --pin-versions, and at worst - ;; adds an extra direct input (which would be transitively included anyway). - (cons new-requirement (alist-delete module-path requirements))) - + ;; 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. However, if a version is specified and the + ;; module is required in this go.mod, the version must match in order to + ;; replace. + ;; + ;; Notably, allow version pinning/updating for indirect dependencies. It + ;; is rare in practice, may be useful with --pin-versions, and at worst + ;; adds an extra direct input (which would be transitively included anyway). + (define (replace* module-path version + to-module-path to-version + requirements) + "Perform the replacement unless VERSION is non-false, MODULE-PATH is found +in REQUIREMENTS, and its version does not match VERSION. Return the updated +REQUIREMENTS." + (let* ((current-version (and=> (assoc-ref requirements module-path) first)) + (version-matches? (equal? version current-version))) + (if (and version current-version (not version-matches?)) + requirements + (cons (list to-module-path to-version) + (alist-delete module-path 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))))))) + ((('original ('module-path module-path) . _) ('with ('file-path _)) . _) + (alist-delete module-path 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)) + ((('original ('module-path module-path) . _) + ('with ('module-path new-module-path) ('version new-version) . _) . _) + (replace* module-path #f new-module-path new-version requirements)))) (define (require directive requirements) (match directive diff --git a/tests/go.scm b/tests/go.scm index 6749f4585f..ec92e3fce4 100644 --- a/tests/go.scm +++ b/tests/go.scm @@ -238,6 +238,8 @@ require github.com/kr/pretty v0.2.1 '(("github.com/corp/arbitrary-repo" "v0.0.2") ("quoted.example.com/abitrary/repo" "v0.0.2") ("one.example.com/abitrary/repo" "v1.1.111") + ("golang.org/x/sys" "v0.0.0-20190813064441-fde4db37ae7a") + ("golang.org/x/tools" "v0.0.0-20190821162956-65e3620a7ae7") ("hub.jazz.net/git/user/project/sub/directory" "v1.1.19") ("hub.jazz.net/git/user/project" "v1.1.18") ("launchpad.net/~user/project/branch/sub/directory" "v1.1.17") -- 2.31.1 --=-=-=--