From unknown Sat Jun 14 03:52:58 2025 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Mailer: MIME-tools 5.509 (Entity 5.509) Content-Type: text/plain; charset=utf-8 From: bug#49602 <49602@debbugs.gnu.org> To: bug#49602 <49602@debbugs.gnu.org> Subject: Status: [PATCH] import: go: Upgrade go.mod parser. Reply-To: bug#49602 <49602@debbugs.gnu.org> Date: Sat, 14 Jun 2025 10:52:58 +0000 retitle 49602 [PATCH] import: go: Upgrade go.mod parser. reassign 49602 guix-patches submitter 49602 Sarah Morgensen severity 49602 normal tag 49602 patch thanks From debbugs-submit-bounces@debbugs.gnu.org Sat Jul 17 00:01:57 2021 Received: (at submit) by debbugs.gnu.org; 17 Jul 2021 04:01:58 +0000 Received: from localhost ([127.0.0.1]:52648 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m4bWM-0007xh-ME for submit@debbugs.gnu.org; Sat, 17 Jul 2021 00:01:57 -0400 Received: from lists.gnu.org ([209.51.188.17]:51908) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m4bWH-0007xV-W0 for submit@debbugs.gnu.org; Sat, 17 Jul 2021 00:01:45 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:56316) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m4bWH-0004hB-Ma for guix-patches@gnu.org; Sat, 17 Jul 2021 00:01:41 -0400 Received: from out0.migadu.com ([94.23.1.103]:25007) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m4bWC-0006aG-OZ for guix-patches@gnu.org; Sat, 17 Jul 2021 00:01:41 -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=1626494491; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Bt7N1jKJ1aX5dak10w/+SP5R7jmyo4Kk45xxNajvPUQ=; b=Zql2M7EXZJsYY6dZnXIu1NSr7hW6VebfW9lMAXfx8qzGlK8m7naTP94jjP5w+erAmiZPNT I82E5IBojSzck6kWudVc4rfECSjMbX3bgeuciBUdez93Hmc43US02FPch0b9j4Na6Xmcwx +YoL6ycV+j2WBnZf8HU71OzavyxQZCk= From: Sarah Morgensen To: guix-patches@gnu.org Subject: [PATCH] import: go: Upgrade go.mod parser. Date: Fri, 16 Jul 2021 21:01:28 -0700 Message-Id: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: iskarian@mgsn.dev Received-SPF: pass client-ip=94.23.1.103; envelope-from=iskarian@mgsn.dev; helo=out0.migadu.com X-Spam_score_int: -26 X-Spam_score: -2.7 X-Spam_bar: -- X-Spam_report: (-2.7 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_SBL_A=0.1 autolearn=ham autolearn_force=no X-Spam_action: no action X-Spam-Score: -0.6 (/) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.1 (/) Upgrade the go.mod parser to handle the full go.mod spec, and to gracefully handle unexpected/malformed syntax. Restructure parser usage, making the parse tree available for other uses. guix/import/go.scm (parse-go.mod): Parse using (ice-9 peg) instead of regex matching for more robustness. Return a list of directives. (go.mod-directives): New procedure. (go.mod-requirements): New procedure. (go-module->guix-package): Use it. (%go.mod-require-directive-rx) (%go.mod-replace-directive-rx): Remove unused variable. tests/go.scm (testing-parse-mod): Adjust accordingly. (go.mod-requirements) (fixture-go-mod-unparseable) (fixture-go-mod-retract) (fixture-go-mod-strings): New variable. ("parse-go.mod: simple") ("parse-go.mod: comments and unparseable lines") ("parse-go.mod: retract") ("parse-go.mod: raw strings and quoted strings") ("parse-go.mod: complete"): New tests. --- Hello Guix, This one is pretty self-explanatory. This parser handles the full go.mod spec (notably: comments, extra whitespace, retract/exclude) and gracefully handles things it doesn't understand. It is a bit more complex, I suppose. WDYT? -- Sarah guix/import/go.scm | 247 +++++++++++++++++++++++---------------------- tests/go.scm | 132 +++++++++++++++++++++++- 2 files changed, 260 insertions(+), 119 deletions(-) diff --git a/guix/import/go.scm b/guix/import/go.scm index d8f838f635..69e71d01e5 100644 --- a/guix/import/go.scm +++ b/guix/import/go.scm @@ -41,6 +41,7 @@ #:autoload (guix base32) (bytevector->nix-base32-string) #:autoload (guix build utils) (mkdir-p) #:use-module (ice-9 match) + #:use-module (ice-9 peg) #:use-module (ice-9 rdelim) #:use-module (ice-9 receive) #:use-module (ice-9 regex) @@ -244,129 +245,139 @@ and VERSION and return an input port." (go-path-escape version)))) (http-fetch* url))) -(define %go.mod-require-directive-rx - ;; A line in a require directive is composed of a module path and - ;; a version separated by whitespace and an optionnal '//' comment at - ;; the end. - (make-regexp - (string-append - "^[[:blank:]]*([^[:blank:]]+)[[:blank:]]+" ;the module path - "([^[:blank:]]+)" ;the version - "([[:blank:]]+//.*)?"))) ;an optional comment - -(define %go.mod-replace-directive-rx - ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline - ;; | ModulePath [ Version ] "=>" ModulePath Version newline . - (make-regexp - (string-append - "([^[:blank:]]+)" ;the module path - "([[:blank:]]+([^[:blank:]]+))?" ;optional version - "[[:blank:]]+=>[[:blank:]]+" - "([^[:blank:]]+)" ;the file or module path - "([[:blank:]]+([^[:blank:]]+))?"))) ;the version (if a module path) (define (parse-go.mod content) - "Parse the go.mod file CONTENT, returning a list of requirements." - ;; We parse only a subset of https://golang.org/ref/mod#go-mod-file-grammar - ;; which we think necessary for our use case. - (define (toplevel requirements replaced) - "This is the main parser. The results are accumulated in THE REQUIREMENTS -and REPLACED lists." - (let ((line (read-line))) - (cond - ((eof-object? line) - ;; parsing ended, give back the result - (values requirements replaced)) - ((string=? line "require (") - ;; a require block begins, delegate parsing to IN-REQUIRE - (in-require requirements replaced)) - ((string=? line "replace (") - ;; a replace block begins, delegate parsing to IN-REPLACE - (in-replace requirements replaced)) - ((string-prefix? "require " line) - ;; a require directive by itself - (let* ((stripped-line (string-drop line 8))) - (call-with-values - (lambda () - (require-directive requirements replaced stripped-line)) - toplevel))) - ((string-prefix? "replace " line) - ;; a replace directive by itself - (let* ((stripped-line (string-drop line 8))) - (call-with-values - (lambda () - (replace-directive requirements replaced stripped-line)) - toplevel))) - (#t - ;; unrecognised line, ignore silently - (toplevel requirements replaced))))) - - (define (in-require requirements replaced) - (let ((line (read-line))) - (cond - ((eof-object? line) - ;; this should never happen here but we ignore silently - (values requirements replaced)) - ((string=? line ")") - ;; end of block, coming back to toplevel - (toplevel requirements replaced)) - (#t - (call-with-values (lambda () - (require-directive requirements replaced line)) - in-require))))) - - (define (in-replace requirements replaced) - (let ((line (read-line))) - (cond - ((eof-object? line) - ;; this should never happen here but we ignore silently - (values requirements replaced)) - ((string=? line ")") - ;; end of block, coming back to toplevel - (toplevel requirements replaced)) - (#t - (call-with-values (lambda () - (replace-directive requirements replaced line)) - in-replace))))) - - (define (replace-directive requirements replaced line) - "Extract replaced modules and new requirements from the replace directive -in LINE and add them to the REQUIREMENTS and REPLACED lists." - (let* ((rx-match (regexp-exec %go.mod-replace-directive-rx line)) - (module-path (match:substring rx-match 1)) - (version (match:substring rx-match 3)) - (new-module-path (match:substring rx-match 4)) - (new-version (match:substring rx-match 6)) - (new-replaced (cons (list module-path version) replaced)) - (new-requirements - (if (string-match "^\\.?\\./" new-module-path) - requirements - (cons (list new-module-path new-version) requirements)))) - (values new-requirements new-replaced))) - - (define (require-directive requirements replaced line) - "Extract requirement from LINE and augment the REQUIREMENTS and REPLACED -lists." - (let* ((rx-match (regexp-exec %go.mod-require-directive-rx line)) - (module-path (match:substring rx-match 1)) - ;; Double-quoted strings were seen in the wild without escape - ;; sequences; trim the quotes to be on the safe side. - (module-path (string-trim-both module-path #\")) - (version (match:substring rx-match 2))) - (values (cons (list module-path version) requirements) replaced))) - - (with-input-from-string content - (lambda () - (receive (requirements replaced) - (toplevel '() '()) - ;; At last remove the replaced modules from the requirements list. - (remove (lambda (r) - (assoc (car r) replaced)) - requirements))))) + "Parse the go.mod file CONTENT, returning a list of directives, comments, +and unknown lines. Each sublist begins with a symbol (go, module, require, +replace, exclude, retract, comment, or unknown) and is followed by one or more +sublists. Each sublist begins with a symbol (module-path, version, file-path, +comment, or unknown) and is followed by the indicated data." + ;; https://golang.org/ref/mod#go-mod-file-grammar + (define-peg-pattern NL none "\n") + (define-peg-pattern WS none (or " " "\t" "\r")) + (define-peg-pattern => none (and (* WS) "=>")) + (define-peg-pattern punctuation none (or "," "=>" "[" "]" "(" ")")) + (define-peg-pattern comment all + (and (ignore "//") (* WS) (* (and (not-followed-by NL) peg-any)))) + (define-peg-pattern EOL body (and (* WS) (? comment) NL)) + (define-peg-pattern block-start none (and (* WS) "(" EOL)) + (define-peg-pattern block-end none (and (* WS) ")" EOL)) + (define-peg-pattern any-line body + (and (* WS) (* (and (not-followed-by NL) peg-any)) EOL)) + + ;; Strings and identifiers + (define-peg-pattern identifier body + (+ (and (not-followed-by (or NL WS punctuation)) peg-any))) + (define-peg-pattern string-raw body + (and (ignore "`") (+ (and (not-followed-by "`") peg-any)) (ignore "`"))) + (define-peg-pattern string-quoted body + (and (ignore "\"") + (+ (or (and (ignore "\\") peg-any) + (and (not-followed-by "\"") peg-any))) + (ignore "\""))) + (define-peg-pattern string-or-ident body + (and (* WS) (or string-raw string-quoted identifier))) + + (define-peg-pattern version all string-or-ident) + (define-peg-pattern module-path all string-or-ident) + (define-peg-pattern file-path all string-or-ident) + + ;; Non-directive lines + (define-peg-pattern unknown all any-line) + (define-peg-pattern block-line body + (or EOL (and (not-followed-by block-end) unknown))) + + ;; GoDirective = "go" GoVersion newline . + (define-peg-pattern go all (and (ignore "go") version EOL)) + + ;; ModuleDirective = "module" ( ModulePath | "(" newline ModulePath newline ")" ) newline . + (define-peg-pattern module all + (and (ignore "module") (or (and block-start module-path EOL block-end) + (and module-path EOL)))) + + ;; The following directives may all be used solo or in a block + ;; RequireSpec = ModulePath Version newline . + (define-peg-pattern require all (and module-path version EOL)) + (define-peg-pattern require-top body + (and (ignore "require") + (or (and block-start (* (or require block-line)) block-end) require))) + + ;; ExcludeSpec = ModulePath Version newline . + (define-peg-pattern exclude all (and module-path version EOL)) + (define-peg-pattern exclude-top body + (and (ignore "exclude") + (or (and block-start (* (or exclude block-line)) block-end) exclude))) + + ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline + ;; | ModulePath [ Version ] "=>" ModulePath Version newline . + (define-peg-pattern original all (or (and module-path version) module-path)) + (define-peg-pattern with all (or (and module-path version) file-path)) + (define-peg-pattern replace all (and original => with EOL)) + (define-peg-pattern replace-top body + (and (ignore "replace") + (or (and block-start (* (or replace block-line)) block-end) replace))) + + ;; RetractSpec = ( Version | "[" Version "," Version "]" ) newline . + (define-peg-pattern range all + (and (* WS) (ignore "[") version + (* WS) (ignore ",") version (* WS) (ignore "]"))) + (define-peg-pattern retract all (and (or range version) EOL)) + (define-peg-pattern retract-top body + (and (ignore "retract") + (or (and block-start (* (or retract block-line)) block-end) retract))) + + (define-peg-pattern go-mod body + (* (and (* WS) (or go module require-top exclude-top replace-top + retract-top EOL unknown)))) + + (let ((tree (peg:tree (match-pattern go-mod content))) + (keywords '(go module require replace exclude retract comment unknown))) + (keyword-flatten keywords tree))) ;; Prevent inlining of this procedure, which is accessed by unit tests. (set! parse-go.mod parse-go.mod) +(define (go.mod-directives go.mod directive) + "Return the list of top-level directive bodies in GO.MOD matching the symbol +DIRECTIVE." + (filter-map (match-lambda + (((? (cut eq? <> directive) head) . rest) rest) + (_ #f)) + go.mod)) + +(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... + (if (and (equal? module-path (first new-requirement)) + (not (assoc-ref requirements module-path))) + requirements + (cons new-requirement (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))))))) + + (define (require directive requirements) + (match directive + ((('module-path module-path) ('version version) . _) + (cons (list module-path version) requirements)))) + + (let* ((requires (go.mod-directives go.mod 'require)) + (replaces (go.mod-directives go.mod 'replace)) + (requirements (fold require '() requires))) + (fold replace requirements replaces))) + +;; Prevent inlining of this procedure, which is accessed by unit tests. +(set! go.mod-requirements go.mod-requirements) + (define-record-type (%make-vcs url-prefix root-regex type) vcs? @@ -588,7 +599,7 @@ When VERSION is unspecified, the latest version available is used." hint: use one of the following available versions ~a\n" version* available-versions)))) (content (fetch-go.mod goproxy module-path version*)) - (dependencies+versions (parse-go.mod content)) + (dependencies+versions (go.mod-requirements (parse-go.mod content))) (dependencies (if pin-versions? dependencies+versions (map car dependencies+versions))) diff --git a/tests/go.scm b/tests/go.scm index b088ab50d2..b16965d941 100644 --- a/tests/go.scm +++ b/tests/go.scm @@ -1,5 +1,6 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2021 François Joulaud +;;; Copyright © 2021 Sarah Morgensen ;;; ;;; This file is part of GNU Guix. ;;; @@ -31,6 +32,9 @@ #:use-module (srfi srfi-64) #:use-module (web response)) +(define go.mod-requirements + (@@ (guix import go) go.mod-requirements)) + (define parse-go.mod (@@ (guix import go) parse-go.mod)) @@ -96,6 +100,40 @@ replace ( ") +(define fixture-go-mod-unparseable + "module my/thing +go 1.12 // avoid feature X +require other/thing v1.0.2 +// Security issue: CVE-XXXXX +exclude old/thing v1.2.3 +new-directive another/thing yet-another/thing +replace ( + bad/thing v1.4.5 => good/thing v1.4.5 + // Unparseable + bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0 +) +") + +(define fixture-go-mod-retract + "retract v0.9.1 + +retract ( + v1.9.2 + [v1.0.0, v1.7.9] +) +") + +(define fixture-go-mod-strings + "require `example.com/\"some-repo\"` v1.9.3 +require ( + `example.com/\"another.repo\"` v1.0.0 + \"example.com/special!repo\" v9.3.1 +) +replace \"example.com/\\\"some-repo\\\"\" => `launchpad.net/some-repo` v1.9.3 +replace ( + \"example.com/\\\"another.repo\\\"\" => launchpad.net/another-repo v1.0.0 +) +") (define fixture-latest-for-go-check @@ -185,7 +223,7 @@ require github.com/kr/pretty v0.2.1 (string good/thing v2.0.0")) + (parse-go.mod fixture-go-mod-unparseable)) + +(test-equal "parse-go.mod: retract" + `((retract (version "v0.9.1")) + (retract (version "v1.9.2")) + (retract (range (version "v1.0.0") (version "v1.7.9")))) + (parse-go.mod fixture-go-mod-retract)) + +(test-equal "parse-go.mod: raw strings and quoted strings" + `((require (module-path "example.com/\"some-repo\"") (version "v1.9.3")) + (require (module-path "example.com/\"another.repo\"") (version "v1.0.0")) + (require (module-path "example.com/special!repo") (version "v9.3.1")) + (replace (original (module-path "example.com/\"some-repo\"")) + (with (module-path "launchpad.net/some-repo") (version "v1.9.3"))) + (replace (original (module-path "example.com/\"another.repo\"")) + (with (module-path "launchpad.net/another-repo") (version "v1.0.0")))) + (parse-go.mod fixture-go-mod-strings)) + +(test-equal "parse-go.mod: complete" + `((module (module-path "M")) + (go (version "1.13")) + (replace (original (module-path "github.com/myname/myproject/myapi")) + (with (file-path "./api"))) + (replace (original (module-path "github.com/mymname/myproject/thissdk")) + (with (file-path "../sdk"))) + (replace (original (module-path "launchpad.net/gocheck")) + (with (module-path "github.com/go-check/check") + (version "v0.0.0-20140225173054-eb6ee6f84d0a"))) + (require (module-path "github.com/user/project") + (version "v1.1.11")) + (require (module-path "github.com/user/project/sub/directory") + (version "v1.1.12")) + (require (module-path "bitbucket.org/user/project") + (version "v1.11.20")) + (require (module-path "bitbucket.org/user/project/sub/directory") + (version "v1.11.21")) + (require (module-path "launchpad.net/project") + (version "v1.1.13")) + (require (module-path "launchpad.net/project/series") + (version "v1.1.14")) + (require (module-path "launchpad.net/project/series/sub/directory") + (version "v1.1.15")) + (require (module-path "launchpad.net/~user/project/branch") + (version "v1.1.16")) + (require (module-path "launchpad.net/~user/project/branch/sub/directory") + (version "v1.1.17")) + (require (module-path "hub.jazz.net/git/user/project") + (version "v1.1.18")) + (require (module-path "hub.jazz.net/git/user/project/sub/directory") + (version "v1.1.19")) + (require (module-path "k8s.io/kubernetes/subproject") + (version "v1.1.101")) + (require (module-path "one.example.com/abitrary/repo") + (version "v1.1.111")) + (require (module-path "two.example.com/abitrary/repo") + (version "v0.0.2")) + (require (module-path "quoted.example.com/abitrary/repo") + (version "v0.0.2")) + (replace (original (module-path "two.example.com/abitrary/repo")) + (with (module-path "github.com/corp/arbitrary-repo") + (version "v0.0.2"))) + (replace (original (module-path "golang.org/x/sys")) + (with (module-path "golang.org/x/sys") + (version "v0.0.0-20190813064441-fde4db37ae7a")) + (comment "pinned to release-branch.go1.13")) + (replace (original (module-path "golang.org/x/tools")) + (with (module-path "golang.org/x/tools") + (version "v0.0.0-20190821162956-65e3620a7ae7")) + (comment "pinned to release-branch.go1.13"))) + (parse-go.mod fixture-go-mod-complete)) + ;;; End-to-end tests for (guix import go) (define (mock-http-fetch testcase) (lambda (url . rest) base-commit: 3ee0f170c8bd883728d8abb2c2e00f445c13f17d -- 2.31.1 From debbugs-submit-bounces@debbugs.gnu.org Sun Jul 18 01:59:58 2021 Received: (at 49602-done) by debbugs.gnu.org; 18 Jul 2021 05:59:58 +0000 Received: from localhost ([127.0.0.1]:55053 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m4zqC-00068K-Kh for submit@debbugs.gnu.org; Sun, 18 Jul 2021 01:59:58 -0400 Received: from mail-qk1-f181.google.com ([209.85.222.181]:44771) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m4zqA-000687-PG for 49602-done@debbugs.gnu.org; Sun, 18 Jul 2021 01:59:52 -0400 Received: by mail-qk1-f181.google.com with SMTP id a80so1990751qkg.11 for <49602-done@debbugs.gnu.org>; Sat, 17 Jul 2021 22:59:50 -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=EZuvC/6atMEYFimCXia/D65CMz0lLb/+Ul2WqyYI8+o=; b=HK4FWe8+zOdiqWq0WYHSLhEWceb81WSLiDmZ121VfU+KdB0l67f650BY3Nx/nmae3k +06lu6VuNt6SQ7g/8ApSrO5OpXxKi1fFwB2ePxo8UPazoJLfP0KX54EA/HYsqlf4EznW RiqeLskFNcJAonXK23fqCfDWQmJFauAdW048fiDZjkXycLt3iCXoxlXXiNvBUXiK1+Ju CWYfqzHYBd26gbjZ81jo5OhhZ7zSK5uE8itqc4o9IhjusuRG8a0zrR8PJO60LT5GoE8o 0/YJgLSM2OQB4m7cqy8TE+lSNlQj9v3trkw5dcK0H1eemcqMuvYgGXl2/iPDUsBX7Xau i6fw== 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=EZuvC/6atMEYFimCXia/D65CMz0lLb/+Ul2WqyYI8+o=; b=egIgB2kkHcBjnGDH9P7uMjWNqeWLI7JQYFE7OlOtF3DIp8w/ch71dJwL7x6dT4Fcca SJS+4G/zIgkyTYq5vhR7cGdr3ywYKfhG2r1zw6XB3XBXJALsfuCmWnx64+rAq7eiHiFG 2E3kiK7gssyvg18CfIJJeOdoNbepQZpmLq77J78tzmNoqwk5O3SA/mcImcG4ATO38y4m mDr9uuer8QwXNhV7cWo8G/VXjo2U+sU3rlVqdoD1i35k38KdgTHd6rf8AEyNlLwhYEat FGmleFGnmZITXLB3aqtsY+uYcio7BZ+br2zOZheDt5UZ/Y1N2cNpCw8JTFu22UNak2uw bE5A== X-Gm-Message-State: AOAM5306gf3G+26jZVnm2WcmhTOeT2Xmh13MF1mYwGN/PtUhBcsZk8g+ R5gYGXV7XVqmS7U6OsPxyVHW4yHbh5rjhw== X-Google-Smtp-Source: ABdhPJxwi77ZOOhtJ+3v51697gD0YAdA3MuP0EjH79hp8pVriFW1w1Ork08r/qpnEs16MBLKv7aYWw== X-Received: by 2002:a37:b586:: with SMTP id e128mr18046827qkf.43.1626587984986; Sat, 17 Jul 2021 22:59:44 -0700 (PDT) Received: from hurd (dsl-148-66.b2b2c.ca. [66.158.148.66]) by smtp.gmail.com with ESMTPSA id m80sm3424473qke.98.2021.07.17.22.59.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 17 Jul 2021 22:59:44 -0700 (PDT) From: Maxim Cournoyer To: Sarah Morgensen Subject: Re: bug#49602: [PATCH] import: go: Upgrade go.mod parser. References: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> Date: Sun, 18 Jul 2021 01:59:43 -0400 In-Reply-To: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> (Sarah Morgensen's message of "Fri, 16 Jul 2021 21:01:28 -0700") Message-ID: <87tuksjh68.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-Spam-Score: 0.7 (/) X-Debbugs-Envelope-To: 49602-done Cc: 49602-done@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.3 (/) Hi! Sarah Morgensen writes: > Upgrade the go.mod parser to handle the full go.mod spec, and to > gracefully handle unexpected/malformed syntax. Restructure parser ^ There is a convention in Guix to add two spaces to separate the the ending (period) of a sentence from the beginning of the next one (which was inherited from GNU Standards, AFAICT: "Please put two spaces after the end of a sentence in your comments, so that the Emacs sentence commands will work.", c.f. info "(standards) Comments"). > usage, making the parse tree available for other uses. > > guix/import/go.scm (parse-go.mod): Parse using (ice-9 peg) instead of > regex matching for more robustness. Return a list of directives. > (go.mod-directives): New procedure. > (go.mod-requirements): New procedure. ^Likewise. > (go-module->guix-package): Use it. > (%go.mod-require-directive-rx) The above line can be removed, as it is duplicated below. > (%go.mod-replace-directive-rx): Remove unused variable. > tests/go.scm (testing-parse-mod): Adjust accordingly. > (go.mod-requirements) > (fixture-go-mod-unparseable) > (fixture-go-mod-retract) > (fixture-go-mod-strings): New variable. ^s. > ("parse-go.mod: simple") > ("parse-go.mod: comments and unparseable lines") > ("parse-go.mod: retract") > ("parse-go.mod: raw strings and quoted strings") > ("parse-go.mod: complete"): New tests. > --- > Hello Guix, > > This one is pretty self-explanatory. This parser handles the full go.mod spec > (notably: comments, extra whitespace, retract/exclude) and gracefully handles > things it doesn't understand. It is a bit more complex, I suppose. WDYT? This looks really neat! I'm happy to see more use of (ice-9 peg). I think the added complexity is worth it, given the benefits it provides. > -- Sarah > > guix/import/go.scm | 247 +++++++++++++++++++++++---------------------- > tests/go.scm | 132 +++++++++++++++++++++++- > 2 files changed, 260 insertions(+), 119 deletions(-) > > diff --git a/guix/import/go.scm b/guix/import/go.scm > index d8f838f635..69e71d01e5 100644 > --- a/guix/import/go.scm > +++ b/guix/import/go.scm > @@ -41,6 +41,7 @@ > #:autoload (guix base32) (bytevector->nix-base32-string) > #:autoload (guix build utils) (mkdir-p) > #:use-module (ice-9 match) > + #:use-module (ice-9 peg) > #:use-module (ice-9 rdelim) > #:use-module (ice-9 receive) > #:use-module (ice-9 regex) > @@ -244,129 +245,139 @@ and VERSION and return an input port." > (go-path-escape version)))) > (http-fetch* url))) > > -(define %go.mod-require-directive-rx > - ;; A line in a require directive is composed of a module path and > - ;; a version separated by whitespace and an optionnal '//' comment at > - ;; the end. > - (make-regexp > - (string-append > - "^[[:blank:]]*([^[:blank:]]+)[[:blank:]]+" ;the module path > - "([^[:blank:]]+)" ;the version > - "([[:blank:]]+//.*)?"))) ;an optional comment > - > -(define %go.mod-replace-directive-rx > - ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline > - ;; | ModulePath [ Version ] "=>" ModulePath Version newline . > - (make-regexp > - (string-append > - "([^[:blank:]]+)" ;the module path > - "([[:blank:]]+([^[:blank:]]+))?" ;optional version > - "[[:blank:]]+=>[[:blank:]]+" > - "([^[:blank:]]+)" ;the file or module path > - "([[:blank:]]+([^[:blank:]]+))?"))) ;the version (if a module path) > > (define (parse-go.mod content) > - "Parse the go.mod file CONTENT, returning a list of requirements." > - ;; We parse only a subset of https://golang.org/ref/mod#go-mod-file-grammar > - ;; which we think necessary for our use case. > - (define (toplevel requirements replaced) > - "This is the main parser. The results are accumulated in THE REQUIREMENTS > -and REPLACED lists." > - (let ((line (read-line))) > - (cond > - ((eof-object? line) > - ;; parsing ended, give back the result > - (values requirements replaced)) > - ((string=? line "require (") > - ;; a require block begins, delegate parsing to IN-REQUIRE > - (in-require requirements replaced)) > - ((string=? line "replace (") > - ;; a replace block begins, delegate parsing to IN-REPLACE > - (in-replace requirements replaced)) > - ((string-prefix? "require " line) > - ;; a require directive by itself > - (let* ((stripped-line (string-drop line 8))) > - (call-with-values > - (lambda () > - (require-directive requirements replaced stripped-line)) > - toplevel))) > - ((string-prefix? "replace " line) > - ;; a replace directive by itself > - (let* ((stripped-line (string-drop line 8))) > - (call-with-values > - (lambda () > - (replace-directive requirements replaced stripped-line)) > - toplevel))) > - (#t > - ;; unrecognised line, ignore silently > - (toplevel requirements replaced))))) > - > - (define (in-require requirements replaced) > - (let ((line (read-line))) > - (cond > - ((eof-object? line) > - ;; this should never happen here but we ignore silently > - (values requirements replaced)) > - ((string=? line ")") > - ;; end of block, coming back to toplevel > - (toplevel requirements replaced)) > - (#t > - (call-with-values (lambda () > - (require-directive requirements replaced line)) > - in-require))))) > - > - (define (in-replace requirements replaced) > - (let ((line (read-line))) > - (cond > - ((eof-object? line) > - ;; this should never happen here but we ignore silently > - (values requirements replaced)) > - ((string=? line ")") > - ;; end of block, coming back to toplevel > - (toplevel requirements replaced)) > - (#t > - (call-with-values (lambda () > - (replace-directive requirements replaced line)) > - in-replace))))) > - > - (define (replace-directive requirements replaced line) > - "Extract replaced modules and new requirements from the replace directive > -in LINE and add them to the REQUIREMENTS and REPLACED lists." > - (let* ((rx-match (regexp-exec %go.mod-replace-directive-rx line)) > - (module-path (match:substring rx-match 1)) > - (version (match:substring rx-match 3)) > - (new-module-path (match:substring rx-match 4)) > - (new-version (match:substring rx-match 6)) > - (new-replaced (cons (list module-path version) replaced)) > - (new-requirements > - (if (string-match "^\\.?\\./" new-module-path) > - requirements > - (cons (list new-module-path new-version) requirements)))) > - (values new-requirements new-replaced))) > - > - (define (require-directive requirements replaced line) > - "Extract requirement from LINE and augment the REQUIREMENTS and REPLACED > -lists." > - (let* ((rx-match (regexp-exec %go.mod-require-directive-rx line)) > - (module-path (match:substring rx-match 1)) > - ;; Double-quoted strings were seen in the wild without escape > - ;; sequences; trim the quotes to be on the safe side. > - (module-path (string-trim-both module-path #\")) > - (version (match:substring rx-match 2))) > - (values (cons (list module-path version) requirements) replaced))) > - > - (with-input-from-string content > - (lambda () > - (receive (requirements replaced) > - (toplevel '() '()) > - ;; At last remove the replaced modules from the requirements list. > - (remove (lambda (r) > - (assoc (car r) replaced)) > - requirements))))) > + "Parse the go.mod file CONTENT, returning a list of directives, comments, > +and unknown lines. Each sublist begins with a symbol (go, module, require, > +replace, exclude, retract, comment, or unknown) and is followed by one or more > +sublists. Each sublist begins with a symbol (module-path, version, file-path, > +comment, or unknown) and is followed by the indicated data." > + ;; https://golang.org/ref/mod#go-mod-file-grammar > + (define-peg-pattern NL none "\n") > + (define-peg-pattern WS none (or " " "\t" "\r")) > + (define-peg-pattern => none (and (* WS) "=>")) > + (define-peg-pattern punctuation none (or "," "=>" "[" "]" "(" ")")) > + (define-peg-pattern comment all > + (and (ignore "//") (* WS) (* (and (not-followed-by NL) peg-any)))) > + (define-peg-pattern EOL body (and (* WS) (? comment) NL)) > + (define-peg-pattern block-start none (and (* WS) "(" EOL)) > + (define-peg-pattern block-end none (and (* WS) ")" EOL))w > + (define-peg-pattern any-line body > + (and (* WS) (* (and (not-followed-by NL) peg-any)) EOL)) > + > + ;; Strings and identifiers > + (define-peg-pattern identifier body > + (+ (and (not-followed-by (or NL WS punctuation)) peg-any))) > + (define-peg-pattern string-raw body > + (and (ignore "`") (+ (and (not-followed-by "`") peg-any)) (ignore "`"))) > + (define-peg-pattern string-quoted body > + (and (ignore "\"") > + (+ (or (and (ignore "\\") peg-any) > + (and (not-followed-by "\"") peg-any))) > + (ignore "\""))) > + (define-peg-pattern string-or-ident body ^ Perhaps prefer the fully spelled out 'string-or-identifier' variable name. > + (and (* WS) (or string-raw string-quoted identifier))) > + > + (define-peg-pattern version all string-or-ident) > + (define-peg-pattern module-path all string-or-ident) > + (define-peg-pattern file-path all string-or-ident) > + > + ;; Non-directive lines > + (define-peg-pattern unknown all any-line) > + (define-peg-pattern block-line body > + (or EOL (and (not-followed-by block-end) unknown))) > + > + ;; GoDirective = "go" GoVersion newline . > + (define-peg-pattern go all (and (ignore "go") version EOL)) > + > + ;; ModuleDirective = "module" ( ModulePath | "(" newline ModulePath newline ")" ) newline . > + (define-peg-pattern module all > + (and (ignore "module") (or (and block-start module-path EOL block-end) > + (and module-path EOL)))) > + > + ;; The following directives may all be used solo or in a block > + ;; RequireSpec = ModulePath Version newline . > + (define-peg-pattern require all (and module-path version EOL)) > + (define-peg-pattern require-top body > + (and (ignore "require") > + (or (and block-start (* (or require block-line)) block-end) require))) > + > + ;; ExcludeSpec = ModulePath Version newline . > + (define-peg-pattern exclude all (and module-path version EOL)) > + (define-peg-pattern exclude-top body > + (and (ignore "exclude") > + (or (and block-start (* (or exclude block-line)) block-end) exclude))) > + > + ;; ReplaceSpec = ModulePath [ Version ] "=>" FilePath newline > + ;; | ModulePath [ Version ] "=>" ModulePath Version newline . > + (define-peg-pattern original all (or (and module-path version) module-path)) > + (define-peg-pattern with all (or (and module-path version) file-path)) > + (define-peg-pattern replace all (and original => with EOL)) > + (define-peg-pattern replace-top body > + (and (ignore "replace") > + (or (and block-start (* (or replace block-line)) block-end) replace))) > + > + ;; RetractSpec = ( Version | "[" Version "," Version "]" ) newline . > + (define-peg-pattern range all > + (and (* WS) (ignore "[") version > + (* WS) (ignore ",") version (* WS) (ignore "]"))) > + (define-peg-pattern retract all (and (or range version) EOL)) > + (define-peg-pattern retract-top body > + (and (ignore "retract") > + (or (and block-start (* (or retract block-line)) block-end) retract))) > + > + (define-peg-pattern go-mod body > + (* (and (* WS) (or go module require-top exclude-top replace-top > + retract-top EOL unknown)))) > + > + (let ((tree (peg:tree (match-pattern go-mod content))) > + (keywords '(go module require replace exclude retract comment unknown))) > + (keyword-flatten keywords tree))) Fun! > ;; Prevent inlining of this procedure, which is accessed by unit tests. > (set! parse-go.mod parse-go.mod) > > +(define (go.mod-directives go.mod directive) > + "Return the list of top-level directive bodies in GO.MOD matching the symbol > +DIRECTIVE." > + (filter-map (match-lambda > + (((? (cut eq? <> directive) head) . rest) rest) > + (_ #f)) > + go.mod)) > + > +(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 > + (if (and (equal? module-path (first new-requirement)) > + (not (assoc-ref requirements module-path))) > + requirements > + (cons new-requirement (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))))))) > + > + (define (require directive requirements) > + (match directive > + ((('module-path module-path) ('version version) . _) > + (cons (list module-path version) requirements)))) > + > + (let* ((requires (go.mod-directives go.mod 'require)) > + (replaces (go.mod-directives go.mod 'replace)) > + (requirements (fold require '() requires))) > + (fold replace requirements replaces))) > + > +;; Prevent inlining of this procedure, which is accessed by unit tests. > +(set! go.mod-requirements go.mod-requirements) > + > (define-record-type > (%make-vcs url-prefix root-regex type) > vcs? > @@ -588,7 +599,7 @@ When VERSION is unspecified, the latest version available is used." > hint: use one of the following available versions ~a\n" > version* available-versions)))) > (content (fetch-go.mod goproxy module-path version*)) > - (dependencies+versions (parse-go.mod content)) > + (dependencies+versions (go.mod-requirements (parse-go.mod content))) > (dependencies (if pin-versions? > dependencies+versions > (map car dependencies+versions))) > diff --git a/tests/go.scm b/tests/go.scm > index b088ab50d2..b16965d941 100644 > --- a/tests/go.scm > +++ b/tests/go.scm > @@ -1,5 +1,6 @@ > ;;; GNU Guix --- Functional package management for GNU > ;;; Copyright 2021 Franois Joulaud > +;;; Copyright 2021 Sarah Morgensen > ;;; > ;;; This file is part of GNU Guix. > ;;; > @@ -31,6 +32,9 @@ > #:use-module (srfi srfi-64) > #:use-module (web response)) > > +(define go.mod-requirements > + (@@ (guix import go) go.mod-requirements)) > + > (define parse-go.mod > (@@ (guix import go) parse-go.mod)) > > @@ -96,6 +100,40 @@ replace ( > > ") > > +(define fixture-go-mod-unparseable > + "module my/thing > +go 1.12 // avoid feature X > +require other/thing v1.0.2 > +// Security issue: CVE-XXXXX > +exclude old/thing v1.2.3 > +new-directive another/thing yet-another/thing > +replace ( > + bad/thing v1.4.5 => good/thing v1.4.5 > + // Unparseable > + bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0 > +) > +") > + > +(define fixture-go-mod-retract > + "retract v0.9.1 > + > +retract ( > + v1.9.2 > + [v1.0.0, v1.7.9] > +) > +") > + > +(define fixture-go-mod-strings > + "require `example.com/\"some-repo\"` v1.9.3 > +require ( > + `example.com/\"another.repo\"` v1.0.0 > + \"example.com/special!repo\" v9.3.1 > +) > +replace \"example.com/\\\"some-repo\\\"\" => `launchpad.net/some-repo` v1.9.3 > +replace ( > + \"example.com/\\\"another.repo\\\"\" => launchpad.net/another-repo v1.0.0 > +) > +") > > > (define fixture-latest-for-go-check > @@ -185,7 +223,7 @@ require github.com/kr/pretty v0.2.1 > (string (test-equal name > (sort expected inf?) > - (sort ((@@ (guix import go) parse-go.mod) input) inf?))) > + (sort (go.mod-requirements (parse-go.mod input)) inf?))) > > (testing-parse-mod "parse-go.mod-simple" > '(("good/thing" "v1.4.5") > @@ -221,6 +259,98 @@ require github.com/kr/pretty v0.2.1 > ("github.com/go-check/check" "v0.0.0-20140225173054-eb6ee6f84d0a")) > fixture-go-mod-complete) > > +(test-equal "parse-go.mod: simple" > + `((module (module-path "my/thing")) > + (go (version "1.12")) > + (require (module-path "other/thing") (version "v1.0.2")) > + (require (module-path "new/thing/v2") (version "v2.3.4")) > + (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")))) > + (parse-go.mod fixture-go-mod-simple)) > + > +(test-equal "parse-go.mod: comments and unparseable lines" > + `((module (module-path "my/thing")) > + (go (version "1.12") (comment "avoid feature X")) > + (require (module-path "other/thing") (version "v1.0.2")) > + (comment "Security issue: CVE-XXXXX") > + (exclude (module-path "old/thing") (version "v1.2.3")) > + (unknown "new-directive another/thing yet-another/thing") > + (replace (original (module-path "bad/thing") (version "v1.4.5")) > + (with (module-path "good/thing") (version "v1.4.5"))) > + (comment "Unparseable") > + (unknown "bad/thing [v1.4.5, v1.9.7] => good/thing v2.0.0")) > + (parse-go.mod fixture-go-mod-unparseable)) > + > +(test-equal "parse-go.mod: retract" > + `((retract (version "v0.9.1")) > + (retract (version "v1.9.2")) > + (retract (range (version "v1.0.0") (version "v1.7.9")))) > + (parse-go.mod fixture-go-mod-retract)) > + > +(test-equal "parse-go.mod: raw strings and quoted strings" > + `((require (module-path "example.com/\"some-repo\"") (version "v1.9.3")) > + (require (module-path "example.com/\"another.repo\"") (version "v1.0.0")) > + (require (module-path "example.com/special!repo") (version "v9.3.1")) > + (replace (original (module-path "example.com/\"some-repo\"")) > + (with (module-path "launchpad.net/some-repo") (version "v1.9.3"))) > + (replace (original (module-path "example.com/\"another.repo\"")) > + (with (module-path "launchpad.net/another-repo") (version "v1.0.0")))) > + (parse-go.mod fixture-go-mod-strings)) > + > +(test-equal "parse-go.mod: complete" > + `((module (module-path "M")) > + (go (version "1.13")) > + (replace (original (module-path "github.com/myname/myproject/myapi")) > + (with (file-path "./api"))) > + (replace (original (module-path "github.com/mymname/myproject/thissdk")) > + (with (file-path "../sdk"))) > + (replace (original (module-path "launchpad.net/gocheck")) > + (with (module-path "github.com/go-check/check") > + (version "v0.0.0-20140225173054-eb6ee6f84d0a"))) > + (require (module-path "github.com/user/project") > + (version "v1.1.11")) > + (require (module-path "github.com/user/project/sub/directory") > + (version "v1.1.12")) > + (require (module-path "bitbucket.org/user/project") > + (version "v1.11.20")) > + (require (module-path "bitbucket.org/user/project/sub/directory") > + (version "v1.11.21")) > + (require (module-path "launchpad.net/project") > + (version "v1.1.13")) > + (require (module-path "launchpad.net/project/series") > + (version "v1.1.14")) > + (require (module-path "launchpad.net/project/series/sub/directory") > + (version "v1.1.15")) > + (require (module-path "launchpad.net/~user/project/branch") > + (version "v1.1.16")) > + (require (module-path "launchpad.net/~user/project/branch/sub/directory") > + (version "v1.1.17")) > + (require (module-path "hub.jazz.net/git/user/project") > + (version "v1.1.18")) > + (require (module-path "hub.jazz.net/git/user/project/sub/directory") > + (version "v1.1.19")) > + (require (module-path "k8s.io/kubernetes/subproject") > + (version "v1.1.101")) > + (require (module-path "one.example.com/abitrary/repo") > + (version "v1.1.111")) > + (require (module-path "two.example.com/abitrary/repo") > + (version "v0.0.2")) > + (require (module-path "quoted.example.com/abitrary/repo") > + (version "v0.0.2")) > + (replace (original (module-path "two.example.com/abitrary/repo")) > + (with (module-path "github.com/corp/arbitrary-repo") > + (version "v0.0.2"))) > + (replace (original (module-path "golang.org/x/sys")) > + (with (module-path "golang.org/x/sys") > + (version "v0.0.0-20190813064441-fde4db37ae7a")) > + (comment "pinned to release-branch.go1.13")) > + (replace (original (module-path "golang.org/x/tools")) > + (with (module-path "golang.org/x/tools") > + (version "v0.0.0-20190821162956-65e3620a7ae7")) > + (comment "pinned to release-branch.go1.13"))) > + (parse-go.mod fixture-go-mod-complete)) > + > ;;; End-to-end tests for (guix import go) > (define (mock-http-fetch testcase) > (lambda (url . rest) > > base-commit: 3ee0f170c8bd883728d8abb2c2e00f445c13f17d LGTM! Pushed with commit 793ba333c6, after fixing up some indents and substituting the TODO for an explanatory comment with a reference to the replace directive, and a few more cosmetic changes. Thanks for the continued improvements to the Go importer :-). Closing. Maxim From debbugs-submit-bounces@debbugs.gnu.org Sun Jul 18 23:15:12 2021 Received: (at 49602) by debbugs.gnu.org; 19 Jul 2021 03:15:12 +0000 Received: from localhost ([127.0.0.1]:57753 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m5JkN-0000nG-RI for submit@debbugs.gnu.org; Sun, 18 Jul 2021 23:15:12 -0400 Received: from out2.migadu.com ([188.165.223.204]:20776) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m5JkJ-0000n0-7s for 49602@debbugs.gnu.org; Sun, 18 Jul 2021 23:15:10 -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=1626664505; 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=6gyxth01K+89CTPi3Iajt32JExWuX6nUO0ITq0Jgi3Q=; b=PMzirhEioQRGxb+W0UQEEvDxcXp9O/dCIaYDJPZAzQrhy7LNsNAaYhijvuFd1XCHO0Ohb7 xaCWZDL7my9A1RHicA7uA8MaKobEeKP6qLNa412G94sJJDJdkEZvwyV700YQf9KiFdLk6c Ao8XwpBxg15+kgqwPvo1WW4UqyYus3U= From: Sarah Morgensen To: Maxim Cournoyer Subject: Re: bug#49602: [PATCH] import: go: Upgrade go.mod parser. References: <8a452809f56c9b62b46a7639ff89148ffc845b3e.1626493564.git.iskarian@mgsn.dev> <87tuksjh68.fsf@gmail.com> Date: Sun, 18 Jul 2021 20:15:03 -0700 In-Reply-To: <87tuksjh68.fsf@gmail.com> (Maxim Cournoyer's message of "Sun, 18 Jul 2021 01:59:43 -0400") Message-ID: <86eebvt2o8.fsf@mgsn.dev> MIME-Version: 1.0 Content-Type: text/plain X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: iskarian@mgsn.dev X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 49602 Cc: 49602@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) Hello, Thanks for the review :) I just knew I wouldn't get that long commit message right the first time! Maxim Cournoyer writes: >> (%go.mod-require-directive-rx) > > The above line can be removed, as it is duplicated below. > >> (%go.mod-replace-directive-rx): Remove unused variable. Those are actually two distinct variables ("...-require-..." vs "...-replace-..."), though I should've written "variables." >> + (define-peg-pattern string-or-ident body > ^ > Perhaps prefer the fully spelled out 'string-or-identifier' variable name. > Agree. >> +(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. 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. > Pushed with commit 793ba333c6, after fixing up some indents and > substituting the TODO for an explanatory comment with a reference to the > replace directive, and a few more cosmetic changes. Thanks for the final touches. > Thanks for the continued improvements to the Go importer :-). Happy to help :) Originally, I was just trying to package a Go program, and look what happened... -- Sarah From debbugs-submit-bounces@debbugs.gnu.org Tue Aug 03 10:22:05 2021 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 To: Sarah Morgensen Subject: Re: bug#49602: [PATCH] import: go: Upgrade go.mod parser. 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-Spam-Score: 0.7 (/) X-Debbugs-Envelope-To: 49602 Cc: 49602@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.3 (/) --=-=-= 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 --=-=-=-- From debbugs-submit-bounces@debbugs.gnu.org Wed Aug 04 14:17:34 2021 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 To: Maxim Cournoyer Subject: Re: bug#49602: [PATCH] import: go: Upgrade go.mod parser. 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-Flow: FLOW_OUT X-Migadu-Auth-User: iskarian@mgsn.dev X-Spam-Score: 0.7 (/) X-Debbugs-Envelope-To: 49602 Cc: 49602@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.3 (/) 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" From debbugs-submit-bounces@debbugs.gnu.org Wed Aug 04 22:04:43 2021 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 To: Sarah Morgensen Subject: Re: bug#49602: [PATCH] import: go: Upgrade go.mod parser. 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-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 49602 Cc: 49602@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) 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 From debbugs-submit-bounces@debbugs.gnu.org Fri Aug 06 00:25:22 2021 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 To: Maxim Cournoyer Subject: Re: bug#49602: [PATCH] import: go: Upgrade go.mod parser. 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-Flow: FLOW_OUT X-Migadu-Auth-User: iskarian@mgsn.dev X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 49602 Cc: 49602@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --=-=-= 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 --=-=-=-- From unknown Sat Jun 14 03:52:58 2025 Received: (at fakecontrol) by fakecontrolmessage; To: internal_control@debbugs.gnu.org From: Debbugs Internal Request Subject: Internal Control Message-Id: bug archived. Date: Fri, 03 Sep 2021 11:24:04 +0000 User-Agent: Fakemail v42.6.9 # This is a fake control message. # # The action: # bug archived. thanks # This fakemail brought to you by your local debbugs # administrator