From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id gBDYAFZHC2HvFQEAgWs5BA (envelope-from ) for ; Thu, 05 Aug 2021 04:05:10 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id 0L8yOFVHC2GtFQAA1q6Kng (envelope-from ) for ; Thu, 05 Aug 2021 02:05:09 +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 BE3F01863D for ; Thu, 5 Aug 2021 04:05:08 +0200 (CEST) Received: from localhost ([::1]:54466 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mBSkt-0007kp-SX for larch@yhetil.org; Wed, 04 Aug 2021 22:05:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41214) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBSko-0007kf-T6 for guix-patches@gnu.org; Wed, 04 Aug 2021 22:05:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:33944) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mBSko-0002O8-Mj for guix-patches@gnu.org; Wed, 04 Aug 2021 22:05:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mBSko-0000yQ-Fj for guix-patches@gnu.org; Wed, 04 Aug 2021 22:05: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: Thu, 05 Aug 2021 02:05: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.16281290833714 (code B ref 49602); Thu, 05 Aug 2021 02:05:02 +0000 Received: (at 49602) by debbugs.gnu.org; 5 Aug 2021 02:04:43 +0000 Received: from localhost ([127.0.0.1]:45490 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mBSkU-0000xp-H9 for submit@debbugs.gnu.org; Wed, 04 Aug 2021 22:04:42 -0400 Received: from mail-qv1-f43.google.com ([209.85.219.43]:43864) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mBSkS-0000xc-3U for 49602@debbugs.gnu.org; Wed, 04 Aug 2021 22:04:41 -0400 Received: by mail-qv1-f43.google.com with SMTP id db14so2154650qvb.10 for <49602@debbugs.gnu.org>; Wed, 04 Aug 2021 19:04:40 -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=0MdtZeLHVMrAd1ZkfOyy/nFJd9evgMqXCHGLF/ZwZNI=; b=QpHEZwPYHFN/3FJh0IlxAXhYRxeXdvnHejFwi+P7nzoEyaud+OF9FYCCDi+gzmMGZq UNoSbcDootVkR3hYtVgJuyeljPlUjmA6geAsR/2EcVA/+OpzTDauH7fS5g5iAACAIlGF 7RWS7Th0aVu/vATqLiwGiSZ1kibBYJ/rkKFCDpjg2V7TxhNwMX0BwollZtFJQzFqStYQ FmmZJ6XHHnwK4WOZGHr1SLq2WTzrhg/3qBRrkA+ms4gLYBYBOzAvAQJ9fKdxOEAFJ3oq iV3mO7+L3tiLmrqNl6J8CNvY6oL2Lc05EbMU+v3aCgqkvOzqjvHpiYn9noXrfeyNDOlC EMcQ== 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=0MdtZeLHVMrAd1ZkfOyy/nFJd9evgMqXCHGLF/ZwZNI=; b=dIuicP1AMRMu9XFx3u+DHaXox2LEEnZBd8mlfUrJj680DXVQbXF0ZEBM0M8ttXNfr0 OPOZmji8uv3cIp0sc6dyN9igpXVucFaOBMnZ5UyxO8tmn8dmNtLFEjTDspH/fazUg3Zx Ocbp7KomUSKS+F8LVfJUZYS1Qw8h/nh/zDvDxDiZc+1MrRVwFY5gy/aJojx12Q83J7Ic sdyQj+v4ZfKOpxcc/801q+dBkxsqAZW0AOcnnLQQFZxglpZ2XuVG3LrsmxN+fT7UmTiM ccIuVc/vPxh0uJn7Z88hgxns4YV8T2SgS/XizUdiujg/cD/9OvfB1OnmVUncLO8DPwYi 32hw== X-Gm-Message-State: AOAM5329vaOURhcqI8lP5gEDzgvXirQ7i+bY5pnZq8bJ36ENjYl1IQbY 0ly+Uy2tpBOH/I1zagDKlYGNkaW0PDmW5g== X-Google-Smtp-Source: ABdhPJxEJGSy5Su4uEqTTZAl3kRxw+SrIGodPgoFmAJg+c87M1dlvWqrE9q8p1EGSZsUUxECCnOoZw== X-Received: by 2002:ad4:4ba7:: with SMTP id i7mr2718299qvw.57.1628129074421; Wed, 04 Aug 2021 19:04:34 -0700 (PDT) Received: from hurd (dsl-10-129-132.b2b2c.ca. [72.10.129.132]) by smtp.gmail.com with ESMTPSA id r77sm2383696qke.15.2021.08.04.19.04.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Aug 2021 19:04:34 -0700 (PDT) From: Maxim Cournoyer References: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> <87tuksjh68.fsf@gmail.com> <86eebvt2o8.fsf@mgsn.dev> <87mtpy7gn2.fsf@gmail.com> <86v94l2hxm.fsf_-_@mgsn.dev> Date: Wed, 04 Aug 2021 22:04:33 -0400 In-Reply-To: <86v94l2hxm.fsf_-_@mgsn.dev> (Sarah Morgensen's message of "Wed, 04 Aug 2021 11:17:25 -0700") Message-ID: <87v94kmytq.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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=1628129109; 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=0MdtZeLHVMrAd1ZkfOyy/nFJd9evgMqXCHGLF/ZwZNI=; b=cABLcov5fGmF9lhEAEWi/lzmexOg+r4Lj9BmJLTlWDEAQk5PF9DL4XtqqMD9+yaT8xQtjH LomgyCN0+xP7b1NtBgvp5QNCuYLNXVZDV3FflLzaKIiR8fYJfLUCfYpE3cpYEQWk4Lk2uw i/AX/Vy6jBOp8+PUgFVMU0H+q6wOm05nEt4aYN4g+Ec5QjRC09CZH3jjSvluxUubEnwQ2Y KCXksQTQV9jiI5lN83nEhIcJ/i9AsgPnwXSsg6uhXVwbiAgAKGfXsygFEBn3YFyu1ZNjcC 4PJbzDzMtViBZagQKraTdsSeAJSrEmoi84IbvrdXIeswDG2cZ79RB5GJ0HQtQQ== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1628129109; a=rsa-sha256; cv=none; b=KW9VeOAYpPkERbQHF7dPOM6oRdg6OIU/jeiMDqHhvMHmYELQ1p0/tfFEp1BcFL8J9vBJLv hHzDzVjmeGzUdd7S3lFTCDYlM5ZPdCvMU8nZbgxuAdlB5XIE3bb/JEEHLn/93qfLN5SnNx VpqAXlFdRGW2xla8XYR14gFlTp7j5OHJ2kw3Jsbf4wKa/mBUbULDxRQpuvKjI+FkgI8yMk mFH+zmq8xMGgrzw9RYVt9cTanziIajrpGhuSs8TMwehYefpfOLM4oSrHlJW7XmOx3RgR5A GYd3BvUNbHizvnabEJ0Ty661XTPl1C9xHyl9NhmaSyKoUjV1m83LWrYNmXdyUQ== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20161025 header.b=QpHEZwPY; 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: -1.31 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20161025 header.b=QpHEZwPY; 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: BE3F01863D X-Spam-Score: -1.31 X-Migadu-Scanner: scn1.migadu.com X-TUID: dhvCwfkLMsTb Hello Sarah, Sarah Morgensen 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 => 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