GNU bug report logs - #65866
[PATCH 0/8] Add built-in builder for Git checkouts

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 65866 in the body.
You can then email your comments to 65866 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: guix-patches <at> gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 11 Sep 2023 16:23:42 +0200

Hello Guix!

This patch series is a first step towards getting Git out of
derivation graphs when it’s only used to fetch source code
(origins with ‘git-fetch’), with the goal of fixing:

  https://issues.guix.gnu.org/63331

The is similar to how we solved the problem for regular file
downloads: we add a new “builtin:git-download” builder for
derivations, which is implemented on the daemon size by the
‘guix perform-download’ helper.  That command uses the same
code that is currently used by ‘git-fetch’.

Eventually, when users are all running recent versions of
‘guix-daemon’ with support for “builtin:git-download” (2–4
years from now?), we’ll be able to use “builtin:git-download”
unconditionally and thus be sure there are no risks of
derivation cycles.

Note that the patch series adds a hard dependency on Git.
This is because the existing ‘git-fetch’ code depends on Git,
which is itself motivated by the fact that Git supports
shallow clones and libgit2/Guile-Git doesn’t.

As a side effect, this dependency will prove useful to
address <https://issues.guix.gnu.org/65720>.

Thoughts?

Ludo’.

Ludovic Courtès (8):
  git-download: Move fallback code to (guix build git).
  git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment
    variable.
  perform-download: Remove unused one-argument clause.
  daemon: Add “git-download” built-in builder.
  build: Add dependency on Git.
  perform-git-download: Use the ‘git’ command captured at configure
    time.
  git-download: Use “builtin:git-download” when available.
  tests: Assume ‘git’ is always available.

 configure.ac                      |   7 ++
 doc/guix.texi                     |   1 +
 guix/build/git.scm                |  44 ++++++++++-
 guix/config.scm.in                |   6 +-
 guix/git-download.scm             | 122 ++++++++++++++++++------------
 guix/scripts/perform-download.scm |  59 +++++++++++----
 guix/self.scm                     |  10 ++-
 nix/libstore/builtins.cc          |   5 +-
 tests/builders.scm                |  29 ++++++-
 tests/channels.scm                |   7 +-
 tests/derivations.scm             |  94 ++++++++++++++++++++++-
 tests/git-authenticate.scm        |   1 -
 tests/git.scm                     |  10 ---
 tests/import-git.scm              |  18 -----
 14 files changed, 308 insertions(+), 105 deletions(-)


base-commit: a4c35c607cfd7d6b0bad90cfcc46188d489e1754
-- 
2.41.0

Message #8 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 1/8] git-download: Move fallback code to (guix build git).
Date: Mon, 11 Sep 2023 16:25:19 +0200

* guix/build/git.scm (git-fetch-with-fallback): New procedure, with code
taken from…
* guix/git-download.scm (git-fetch): … here.
[modules]: Remove modules that are no longer directly used in ‘build’.
[build]: Use ‘git-fetch-with-fallback’.
---
 guix/build/git.scm    | 44 ++++++++++++++++++++++++++++++++++++++--
 guix/git-download.scm | 47 ++++++++-----------------------------------
 2 files changed, 50 insertions(+), 41 deletions(-)

diff --git a/guix/build/git.scm b/guix/build/git.scm
index deda10fee8..0ff263c81b 100644
--- a/guix/build/git.scm
+++ b/guix/build/git.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2014, 2016, 2019 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2014, 2016, 2019, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -18,9 +18,12 @@
 
 (define-module (guix build git)
   #:use-module (guix build utils)
+  #:autoload   (guix build download-nar) (download-nar)
+  #:autoload   (guix swh) (%verify-swh-certificate? swh-download)
   #:use-module (srfi srfi-34)
   #:use-module (ice-9 format)
-  #:export (git-fetch))
+  #:export (git-fetch
+            git-fetch-with-fallback))
 
 ;;; Commentary:
 ;;;
@@ -76,4 +79,41 @@ (define* (git-fetch url commit directory
       (delete-file-recursively ".git")
       #t)))
 
+
+(define* (git-fetch-with-fallback url commit directory
+                                  #:key (git-command "git") recursive?)
+  "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
+alternative methods when fetching from URL fails: attempt to download a nar,
+and if that also fails, download from the Software Heritage archive."
+  (or (git-fetch url commit directory
+                 #:recursive? recursive?
+                 #:git-command git-command)
+      (download-nar directory)
+
+      ;; As a last resort, attempt to download from Software Heritage.
+      ;; Disable X.509 certificate verification to avoid depending
+      ;; on nss-certs--we're authenticating the checkout anyway.
+      ;; XXX: Currently recursive checkouts are not supported.
+      (and (not recursive?)
+           (parameterize ((%verify-swh-certificate? #f))
+             (format (current-error-port)
+                     "Trying to download from Software Heritage...~%")
+
+             (swh-download url commit directory)
+             (when (file-exists?
+                    (string-append directory "/.gitattributes"))
+               ;; Perform CR/LF conversion and other changes
+               ;; specificied by '.gitattributes'.
+               (invoke git-command "-C" directory "init")
+               (invoke git-command "-C" directory "config" "--local"
+                       "user.email" "you <at> example.org")
+               (invoke git-command "-C" directory "config" "--local"
+                       "user.name" "Your Name")
+               (invoke git-command "-C" directory "add" ".")
+               (invoke git-command "-C" directory "commit" "-am" "init")
+               (invoke git-command "-C" directory "read-tree" "--empty")
+               (invoke git-command "-C" directory "reset" "--hard")
+               (delete-file-recursively
+                (string-append directory "/.git")))))))
+
 ;;; git.scm ends here
diff --git a/guix/git-download.scm b/guix/git-download.scm
index d88f4c40ee..8989b1b463 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -116,19 +116,16 @@ (define* (git-fetch ref hash-algo hash
   (define modules
     (delete '(guix config)
             (source-module-closure '((guix build git)
-                                     (guix build utils)
-                                     (guix build download-nar)
-                                     (guix swh)))))
+                                     (guix build utils)))))
 
   (define build
     (with-imported-modules modules
-      (with-extensions (list guile-json gnutls   ;for (guix swh)
+      (with-extensions (list guile-json gnutls    ;for (guix swh)
                              guile-lzlib)
         #~(begin
             (use-modules (guix build git)
-                         (guix build utils)
-                         (guix build download-nar)
-                         (guix swh)
+                         ((guix build utils)
+                          #:select (set-path-environment-variable))
                          (ice-9 match))
 
             (define recursive?
@@ -151,38 +148,10 @@ (define* (git-fetch ref hash-algo hash
             (setvbuf (current-output-port) 'line)
             (setvbuf (current-error-port) 'line)
 
-            (or (git-fetch (getenv "git url") (getenv "git commit")
-                           #$output
-                           #:recursive? recursive?
-                           #:git-command "git")
-                (download-nar #$output)
-
-                ;; As a last resort, attempt to download from Software Heritage.
-                ;; Disable X.509 certificate verification to avoid depending
-                ;; on nss-certs--we're authenticating the checkout anyway.
-                ;; XXX: Currently recursive checkouts are not supported.
-                (and (not recursive?)
-                     (parameterize ((%verify-swh-certificate? #f))
-                       (format (current-error-port)
-                               "Trying to download from Software Heritage...~%")
-
-                       (swh-download (getenv "git url") (getenv "git commit")
-                                     #$output)
-                       (when (file-exists?
-                              (string-append #$output "/.gitattributes"))
-                         ;; Perform CR/LF conversion and other changes
-                         ;; specificied by '.gitattributes'.
-                         (invoke "git" "-C" #$output "init")
-                         (invoke "git" "-C" #$output "config" "--local"
-                                 "user.email" "you <at> example.org")
-                         (invoke "git" "-C" #$output "config" "--local"
-                                 "user.name" "Your Name")
-                         (invoke "git" "-C" #$output "add" ".")
-                         (invoke "git" "-C" #$output "commit" "-am" "init")
-                         (invoke "git" "-C" #$output "read-tree" "--empty")
-                         (invoke "git" "-C" #$output "reset" "--hard")
-                         (delete-file-recursively
-                          (string-append #$output "/.git"))))))))))
+            (git-fetch-with-fallback (getenv "git url") (getenv "git commit")
+                                     #$output
+                                     #:recursive? recursive?
+                                     #:git-command "git")))))
 
   (mlet %store-monad ((guile (package->derivation guile system)))
     (gexp->derivation (or name "git-checkout") build
-- 
2.41.0

Message #11 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 2/8] git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment variable.
Date: Mon, 11 Sep 2023 16:25:20 +0200

* guix/git-download.scm (git-fetch): Honor ‘%download-fallback-test’.
---
 guix/git-download.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/guix/git-download.scm b/guix/git-download.scm
index 8989b1b463..f1f19397c6 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -28,6 +28,7 @@ (define-module (guix git-download)
   #:use-module (guix packages)
   #:use-module (guix modules)
   #:autoload   (guix build-system gnu) (standard-packages)
+  #:autoload   (guix download) (%download-fallback-test)
   #:autoload   (git bindings)   (libgit2-init!)
   #:autoload   (git repository) (repository-open
                                  repository-close!
@@ -161,7 +162,11 @@ (define* (git-fetch ref hash-algo hash
                       ;; downloads.
                       #:script-name "git-download"
                       #:env-vars
-                      `(("git url" . ,(git-reference-url ref))
+                      `(("git url" . ,(match (%download-fallback-test)
+                                        ('content-addressed-mirrors
+                                         "https://example.org/does-not-exist")
+                                        (_
+                                         (git-reference-url ref))))
                         ("git commit" . ,(git-reference-commit ref))
                         ("git recursive?" . ,(object->string
                                               (git-reference-recursive? ref))))
-- 
2.41.0

Message #14 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 3/8] perform-download: Remove unused one-argument clause.
Date: Mon, 11 Sep 2023 16:25:21 +0200

Code in ‘builtins.cc’ only ever invokes ‘guix perform-download’ with two
arguments.

* guix/scripts/perform-download.scm (guix-perform-download): Remove
unused one-argument clause.
---
 guix/scripts/perform-download.scm | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index 6889bcef79..c8f044e82e 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -120,13 +120,8 @@ (define-command (guix-perform-download . args)
     (match args
       (((? derivation-path? drv) (? store-path? output))
        (assert-low-privileges)
-       (perform-download (read-derivation-from-file drv)
-                         output
-                         #:print-build-trace? print-build-trace?))
-      (((? derivation-path? drv))                 ;backward compatibility
-       (assert-low-privileges)
-       (perform-download (read-derivation-from-file drv)
-                         #:print-build-trace? print-build-trace?))
+       (let ((drv (read-derivation-from-file drv)))
+         (perform-download drv output #:print-build-trace? print-build-trace?)))
       (("--version")
        (show-version-and-exit))
       (x
-- 
2.41.0

Message #17 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludovic.courtes <at> inria.fr>
Subject: [PATCH 4/8] daemon: Add “git-download” built-in builder.
Date: Mon, 11 Sep 2023 16:25:22 +0200

From: Ludovic Courtès <ludovic.courtes <at> inria.fr>

The new builder makes it possible to break cycles that occurs when the
fixed-output derivation for the source of a dependency of ‘git’ would
itself depend on ‘git’.

* guix/scripts/perform-download.scm (perform-git-download): New
procedure.
(perform-download): Move fixed-output derivation check to…
(guix-perform-download): … here.  Invoke ‘perform-download’ or
‘perform-git-download’ depending on what ‘derivation-builder’ returns.
* nix/libstore/builtins.cc (builtins): Add “git-download”.
* tests/derivations.scm ("built-in-builders"): Update.
("'git-download' built-in builder")
("'git-download' built-in builder, invalid hash")
("'git-download' built-in builder, invalid commit")
("'git-download' built-in builder, not found"): New tests.
---
 guix/scripts/perform-download.scm |  52 +++++++++++++---
 nix/libstore/builtins.cc          |   5 +-
 tests/derivations.scm             | 100 +++++++++++++++++++++++++++++-
 3 files changed, 145 insertions(+), 12 deletions(-)

diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index c8f044e82e..a287e97528 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2016, 2017, 2018, 2020 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2016-2018, 2020, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -21,7 +21,8 @@ (define-module (guix scripts perform-download)
   #:use-module (guix scripts)
   #:use-module (guix derivations)
   #:use-module ((guix store) #:select (derivation-path? store-path?))
-  #:use-module (guix build download)
+  #:autoload   (guix build download) (url-fetch)
+  #:autoload   (guix build git) (git-fetch-with-fallback)
   #:use-module (ice-9 match)
   #:export (guix-perform-download))
 
@@ -64,10 +65,6 @@ (define* (perform-download drv #:optional output
            (drv-output (assoc-ref (derivation-outputs drv) "out"))
            (algo       (derivation-output-hash-algo drv-output))
            (hash       (derivation-output-hash drv-output)))
-      (unless (and algo hash)
-        (leave (G_ "~a is not a fixed-output derivation~%")
-               (derivation-file-name drv)))
-
       ;; We're invoked by the daemon, which gives us write access to OUTPUT.
       (when (url-fetch url output
                        #:print-build-trace? print-build-trace?
@@ -92,6 +89,33 @@ (define* (perform-download drv #:optional output
         (when (and executable (string=? executable "1"))
           (chmod output #o755))))))
 
+(define* (perform-git-download drv #:optional output
+                               #:key print-build-trace?)
+  "Perform the download described by DRV, a fixed-output derivation, to
+OUTPUT.
+
+Note: Unless OUTPUT is #f, we don't read the value of 'out' in DRV since the
+actual output is different from that when we're doing a 'bmCheck' or
+'bmRepair' build."
+  (derivation-let drv ((output* "out")
+                       (url "url")
+                       (commit "commit")
+                       (recursive? "recursive?"))
+    (unless url
+      (leave (G_ "~a: missing Git URL~%") (derivation-file-name drv)))
+    (unless commit
+      (leave (G_ "~a: missing Git commit~%") (derivation-file-name drv)))
+
+    (let* ((output     (or output output*))
+           (url        (call-with-input-string url read))
+           (recursive? (and recursive?
+                            (call-with-input-string recursive? read)))
+           (drv-output (assoc-ref (derivation-outputs drv) "out"))
+           (algo       (derivation-output-hash-algo drv-output))
+           (hash       (derivation-output-hash drv-output)))
+      (git-fetch-with-fallback url commit output
+                               #:recursive? recursive?))))
+
 (define (assert-low-privileges)
   (when (zero? (getuid))
     (leave (G_ "refusing to run with elevated privileges (UID ~a)~%")
@@ -120,8 +144,20 @@ (define-command (guix-perform-download . args)
     (match args
       (((? derivation-path? drv) (? store-path? output))
        (assert-low-privileges)
-       (let ((drv (read-derivation-from-file drv)))
-         (perform-download drv output #:print-build-trace? print-build-trace?)))
+       (let* ((drv (read-derivation-from-file drv))
+              (download (match (derivation-builder drv)
+                          ("builtin:download" perform-download)
+                          ("builtin:git-download" perform-git-download)
+                          (unknown (leave (G_ "~a: unknown builtin builder")
+                                          unknown))))
+              (drv-output (assoc-ref (derivation-outputs drv) "out"))
+              (algo       (derivation-output-hash-algo drv-output))
+              (hash       (derivation-output-hash drv-output)))
+         (unless (and hash algo)
+           (leave (G_ "~a is not a fixed-output derivation~%")
+                  (derivation-file-name drv)))
+
+         (download drv output #:print-build-trace? print-build-trace?)))
       (("--version")
        (show-version-and-exit))
       (x
diff --git a/nix/libstore/builtins.cc b/nix/libstore/builtins.cc
index 4111ac4760..6bf467354a 100644
--- a/nix/libstore/builtins.cc
+++ b/nix/libstore/builtins.cc
@@ -1,5 +1,5 @@
 /* GNU Guix --- Functional package management for GNU
-   Copyright (C) 2016, 2017, 2018, 2019 Ludovic Courtès <ludo <at> gnu.org>
+   Copyright (C) 2016-2019, 2023 Ludovic Courtès <ludo <at> gnu.org>
 
    This file is part of GNU Guix.
 
@@ -58,7 +58,8 @@ static void builtinDownload(const Derivation &drv,
 
 static const std::map<std::string, derivationBuilder> builtins =
 {
-    { "download", builtinDownload }
+    { "download", builtinDownload },
+    { "git-download", builtinDownload }
 };
 
 derivationBuilder lookupBuiltinBuilder(const std::string & name)
diff --git a/tests/derivations.scm b/tests/derivations.scm
index 66c777cfe7..e1312bd46b 100644
--- a/tests/derivations.scm
+++ b/tests/derivations.scm
@@ -24,10 +24,15 @@ (define-module (test-derivations)
   #:use-module (guix utils)
   #:use-module ((gcrypt hash) #:prefix gcrypt:)
   #:use-module (guix base32)
+  #:use-module ((guix git) #:select (with-repository))
   #:use-module (guix tests)
+  #:use-module (guix tests git)
   #:use-module (guix tests http)
   #:use-module ((guix packages) #:select (package-derivation base32))
-  #:use-module ((guix build utils) #:select (executable-file?))
+  #:use-module ((guix build utils) #:select (executable-file? which))
+  #:use-module ((guix hash) #:select (file-hash*))
+  #:use-module ((git oid) #:select (oid->string))
+  #:use-module ((git reference) #:select (reference-name->oid))
   #:use-module (gnu packages bootstrap)
   #:use-module ((gnu packages guile) #:select (guile-1.8))
   #:use-module (srfi srfi-1)
@@ -195,7 +200,7 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
                    (stat:ino (lstat file2))))))))
 
 (test-equal "built-in-builders"
-  '("download")
+  '("download" "git-download")
   (built-in-builders %store))
 
 (test-assert "unknown built-in builder"
@@ -290,6 +295,97 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
                          get-string-all)
                        text))))))
 
+;; 'with-temporary-git-repository' relies on the 'git' command.
+(unless (which (git-command)) (test-skip 1))
+(test-equal "'git-download' built-in builder"
+  `(("/a.txt" . "AAA")
+    ("/b.scm" . "#t"))
+  (let ((nonce (random-text)))
+    (with-temporary-git-repository directory
+        `((add "a.txt" "AAA")
+          (add "b.scm" "#t")
+          (commit ,nonce))
+      (let* ((commit (with-repository directory repository
+                       (oid->string
+                        (reference-name->oid repository "HEAD"))))
+             (drv (derivation %store "git-download"
+                              "builtin:git-download" '()
+                              #:env-vars
+                              `(("url"
+                                 . ,(object->string
+                                     (string-append "file://" directory)))
+                                ("commit" . ,commit))
+                              #:hash-algo 'sha256
+                              #:hash (file-hash* directory
+                                                 #:algorithm
+                                                 (gcrypt:hash-algorithm
+                                                  gcrypt:sha256)
+                                                 #:recursive? #t)
+                              #:recursive? #t)))
+        (build-derivations %store (list drv))
+        (directory-contents (derivation->output-path drv) get-string-all)))))
+
+(unless (which (git-command)) (test-skip 1))
+(test-assert "'git-download' built-in builder, invalid hash"
+  (with-temporary-git-repository directory
+      `((add "a.txt" "AAA")
+        (add "b.scm" "#t")
+        (commit "Commit!"))
+    (let* ((commit (with-repository directory repository
+                     (oid->string
+                      (reference-name->oid repository "HEAD"))))
+           (drv (derivation %store "git-download"
+                            "builtin:git-download" '()
+                            #:env-vars
+                            `(("url"
+                               . ,(object->string
+                                   (string-append "file://" directory)))
+                              ("commit" . ,commit))
+                            #:hash-algo 'sha256
+                            #:hash (gcrypt:sha256 #vu8())
+                            #:recursive? #t)))
+      (guard (c ((store-protocol-error? c)
+                 (string-contains (store-protocol-error-message c) "failed")))
+        (build-derivations %store (list drv))
+        #f))))
+
+(unless (which (git-command)) (test-skip 1))
+(test-assert "'git-download' built-in builder, invalid commit"
+  (with-temporary-git-repository directory
+      `((add "a.txt" "AAA")
+        (add "b.scm" "#t")
+        (commit "Commit!"))
+    (let* ((drv (derivation %store "git-download"
+                            "builtin:git-download" '()
+                            #:env-vars
+                            `(("url"
+                               . ,(object->string
+                                   (string-append "file://" directory)))
+                              ("commit"
+                               . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
+                            #:hash-algo 'sha256
+                            #:hash (gcrypt:sha256 #vu8())
+                            #:recursive? #t)))
+      (guard (c ((store-protocol-error? c)
+                 (string-contains (store-protocol-error-message c) "failed")))
+        (build-derivations %store (list drv))
+        #f))))
+
+(test-assert "'git-download' built-in builder, not found"
+  (let* ((drv (derivation %store "git-download"
+                          "builtin:git-download" '()
+                          #:env-vars
+                          `(("url" . "file:///does-not-exist.git")
+                            ("commit"
+                             . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
+                          #:hash-algo 'sha256
+                          #:hash (gcrypt:sha256 #vu8())
+                          #:recursive? #t)))
+    (guard (c ((store-protocol-error? c)
+               (string-contains (store-protocol-error-message c) "failed")))
+      (build-derivations %store (list drv))
+      #f)))
+
 (test-equal "derivation-name"
   "foo-0.0"
   (let ((drv (derivation %store "foo-0.0" %bash '())))
-- 
2.41.0

Message #20 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 6/8] perform-git-download: Use the ‘git’ command captured at configure time.
Date: Mon, 11 Sep 2023 16:25:24 +0200

* guix/scripts/perform-download.scm (perform-git-download): Pass #:git-command
to ‘git-fetch-with-fallback’.
---
 guix/scripts/perform-download.scm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index a287e97528..e1c584fbda 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -23,6 +23,7 @@ (define-module (guix scripts perform-download)
   #:use-module ((guix store) #:select (derivation-path? store-path?))
   #:autoload   (guix build download) (url-fetch)
   #:autoload   (guix build git) (git-fetch-with-fallback)
+  #:autoload   (guix config) (%git)
   #:use-module (ice-9 match)
   #:export (guix-perform-download))
 
@@ -114,7 +115,8 @@ (define* (perform-git-download drv #:optional output
            (algo       (derivation-output-hash-algo drv-output))
            (hash       (derivation-output-hash drv-output)))
       (git-fetch-with-fallback url commit output
-                               #:recursive? recursive?))))
+                               #:recursive? recursive?
+                               #:git-command %git))))
 
 (define (assert-low-privileges)
   (when (zero? (getuid))
-- 
2.41.0

Message #23 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 7/8] git-download: Use “builtin:git-download” when available.
Date: Mon, 11 Sep 2023 16:25:25 +0200

Fixes <https://issues.guix.gnu.org/63331>.

Longer-term this will remove Git from the derivation graph when its sole
use is to perform a checkout for a fixed-output derivation, thereby
breaking dependency cycles that can arise in these situations.

* guix/git-download.scm (git-fetch): Rename to…
(git-fetch/in-band): … this.  Deal with GIT or GUILE being #f.
(git-fetch/built-in, built-in-builders*, git-fetch): New procedures.
* tests/builders.scm ("git-fetch, file URI"): New test.
---
 guix/git-download.scm | 68 +++++++++++++++++++++++++++++++++++++------
 tests/builders.scm    | 29 +++++++++++++++++-
 2 files changed, 87 insertions(+), 10 deletions(-)

diff --git a/guix/git-download.scm b/guix/git-download.scm
index f1f19397c6..505dff0a89 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -27,6 +27,7 @@ (define-module (guix git-download)
   #:use-module (guix records)
   #:use-module (guix packages)
   #:use-module (guix modules)
+  #:use-module ((guix derivations) #:select (raw-derivation))
   #:autoload   (guix build-system gnu) (standard-packages)
   #:autoload   (guix download) (%download-fallback-test)
   #:autoload   (git bindings)   (libgit2-init!)
@@ -78,15 +79,19 @@ (define (git-package)
   (let ((distro (resolve-interface '(gnu packages version-control))))
     (module-ref distro 'git-minimal)))
 
-(define* (git-fetch ref hash-algo hash
-                    #:optional name
-                    #:key (system (%current-system)) (guile (default-guile))
-                    (git (git-package)))
-  "Return a fixed-output derivation that fetches REF, a <git-reference>
-object.  The output is expected to have recursive hash HASH of type
-HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
+(define* (git-fetch/in-band ref hash-algo hash
+                            #:optional name
+                            #:key (system (%current-system))
+                            (guile (default-guile))
+                            (git (git-package)))
+  "Return a fixed-output derivation that performs a Git checkout of REF, using
+GIT and GUILE (thus, said derivation depends on GIT and GUILE).
+
+This method is deprecated in favor of the \"builtin:git-download\" builder.
+It will be removed when versions of guix-daemon implementing
+\"builtin:git-download\" will be sufficiently widespread."
   (define inputs
-    `(("git" ,git)
+    `(("git" ,(or git (git-package)))
 
       ;; When doing 'git clone --recursive', we need sed, grep, etc. to be
       ;; available so that 'git submodule' works.
@@ -154,7 +159,8 @@ (define* (git-fetch ref hash-algo hash
                                      #:recursive? recursive?
                                      #:git-command "git")))))
 
-  (mlet %store-monad ((guile (package->derivation guile system)))
+  (mlet %store-monad ((guile (package->derivation (or guile (default-guile))
+                                                  system)))
     (gexp->derivation (or name "git-checkout") build
 
                       ;; Use environment variables and a fixed script name so
@@ -181,6 +187,50 @@ (define* (git-fetch ref hash-algo hash
                       #:recursive? #t
                       #:guile-for-build guile)))
 
+(define* (git-fetch/built-in ref hash-algo hash
+                             #:optional name
+                             #:key (system (%current-system)))
+  "Return a fixed-output derivation without any dependency that performs a Git
+checkout of REF, using the \"builtin:git-download\" derivation builder."
+  (raw-derivation (or name "git-checkout") "builtin:git-download" '()
+                  #:system system
+                  #:hash-algo hash-algo
+                  #:hash hash
+                  #:recursive? #t
+                  #:env-vars
+                  `(("url" . ,(object->string
+                               (match (%download-fallback-test)
+                                 ('content-addressed-mirrors
+                                  "https://example.org/does-not-exist")
+                                 (_
+                                  (git-reference-url ref)))))
+                    ("commit" . ,(git-reference-commit ref))
+                    ("recursive?" . ,(object->string
+                                      (git-reference-recursive? ref))))
+                  #:leaked-env-vars '("http_proxy" "https_proxy"
+                                      "LC_ALL" "LC_MESSAGES" "LANG"
+                                      "COLUMNS")
+                  #:local-build? #t))
+
+(define built-in-builders*
+  (store-lift built-in-builders))
+
+(define* (git-fetch ref hash-algo hash
+                    #:optional name
+                    #:key (system (%current-system))
+                    guile git)
+  "Return a fixed-output derivation that fetches REF, a <git-reference>
+object.  The output is expected to have recursive hash HASH of type
+HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
+  (mlet %store-monad ((builtins (built-in-builders*)))
+    (if (member "git-download" builtins)
+        (git-fetch/built-in ref hash-algo hash name
+                            #:system system)
+        (git-fetch/in-band ref hash-algo hash name
+                           #:system system
+                           #:guile guile
+                           #:git git))))
+
 (define (git-version version revision commit)
   "Return the version string for packages using git-download."
   ;; git-version is almost exclusively executed while modules are being loaded.
diff --git a/tests/builders.scm b/tests/builders.scm
index 0b5577c7a3..619caa5f31 100644
--- a/tests/builders.scm
+++ b/tests/builders.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2018, 2019, 2021 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2021 Lars-Dominik Braun <lars <at> 6xq.net>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -20,6 +20,7 @@
 
 (define-module (tests builders)
   #:use-module (guix download)
+  #:use-module (guix git-download)
   #:use-module (guix build-system)
   #:use-module (guix build-system gnu)
   #:use-module (guix build gnu-build-system)
@@ -31,9 +32,12 @@ (define-module (tests builders)
   #:use-module (guix base32)
   #:use-module (guix derivations)
   #:use-module (gcrypt hash)
+  #:use-module ((guix hash) #:select (file-hash*))
   #:use-module (guix tests)
+  #:use-module (guix tests git)
   #:use-module (guix packages)
   #:use-module (gnu packages bootstrap)
+  #:use-module ((ice-9 ftw) #:select (scandir))
   #:use-module (ice-9 match)
   #:use-module (ice-9 textual-ports)
   #:use-module (srfi srfi-1)
@@ -84,6 +88,29 @@ (define url-fetch*
     (and (file-exists? out)
          (valid-path? %store out))))
 
+(test-equal "git-fetch, file URI"
+  '("." ".." "a.txt" "b.scm")
+  (let ((nonce (random-text)))
+    (with-temporary-git-repository directory
+        `((add "a.txt" ,nonce)
+          (add "b.scm" "#t")
+          (commit "Commit.")
+          (tag "v1.0.0" "The tag."))
+      (run-with-store %store
+        (mlet* %store-monad ((hash
+                              -> (file-hash* directory
+                                             #:algorithm (hash-algorithm sha256)
+                                             #:recursive? #t))
+                             (drv (git-fetch
+                                   (git-reference
+                                    (url (string-append "file://" directory))
+                                    (commit "v1.0.0"))
+                                   'sha256 hash
+                                   "git-fetch-test")))
+          (mbegin %store-monad
+            (built-derivations (list drv))
+            (return (scandir (derivation->output-path drv)))))))))
+
 (test-assert "gnu-build-system"
   (build-system? gnu-build-system))
 
-- 
2.41.0

Message #26 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 5/8] build: Add dependency on Git.
Date: Mon, 11 Sep 2023 16:25:23 +0200

* configure.ac: Check for ‘git’ and substitute ‘GIT’.
* guix/config.scm.in (%git): New variable.
* guix/self.scm (compiled-guix): Define ‘git’ and pass it to
‘make-config.scm’.
(make-config.scm): Add #:git; emit a ‘%git’ variable.
* doc/guix.texi (Requirements): Add it.
---
 configure.ac       |  7 +++++++
 doc/guix.texi      |  1 +
 guix/config.scm.in |  6 +++++-
 guix/self.scm      | 10 +++++++++-
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 92dede8014..d817f620cf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -201,6 +201,13 @@ AC_SUBST([GZIP])
 AC_SUBST([BZIP2])
 AC_SUBST([XZ])
 
+dnl Git is now required for the "builtin:git-download" derivation builder.
+AC_PATH_PROG([GIT], [git])
+if test "x$GIT" = "x"; then
+  AC_MSG_ERROR([Git is missing; please install it.])
+fi
+AC_SUBST([GIT])
+
 LIBGCRYPT_LIBDIR="no"
 LIBGCRYPT_PREFIX="no"
 
diff --git a/doc/guix.texi b/doc/guix.texi
index 339dcb2a41..a2520ce89d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -1011,6 +1011,7 @@ Requirements
 @item
 @uref{https://gitlab.com/guile-git/guile-git, Guile-Git}, version 0.5.0
 or later;
+@item @uref{https://git-scm.com, Git} (yes, both!);
 @item @uref{https://savannah.nongnu.org/projects/guile-json/, Guile-JSON}
 4.3.0 or later;
 @item @url{https://www.gnu.org/software/make/, GNU Make}.
diff --git a/guix/config.scm.in b/guix/config.scm.in
index d582d91d74..62e15dd713 100644
--- a/guix/config.scm.in
+++ b/guix/config.scm.in
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2018, 2019, 2021 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2012-2016, 2018-2019, 2021, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2017 Caleb Ristvedt <caleb.ristvedt <at> cune.org>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -35,6 +35,7 @@ (define-module (guix config)
             %config-directory
 
             %system
+            %git
             %gzip
             %bzip2
             %xz))
@@ -109,6 +110,9 @@ (define %config-directory
 (define %system
   "@guix_system@")
 
+(define %git
+  "@GIT@")
+
 (define %gzip
   "@GZIP@")
 
diff --git a/guix/self.scm b/guix/self.scm
index 81a36e007f..41c5f40786 100644
--- a/guix/self.scm
+++ b/guix/self.scm
@@ -68,6 +68,7 @@ (define %packages
       ("gzip"               . ,(ref 'compression 'gzip))
       ("bzip2"              . ,(ref 'compression 'bzip2))
       ("xz"                 . ,(ref 'compression 'xz))
+      ("git-minimal"        . ,(ref 'version-control 'git-minimal))
       ("po4a"               . ,(ref 'gettext 'po4a))
       ("gettext-minimal"    . ,(ref 'gettext 'gettext-minimal))
       ("gcc-toolchain"      . ,(ref 'commencement 'gcc-toolchain))
@@ -825,6 +826,9 @@ (define* (compiled-guix source #:key
   (define guile-lzma
     (specification->package "guile-lzma"))
 
+  (define git
+    (specification->package "git-minimal"))
+
   (define dependencies
     (append-map transitive-package-dependencies
                 (list guile-gcrypt guile-gnutls guile-git guile-avahi
@@ -998,6 +1002,7 @@ (define* (compiled-guix source #:key
                     => ,(make-config.scm #:gzip gzip
                                          #:bzip2 bzip2
                                          #:xz xz
+                                         #:git git
                                          #:package-name
                                          %guix-package-name
                                          #:package-version
@@ -1103,7 +1108,7 @@ (define %default-config-variables
     (%storedir . "/gnu/store")
     (%sysconfdir . "/etc")))
 
-(define* (make-config.scm #:key gzip xz bzip2
+(define* (make-config.scm #:key gzip xz bzip2 git
                           (package-name "GNU Guix")
                           (package-version "0")
                           (channel-metadata #f)
@@ -1133,6 +1138,7 @@ (define* (make-config.scm #:key gzip xz bzip2
                                %state-directory
                                %store-database-directory
                                %config-directory
+                               %git
                                %gzip
                                %bzip2
                                %xz))
@@ -1175,6 +1181,8 @@ (define* (make-config.scm #:key gzip xz bzip2
                      ;; information is used by (guix describe).
                      '#$channel-metadata)
 
+                   (define %git
+                     #+(and git (file-append git "/bin/git")))
                    (define %gzip
                      #+(and gzip (file-append gzip "/bin/gzip")))
                    (define %bzip2
-- 
2.41.0

Message #29 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 8/8] tests: Assume ‘git’ is always available.
Date: Mon, 11 Sep 2023 16:25:26 +0200

* tests/channels.scm (gpg+git-available?): Check for ‘gpg-command’
only.
Remove all ‘test-skip’ statements.
* tests/derivations.scm: Likewise.
* tests/git-authenticate.scm: Likewise.
* tests/git.scm: Likewise.
* tests/import-git.scm: Likewise.
---
 tests/channels.scm         |  7 +------
 tests/derivations.scm      |  6 +-----
 tests/git-authenticate.scm |  1 -
 tests/git.scm              | 10 ----------
 tests/import-git.scm       | 18 ------------------
 5 files changed, 2 insertions(+), 40 deletions(-)

diff --git a/tests/channels.scm b/tests/channels.scm
index 62312e240c..6c4276deb4 100644
--- a/tests/channels.scm
+++ b/tests/channels.scm
@@ -50,7 +50,7 @@ (define-module (test-channels)
   #:use-module (ice-9 match))
 
 (define (gpg+git-available?)
-  (and (which (git-command))
+  (and #t                                         ;'git' is always available
        (which (gpg-command)) (which (gpgconf-command))))
 
 (define commit-id-string
@@ -196,7 +196,6 @@ (define channel-metadata-dependencies
                                           "abc1234")))
                          instances)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-channel-instances #:validate-pull"
   'descendant
 
@@ -306,7 +305,6 @@ (define channel-metadata-dependencies
                (depends? drv3
                          (list drv2 drv0) (list))))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "channel-news, no news"
   '()
   (with-temporary-git-repository directory
@@ -318,7 +316,6 @@ (define channel-metadata-dependencies
             (latest  (reference-name->oid repository "HEAD")))
         (channel-news-for-commit channel (oid->string latest))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "channel-news, one entry"
   (with-temporary-git-repository directory
       `((add ".guix-channel"
@@ -406,7 +403,6 @@ (define channel-metadata-dependencies
                          (channel-news-for-commit channel commit5 commit1))
                     '(#f "tag-for-first-news-entry")))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "channel-news, annotated tag"
   (with-temporary-git-repository directory
       `((add ".guix-channel"
@@ -453,7 +449,6 @@ (define channel-metadata-dependencies
                          (channel-news-for-commit channel commit2))
                     (list commit1)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "latest-channel-instances, missing introduction for 'guix'"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
diff --git a/tests/derivations.scm b/tests/derivations.scm
index e1312bd46b..0e87778981 100644
--- a/tests/derivations.scm
+++ b/tests/derivations.scm
@@ -29,7 +29,7 @@ (define-module (test-derivations)
   #:use-module (guix tests git)
   #:use-module (guix tests http)
   #:use-module ((guix packages) #:select (package-derivation base32))
-  #:use-module ((guix build utils) #:select (executable-file? which))
+  #:use-module ((guix build utils) #:select (executable-file?))
   #:use-module ((guix hash) #:select (file-hash*))
   #:use-module ((git oid) #:select (oid->string))
   #:use-module ((git reference) #:select (reference-name->oid))
@@ -295,8 +295,6 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
                          get-string-all)
                        text))))))
 
-;; 'with-temporary-git-repository' relies on the 'git' command.
-(unless (which (git-command)) (test-skip 1))
 (test-equal "'git-download' built-in builder"
   `(("/a.txt" . "AAA")
     ("/b.scm" . "#t"))
@@ -325,7 +323,6 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
         (build-derivations %store (list drv))
         (directory-contents (derivation->output-path drv) get-string-all)))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "'git-download' built-in builder, invalid hash"
   (with-temporary-git-repository directory
       `((add "a.txt" "AAA")
@@ -349,7 +346,6 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
         (build-derivations %store (list drv))
         #f))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "'git-download' built-in builder, invalid commit"
   (with-temporary-git-repository directory
       `((add "a.txt" "AAA")
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index c063920c12..4de223d422 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -44,7 +44,6 @@ (define (gpg+git-available?)
 
 (test-begin "git-authenticate")
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "unsigned commits"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
diff --git a/tests/git.scm b/tests/git.scm
index 9c944d65b1..ad43435b67 100644
--- a/tests/git.scm
+++ b/tests/git.scm
@@ -21,7 +21,6 @@ (define-module (test-git)
   #:use-module (git)
   #:use-module (guix git)
   #:use-module (guix tests git)
-  #:use-module (guix build utils)
   #:use-module ((guix utils) #:select (call-with-temporary-directory))
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-64)
@@ -33,8 +32,6 @@ (define-module (test-git)
 
 (test-begin "git")
 
-;; 'with-temporary-git-repository' relies on the 'git' command.
-(unless (which (git-command)) (test-skip 1))
 (test-assert "commit-difference, linear history"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
@@ -61,7 +58,6 @@ (define-module (test-git)
              ;; empty list.
              (null? (commit-difference commit1 commit4)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "commit-difference, fork"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
@@ -101,7 +97,6 @@ (define-module (test-git)
              (lset= eq? (commit-difference master4 master2)
                     (list master4 merge master3 devel1 devel2)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "commit-difference, excluded commits"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
@@ -126,7 +121,6 @@ (define-module (test-git)
                     (list commit4))
              (null? (commit-difference commit4 commit1 (list commit5))))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "commit-relation"
   '(self                                          ;master3 master3
     ancestor                                      ;master1 master3
@@ -166,7 +160,6 @@ (define-module (test-git)
               (commit-relation master1 merge)
               (commit-relation merge master1))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "commit-descendant?"
   '((master3 master3 => #t)
     (master1 master3 => #f)
@@ -216,7 +209,6 @@ (define-module (test-git)
                   (master1 merge)
                   (merge master1)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "remote-refs"
   '("refs/heads/develop" "refs/heads/master"
     "refs/tags/v1.0" "refs/tags/v1.1")
@@ -231,7 +223,6 @@ (define-module (test-git)
         (tag "v1.1" "release-1.1"))
     (remote-refs directory)))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "remote-refs: only tags"
  '("refs/tags/v1.0" "refs/tags/v1.1")
   (with-temporary-git-repository directory
@@ -243,7 +234,6 @@ (define-module (test-git)
         (tag "v1.1" "Release 1.1"))
     (remote-refs directory #:tags? #t)))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "update-cached-checkout, tag"
   (call-with-temporary-directory
    (lambda (cache)
diff --git a/tests/import-git.scm b/tests/import-git.scm
index f1bce154bb..20255dedb3 100644
--- a/tests/import-git.scm
+++ b/tests/import-git.scm
@@ -24,7 +24,6 @@ (define-module (test-import-git)
   #:use-module (guix import git)
   #:use-module (guix git-download)
   #:use-module (guix tests git)
-  #:use-module (guix build utils)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-64))
 
@@ -46,7 +45,6 @@ (define* (make-package directory version #:optional (properties '()))
         (base32
          "0000000000000000000000000000000000000000000000000000"))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: no custom prefix, suffix, and delimiter"
   "1.0.1"
   (with-temporary-git-repository directory
@@ -56,7 +54,6 @@ (define* (make-package directory version #:optional (properties '()))
     (let ((package (make-package directory "1.0.0")))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom prefix, no suffix and delimiter"
   "1.0.1"
   (with-temporary-git-repository directory
@@ -67,7 +64,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-prefix . "prefix-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom suffix, no prefix and delimiter"
   "1.0.1"
   (with-temporary-git-repository directory
@@ -78,7 +74,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-suffix . "-suffix-[0-9]*")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom delimiter, no prefix and suffix"
   "2021.09.07"
   (with-temporary-git-repository directory
@@ -89,7 +84,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-version-delimiter . "-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: empty delimiter, no prefix and suffix"
   "20210907"
   (with-temporary-git-repository directory
@@ -100,7 +94,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-version-delimiter . "")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom prefix and suffix, no delimiter"
   "2.0.0"
   (with-temporary-git-repository directory
@@ -112,7 +105,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-suffix . "suffix-[0-9]")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom prefix, suffix, and delimiter"
   "2.0.0"
   (with-temporary-git-repository directory
@@ -125,7 +117,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "_")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: only pre-releases available"
   #f
   (with-temporary-git-repository directory
@@ -135,7 +126,6 @@ (define* (make-package directory version #:optional (properties '()))
     (let ((package (make-package directory "1.0.0")))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases"
   "2.0.0-rc1"
   (with-temporary-git-repository directory
@@ -146,7 +136,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((accept-pre-releases? . #t)))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom prefix"
   "2.0.0-rc1"
   (with-temporary-git-repository directory
@@ -158,7 +147,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-prefix . "version-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom suffix"
   "2.0.0-rc1"
   (with-temporary-git-repository directory
@@ -170,7 +158,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-suffix . "-suffix")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, delimiter conflicts with pre-release part"
   "2.0.0_alpha"
   (with-temporary-git-repository directory
@@ -182,7 +169,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "_")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom suffix and prefix"
   "2.0.0-alpha"
   (with-temporary-git-repository directory
@@ -195,7 +181,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-suffix . "-suffix")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom suffix, prefix, and delimiter"
   "2.0.0-alpha"
   (with-temporary-git-repository directory
@@ -209,7 +194,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, no delimiter, and custom suffix, prefix"
   "2alpha"
   (with-temporary-git-repository directory
@@ -223,7 +207,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: no tags found"
   #f
   (with-temporary-git-repository directory
@@ -232,7 +215,6 @@ (define* (make-package directory version #:optional (properties '()))
     (let ((package (make-package directory "1.0.0")))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: no valid tags found"
   #f
   (with-temporary-git-repository directory
-- 
2.41.0

Message #32 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Vivien Kraus <vivien <at> planete-kraus.eu>
To: Ludovic Courtès <ludo <at> gnu.org>, 63331 <at> debbugs.gnu.org
Cc: Simon Josefsson <simon <at> josefsson.org>, 65866 <at> debbugs.gnu.org
Subject: Re: bug#63331: Guile-GnuTLS/Git circular dependency and built-in
 git checkouts
Date: Mon, 11 Sep 2023 17:16:14 +0200

Hello!

Le lundi 11 septembre 2023 à 16:36 +0200, Ludovic Courtès a écrit :
> Eventually, when users are all running recent versions of
> ‘guix-daemon’ with support for “builtin:git-download” (2–4
> years from now?), we’ll be able to use “builtin:git-download”
> unconditionally and thus be sure there are no risks of
> derivation cycles.

Do foreign distros need to update their guix package as well? If that
is the case, the provided time frame might be optimistic.

> Note that the patch series adds a hard dependency on Git.
> This is because the existing ‘git-fetch’ code depends on Git

I applaud the switch to the regular git program from libgit2, as I
would then be able to pull from my cgit "dumb" server instead of having
to maintain a mirror.

Vivien

Message #35 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Vivien Kraus <vivien <at> planete-kraus.eu>
Cc: Simon Josefsson <simon <at> josefsson.org>, 65866 <at> debbugs.gnu.org,
 63331 <at> debbugs.gnu.org
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 11 Sep 2023 22:57:35 +0200

Hi,

Vivien Kraus <vivien <at> planete-kraus.eu> skribis:

> Le lundi 11 septembre 2023 à 16:36 +0200, Ludovic Courtès a écrit :
>> Eventually, when users are all running recent versions of
>> ‘guix-daemon’ with support for “builtin:git-download” (2–4
>> years from now?), we’ll be able to use “builtin:git-download”
>> unconditionally and thus be sure there are no risks of
>> derivation cycles.
>
> Do foreign distros need to update their guix package as well? If that
> is the case, the provided time frame might be optimistic.

At some point, we can change clients to print a warning saying that
their daemon is outdated if it lacks “builtin:git-download”.  That
should help speed things up.

>> Note that the patch series adds a hard dependency on Git.
>> This is because the existing ‘git-fetch’ code depends on Git
>
> I applaud the switch to the regular git program from libgit2, as I
> would then be able to pull from my cgit "dumb" server instead of having
> to maintain a mirror.

Nothing changes here: ‘git-fetch’ already uses Git.

Ludo’.

Message #38 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: hard dependency on Git? (was bug#65866: [PATCH 0/8] Add
 built-in builder for Git checkouts)
Date: Thu, 14 Sep 2023 18:51:42 +0200

Hi!

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> So given there's no technical reasons not to use libgit2, I'd use that
> and keep the closure size down.

For the record, that’s a 6% increase:

--8<---------------cut here---------------start------------->8---
$ guix size guix | tail -1
total: 633.0 MiB
$ guix size guix git-minimal | tail -1
total: 675.7 MiB
--8<---------------cut here---------------end--------------->8---

(Of course it all adds up; I’m not saying we can dismiss it.)

In the context of <https://issues.guix.gnu.org/65866> plus the lack of
GC in libgit2 discussed in <https://issues.guix.gnu.org/65720>, my
inclination is to include that hard dependency on Git.

That’s not a happy choice for me, but it has the advantage of solving
two immediate problems.

I would revisit it as soon as libgit2 supports shallow clones (which is
coming, as you write) and GC (or a workaround to that effect).  SHA256
may also soon be a requirement: we’ll need to be able to clone repos
that use it.

How does that sound?

Ludo’.

Message #41 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: hard dependency on Git? (was bug#65866: [PATCH 0/8] Add built-in
 builder for Git checkouts)
Date: Thu, 14 Sep 2023 19:28:06 +0200

Hi,

On Thu, 14 Sept 2023 at 18:51, Ludovic Courtès <ludo <at> gnu.org> wrote:

> For the record, that’s a 6% increase:
>
> --8<---------------cut here---------------start------------->8---
> $ guix size guix | tail -1
> total: 633.0 MiB
> $ guix size guix git-minimal | tail -1
> total: 675.7 MiB
> --8<---------------cut here---------------end--------------->8---
>
> (Of course it all adds up; I’m not saying we can dismiss it.)
>
> In the context of <https://issues.guix.gnu.org/65866> plus the lack of
> GC in libgit2 discussed in <https://issues.guix.gnu.org/65720>, my
> inclination is to include that hard dependency on Git.

And considering bug#65924 [1], it is not 6% but more.  Because
currently git-minimal is broken and coreutils and potentially
util-linux would also be part of the closure.

--8<---------------cut here---------------start------------->8---
$ guix size guix | tail -1
total: 633.0 MiB
$ guix size guix git-minimal coreutils | tail -1
total: 692.7 MiB
$ guix size guix git-minimal coreutils util-linux | tail -1
total: 706.6 MiB
--8<---------------cut here---------------end--------------->8---

Therefore. it is 9.4% or worse 11.6%.

1: bug#65924: git searches coreutils and util-linux commands in PATH
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Wed, 13 Sep 2023 14:00:09 -0400
id:87fs3iuf6e.fsf <at> gmail.com
https://yhetil.org/guix/87fs3iuf6e.fsf <at> gmail.com
https://issues.guix.gnu.org/msgid/87fs3iuf6e.fsf <at> gmail.com


> That’s not a happy choice for me, but it has the advantage of solving
> two immediate problems.

Three. :-)

Once git-minimal and plumbing Git commands around, some slow
procedures using libgit2 will be replaced by faster ones.  And I also
have in mind some Git repository normalization that differs from SWH;
having plain Git commands would also easy that part.

Just to point that the introduction of git-minimal as hard dependency
of Guix means that libgit2 will be slowly removed from the picture,
IMHO.

Cheers,
simon

Message #44 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: hard dependency on Git? (was bug#65866: [PATCH 0/8] Add
 built-in builder for Git checkouts)
Date: Sat, 16 Sep 2023 22:16:41 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> So given there's no technical reasons not to use libgit2, I'd use that
>> and keep the closure size down.
>
> For the record, that’s a 6% increase:
>
> $ guix size guix | tail -1
> total: 633.0 MiB
> $ guix size guix git-minimal | tail -1
> total: 675.7 MiB
>
> (Of course it all adds up; I’m not saying we can dismiss it.)

As Simon pointed out, it'd be more after wrapping 'git' with coreutils
and possible util-linux on its PATH.

> In the context of <https://issues.guix.gnu.org/65866> plus the lack of
> GC in libgit2 discussed in <https://issues.guix.gnu.org/65720>, my
> inclination is to include that hard dependency on Git.
>
> That’s not a happy choice for me, but it has the advantage of solving
> two immediate problems.
>
> I would revisit it as soon as libgit2 supports shallow clones (which is
> coming, as you write)

This isn't "coming", it's already been released :-).

> and GC (or a workaround to that effect).  SHA256 may also soon be a
> requirement: we’ll need to be able to clone repos that use it.
>
> How does that sound?

Yeah, 'git gc' is lacking from libgit2.  I'm not against adding
dependency on the real 'git' CLI, but at that point, as Simon mentioned,
I see little reason to keep libgit2 around for much longer, given it
performs worst than git CLI in every aspect.  I doubt forking processes
on GNU/Linux would cause a performance hit compared to using libgit2,
especially given how optimized git appears to be (at least compared to
libgit2).

So, I think we need to agree on the future of libgit2 in the big picture
and decide to invest in it or let it in favor of just using git.

-- 
Thanks,
Maxim

Message #47 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: Re: hard dependency on Git? (was bug#65866: [PATCH 0/8] Add
 built-in builder for Git checkouts)
Date: Mon, 18 Sep 2023 15:56:21 +0200

Hello!

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Yeah, 'git gc' is lacking from libgit2.  I'm not against adding
> dependency on the real 'git' CLI, but at that point, as Simon mentioned,
> I see little reason to keep libgit2 around for much longer, given it
> performs worst than git CLI in every aspect.  I doubt forking processes
> on GNU/Linux would cause a performance hit compared to using libgit2,
> especially given how optimized git appears to be (at least compared to
> libgit2).

As I wrote, as an example, I don’t think that there could be a practical
implementation of (guix git-authenticate) shelling out to ‘git’.

Anyhow, how about this plan:

  1. Merge <https://issues.guix.gnu.org/65866> with the hard Git
     dependency.

  2. When libgit2 1.7 with shallow clones is available in Guix, work on
     a patch to use Guile-Git for clones and evaluate it.

  3. Brainstorm on ways to address lack of GC support based on a closer
     analysis of disk usage for Guix’s cached checkouts.

Deal?

Ludo’.

PS: I don’t buy the “libgit2 will disappear from Guix” argument because
    it’s not a natural phenomenon that we’re observing but a willful
    construction.

Message #50 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: hard dependency on Git? (was bug#65866: [PATCH 0/8] Add built-in
 builder for Git checkouts)
Date: Mon, 18 Sep 2023 16:45:00 +0200

Hi Ludo,

On Mon, 18 Sept 2023 at 15:56, Ludovic Courtès <ludo <at> gnu.org> wrote:

> Anyhow, how about this plan:
>
>   1. Merge <https://issues.guix.gnu.org/65866> with the hard Git
>      dependency.

Is #65866 fixing bug#63331 (Guile-GnuTLS/Git circular dependency) [1]?

Does the merge of #65866 lead to close #63331?  Because you wrote,

    This patch series is a first step towards getting Git out of
    derivation graphs when it’s only used to fetch source code
    (origins with ‘git-fetch’), with the goal of fixing:

so...

>   2. When libgit2 1.7 with shallow clones is available in Guix, work on
>      a patch to use Guile-Git for clones and evaluate it.

...we could also suggest to continue and have a complete fix of #63331
before merging #65866.  It avoids to introduce a hard dependency which
will be difficult to remove and let the time for this evaluation of
libgit-2.1.7, no?

For what my opinion is worth, I have nothing for introducing a hard
dependency to Git but we have to be clear that 1. once introduced it
will hard to remove, 2. we will merge patches using faster Git
plumbing equivalent implementation and so 3. it will push out
Guile-Git.


> As I wrote, as an example, I don’t think that there could be a practical
> implementation of (guix git-authenticate) shelling out to ‘git’.

[...]

> PS: I don’t buy the “libgit2 will disappear from Guix” argument because
>     it’s not a natural phenomenon that we’re observing but a willful
>     construction.

As I wrote elsewhere, Git-Annex (or Magit) are shelling out to 'git',
IIRC.  Well, personally I do not consider that Git-Annex is slow or
that Git-Annex does not implement features as complex as (guix
git-authenticate).

After reading [2],

     I cannot imagine a viable implementation of things like ‘commit-closure’
     and ‘commit-relation’ from (guix git) done by shelling out to ‘git’.
     I’m quite confident this would be slow and brittle.

wolf came 3 days later [3] with a first rough implementation for
'commit-relation' using Git plumbing which is much more faster than
the one implemented with Guile-Git.  Even, speaking specifically about
'commit-relation', I challenge whoever to beat "git merge-base
--is-ancestor"; life is risky: I bet my round of beers in Brussels in
the next Guix Days. :-)  Reading the C implementation of
"merge-base.c" [4] and following the various procedures, it appears to
me impossible to beat it; bah using Guile-Git cumulates various
penalties from talking to libgit2 to the Garbage Collector of Guile.

The question does not appear to me if you buy it or not. :-)  The
question is instead: do we merge code that uses Git plumbing shelling
out that is faster than the current implementation using Guile-Git?

Cheers,
simon

1: https://issues.guix.gnu.org/issue/63331

2: bug#65720: Guile-Git-managed checkouts grow way too much
Ludovic Courtès <ludo <at> gnu.org>
Fri, 08 Sep 2023 19:08:05 +0200
id:87pm2s385m.fsf <at> gnu.org
https://issues.guix.gnu.org//65720
https://issues.guix.gnu.org/msgid/87pm2s385m.fsf <at> gnu.org
https://yhetil.org/guix/87pm2s385m.fsf <at> gnu.org

3: bug#65720: Guile-Git-managed checkouts grow way too much
wolf <wolf <at> wolfsden.cz>
Mon, 11 Sep 2023 16:42:59 +0200
id:ZP8nc1m8rN_34XV-@ws
https://issues.guix.gnu.org//65720
https://issues.guix.gnu.org/msgid/ZP8nc1m8rN_34XV-@ws
https://yhetil.org/guix/ZP8nc1m8rN_34XV-@ws

4: https://github.com/git/git/blob/bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518/builtin/merge-base.c#L103-L115

Message #53 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Tue, 19 Sep 2023 16:43:34 +0200

Hi Simon,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Mon, 18 Sept 2023 at 15:56, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> Anyhow, how about this plan:
>>
>>   1. Merge <https://issues.guix.gnu.org/65866> with the hard Git
>>      dependency.
>
> Is #65866 fixing bug#63331 (Guile-GnuTLS/Git circular dependency) [1]?

Yes, as written in the cover letter.

[...]

>>   2. When libgit2 1.7 with shallow clones is available in Guix, work on
>>      a patch to use Guile-Git for clones and evaluate it.
>
> ...we could also suggest to continue and have a complete fix of #63331
> before merging #65866.

Sorry, I don’t understand.  As I wrote in the cover letter, this patch
series is the complete fix for <https://issues.guix.gnu.org/63331>.

> It avoids to introduce a hard dependency which will be difficult to
> remove and let the time for this evaluation of libgit-2.1.7, no?

What this patch series sets in stone is “builtin:git-download” and its
semantics.

Its implementation can change over time though: it can switch to
libgit2, to OCaml-Git, or anything that pleases us.  These are
implementation details not visible from the outside.

>> As I wrote, as an example, I don’t think that there could be a practical
>> implementation of (guix git-authenticate) shelling out to ‘git’.
>
> [...]
>
>> PS: I don’t buy the “libgit2 will disappear from Guix” argument because
>>     it’s not a natural phenomenon that we’re observing but a willful
>>     construction.
>
> As I wrote elsewhere, Git-Annex (or Magit) are shelling out to 'git',
> IIRC.  Well, personally I do not consider that Git-Annex is slow or
> that Git-Annex does not implement features as complex as (guix
> git-authenticate).
>
> After reading [2],
>
>      I cannot imagine a viable implementation of things like ‘commit-closure’
>      and ‘commit-relation’ from (guix git) done by shelling out to ‘git’.
>      I’m quite confident this would be slow and brittle.
>
> wolf came 3 days later [3] with a first rough implementation for
> 'commit-relation' using Git plumbing which is much more faster than
> the one implemented with Guile-Git.

Yes, point taken.  It’s not so much about whether Git-Annex is “less
complex”, it’s about the level of integration needed.  But you don’t
have to take my word for it.

We’ve spent lots of words on the issue of a dependency on Git, and yet
this patch series doesn’t actually change much in that regard:
‘git-fetch’ already uses Git.

I suggest that we focus on the various sub-problems we’re trying to
solve without losing sight of the big picture, yet without conflating
them all.

Ludo’.

Message #56 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: guix-devel <at> gnu.org, 65866 <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Tue, 19 Sep 2023 19:09:22 +0200

Hi Ludo,

On Tue, 19 Sep 2023 at 16:43, Ludovic Courtès <ludo <at> gnu.org> wrote:

>>>   1. Merge <https://issues.guix.gnu.org/65866> with the hard Git
>>>      dependency.
>>
>> Is #65866 fixing bug#63331 (Guile-GnuTLS/Git circular dependency) [1]?
>
> Yes, as written in the cover letter.

[...]

>                            As I wrote in the cover letter, this patch
> series is the complete fix for <https://issues.guix.gnu.org/63331>.

Thanks for clarifying the cover letter:

        This patch series is a first step towards getting Git out of
        derivation graphs when it’s only used to fetch source code
        (origins with ‘git-fetch’), with the goal of fixing:

          https://issues.guix.gnu.org/63331

Because I am not native, my dictionary says, Goal: Something that is
your goal is something that you hope to achieve, especially when much
time and effort will be needed.

Sorry if, from the cover letter and my vague understanding of the code,
it was not obvious for me that merging #65866 directly close #63331.
From my understanding, #65866 was one step toward closing #63331 and not
the complete fix.

Anyway. :-)


> I suggest that we focus on the various sub-problems we’re trying to
> solve without losing sight of the big picture, yet without conflating
> them all.

The way we are trying to focus or solve these various sub-problems
depends on what we have at hand (the big picture).  Having an hard
dependency of Git means these immediate improvements by drop-in
replacements:

 + git clone => 3x faster for full Guix repository
 + shallow clone => 25% of improvements
 + git fetch => no worry much about gc
 + commit-relation => 35x faster

for an increase of the closure between 9% and 12%.  All these numbers
are for my machine and I guess they would be the order on average.

That said, I expressed my concerns about the “big picture” and
libgit2. :-)

Cheers,
simon

Message #59 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 12:05:23 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> * guix/build/git.scm (git-fetch-with-fallback): New procedure, with code
> taken from…
> * guix/git-download.scm (git-fetch): … here.
> [modules]: Remove modules that are no longer directly used in ‘build’.
> [build]: Use ‘git-fetch-with-fallback’.

[...]

> +
> +(define* (git-fetch-with-fallback url commit directory
> +                                  #:key (git-command "git") recursive?)
> +  "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
> +alternative methods when fetching from URL fails: attempt to download a nar,
> +and if that also fails, download from the Software Heritage archive."
> +  (or (git-fetch url commit directory
> +                 #:recursive? recursive?
> +                 #:git-command git-command)
> +      (download-nar directory)
> +
> +      ;; As a last resort, attempt to download from Software Heritage.
> +      ;; Disable X.509 certificate verification to avoid depending
> +      ;; on nss-certs--we're authenticating the checkout anyway.
> +      ;; XXX: Currently recursive checkouts are not supported.
> +      (and (not recursive?)

I know this is code moved from elsewhere, but it seems it'd be useful to
fail hard here with a proper error instead of returning #f silently?  Or
add support for recursive clones; was is missing to enable that?  It's
at least easy from the git CLI.

> +           (parameterize ((%verify-swh-certificate? #f))
> +             (format (current-error-port)
> +                     "Trying to download from Software Heritage...~%")
> +
> +             (swh-download url commit directory)
> +             (when (file-exists?
> +                    (string-append directory "/.gitattributes"))
> +               ;; Perform CR/LF conversion and other changes
> +               ;; specificied by '.gitattributes'.
> +               (invoke git-command "-C" directory "init")
> +               (invoke git-command "-C" directory "config" "--local"
> +                       "user.email" "you <at> example.org")
> +               (invoke git-command "-C" directory "config" "--local"
> +                       "user.name" "Your Name")
> +               (invoke git-command "-C" directory "add" ".")
> +               (invoke git-command "-C" directory "commit" "-am" "init")
> +               (invoke git-command "-C" directory "read-tree" "--empty")
> +               (invoke git-command "-C" directory "reset" "--hard")
> +               (delete-file-recursively
> +                (string-append directory "/.git")))))))

I'm not familiar with this code, but was wondering why we need to do
this post processing and handle .gitattributes.  I never care about this
on my GNU/Linux machine when using 'git clone'.  Perhaps 'git fetch' is
used directly, which is why?

Time passes...  Ah!  I misread -- that's peculiar to Software Heritage.

>  ;;; git.scm ends here
> diff --git a/guix/git-download.scm b/guix/git-download.scm
> index d88f4c40ee..8989b1b463 100644
> --- a/guix/git-download.scm
> +++ b/guix/git-download.scm
> @@ -116,19 +116,16 @@ (define* (git-fetch ref hash-algo hash
>    (define modules
>      (delete '(guix config)
>              (source-module-closure '((guix build git)
> -                                     (guix build utils)
> -                                     (guix build download-nar)
> -                                     (guix swh)))))
> +                                     (guix build utils)))))
>  
>    (define build
>      (with-imported-modules modules
> -      (with-extensions (list guile-json gnutls   ;for (guix swh)
> +      (with-extensions (list guile-json gnutls    ;for (guix swh)
>                               guile-lzlib)
>          #~(begin
>              (use-modules (guix build git)
> -                         (guix build utils)
> -                         (guix build download-nar)
> -                         (guix swh)
> +                         ((guix build utils)
> +                          #:select (set-path-environment-variable))
>                           (ice-9 match))
>  
>              (define recursive?
> @@ -151,38 +148,10 @@ (define* (git-fetch ref hash-algo hash
>              (setvbuf (current-output-port) 'line)
>              (setvbuf (current-error-port) 'line)
>  
> -            (or (git-fetch (getenv "git url") (getenv "git commit")
> -                           #$output
> -                           #:recursive? recursive?
> -                           #:git-command "git")
> -                (download-nar #$output)
> -
> -                ;; As a last resort, attempt to download from Software Heritage.
> -                ;; Disable X.509 certificate verification to avoid depending
> -                ;; on nss-certs--we're authenticating the checkout anyway.
> -                ;; XXX: Currently recursive checkouts are not supported.
> -                (and (not recursive?)
> -                     (parameterize ((%verify-swh-certificate? #f))
> -                       (format (current-error-port)
> -                               "Trying to download from Software Heritage...~%")
> -
> -                       (swh-download (getenv "git url") (getenv "git commit")
> -                                     #$output)
> -                       (when (file-exists?
> -                              (string-append #$output "/.gitattributes"))
> -                         ;; Perform CR/LF conversion and other changes
> -                         ;; specificied by '.gitattributes'.
> -                         (invoke "git" "-C" #$output "init")
> -                         (invoke "git" "-C" #$output "config" "--local"
> -                                 "user.email" "you <at> example.org")
> -                         (invoke "git" "-C" #$output "config" "--local"
> -                                 "user.name" "Your Name")
> -                         (invoke "git" "-C" #$output "add" ".")
> -                         (invoke "git" "-C" #$output "commit" "-am" "init")
> -                         (invoke "git" "-C" #$output "read-tree" "--empty")
> -                         (invoke "git" "-C" #$output "reset" "--hard")
> -                         (delete-file-recursively
> -                          (string-append #$output "/.git"))))))))))
> +            (git-fetch-with-fallback (getenv "git url") (getenv "git commit")
> +                                     #$output
> +                                     #:recursive? recursive?
> +                                     #:git-command "git")))))
>  
>    (mlet %store-monad ((guile (package->derivation guile system)))
>      (gexp->derivation (or name "git-checkout") build

LGTM.

-- 
Thanks,
Maxim

Message #62 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 12:07:39 -0400

Hi,

Ludovic Courtès <ludo <at> gnu.org> writes:

> * guix/git-download.scm (git-fetch): Honor ‘%download-fallback-test’.

LGTM!

-- 
Thanks,
Maxim

Message #65 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 12:09:15 -0400

Hi,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Code in ‘builtins.cc’ only ever invokes ‘guix perform-download’ with two
> arguments.
>
> * guix/scripts/perform-download.scm (guix-perform-download): Remove
> unused one-argument clause.

LGTM!

-- 
Thanks,
Maxim

Message #68 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 18:40:41 +0200

Hi Maxim,

On Wed, 20 Sept 2023 at 18:05, Maxim Cournoyer
<maxim.cournoyer <at> gmail.com> wrote:

> > +           (parameterize ((%verify-swh-certificate? #f))
> > +             (format (current-error-port)
> > +                     "Trying to download from Software Heritage...~%")
> > +
> > +             (swh-download url commit directory)
> > +             (when (file-exists?
> > +                    (string-append directory "/.gitattributes"))
> > +               ;; Perform CR/LF conversion and other changes
> > +               ;; specificied by '.gitattributes'.
> > +               (invoke git-command "-C" directory "init")
> > +               (invoke git-command "-C" directory "config" "--local"
> > +                       "user.email" "you <at> example.org")
> > +               (invoke git-command "-C" directory "config" "--local"
> > +                       "user.name" "Your Name")
> > +               (invoke git-command "-C" directory "add" ".")
> > +               (invoke git-command "-C" directory "commit" "-am" "init")
> > +               (invoke git-command "-C" directory "read-tree" "--empty")
> > +               (invoke git-command "-C" directory "reset" "--hard")
> > +               (delete-file-recursively
> > +                (string-append directory "/.git")))))))
>
> I'm not familiar with this code, but was wondering why we need to do
> this post processing and handle .gitattributes.  I never care about this
> on my GNU/Linux machine when using 'git clone'.  Perhaps 'git fetch' is
> used directly, which is why?
>
> Time passes...  Ah!  I misread -- that's peculiar to Software Heritage.

We need to post-process .gitattributes because it depends on how the
remote host serves the files.  And yeah it mainly comes from SWH. :-)
They store the files with an uniform normalization and so without
applying .gitattributes, we do not necessary get the correct checksum.

To my knowledge, we cannot do better than these sequential Git commands.

Cheers,
simon

Message #71 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Ludovic Courtès <ludovic.courtes <at> inria.fr>,
 Ricardo Wurmus <rekado <at> elephly.net>, 65866 <at> debbugs.gnu.org,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 13:32:37 -0400

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> From: Ludovic Courtès <ludovic.courtes <at> inria.fr>
>
> The new builder makes it possible to break cycles that occurs when the
> fixed-output derivation for the source of a dependency of ‘git’ would
> itself depend on ‘git’.
>
> * guix/scripts/perform-download.scm (perform-git-download): New
> procedure.
> (perform-download): Move fixed-output derivation check to…
> (guix-perform-download): … here.  Invoke ‘perform-download’ or
> ‘perform-git-download’ depending on what ‘derivation-builder’ returns.
> * nix/libstore/builtins.cc (builtins): Add “git-download”.
> * tests/derivations.scm ("built-in-builders"): Update.
> ("'git-download' built-in builder")
> ("'git-download' built-in builder, invalid hash")
> ("'git-download' built-in builder, invalid commit")
> ("'git-download' built-in builder, not found"): New tests.
> ---
>  guix/scripts/perform-download.scm |  52 +++++++++++++---
>  nix/libstore/builtins.cc          |   5 +-
>  tests/derivations.scm             | 100 +++++++++++++++++++++++++++++-
>  3 files changed, 145 insertions(+), 12 deletions(-)
>
> diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
> index c8f044e82e..a287e97528 100644
> --- a/guix/scripts/perform-download.scm
> +++ b/guix/scripts/perform-download.scm
> @@ -1,5 +1,5 @@
>  ;;; GNU Guix --- Functional package management for GNU
> -;;; Copyright © 2016, 2017, 2018, 2020 Ludovic Courtès <ludo <at> gnu.org>
> +;;; Copyright © 2016-2018, 2020, 2023 Ludovic Courtès <ludo <at> gnu.org>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -21,7 +21,8 @@ (define-module (guix scripts perform-download)
>    #:use-module (guix scripts)
>    #:use-module (guix derivations)
>    #:use-module ((guix store) #:select (derivation-path? store-path?))
> -  #:use-module (guix build download)
> +  #:autoload   (guix build download) (url-fetch)
> +  #:autoload   (guix build git) (git-fetch-with-fallback)
>    #:use-module (ice-9 match)
>    #:export (guix-perform-download))
>  
> @@ -64,10 +65,6 @@ (define* (perform-download drv #:optional output
>             (drv-output (assoc-ref (derivation-outputs drv) "out"))
>             (algo       (derivation-output-hash-algo drv-output))
>             (hash       (derivation-output-hash drv-output)))
> -      (unless (and algo hash)
> -        (leave (G_ "~a is not a fixed-output derivation~%")
> -               (derivation-file-name drv)))
> -
>        ;; We're invoked by the daemon, which gives us write access to OUTPUT.
>        (when (url-fetch url output
>                         #:print-build-trace? print-build-trace?
> @@ -92,6 +89,33 @@ (define* (perform-download drv #:optional output
>          (when (and executable (string=? executable "1"))
>            (chmod output #o755))))))
>  
> +(define* (perform-git-download drv #:optional output
> +                               #:key print-build-trace?)
> +  "Perform the download described by DRV, a fixed-output derivation, to
> +OUTPUT.
> +
> +Note: Unless OUTPUT is #f, we don't read the value of 'out' in DRV since the
> +actual output is different from that when we're doing a 'bmCheck' or

I'd drop the 'we's and use impersonal imperative tense or at least
's/when we're doing/when doing/'.

> +'bmRepair' build."
> +  (derivation-let drv ((output* "out")

I'd name this variable just 'out', for consistency with the others.

> +                       (url "url")
> +                       (commit "commit")
> +                       (recursive? "recursive?"))
> +    (unless url
> +      (leave (G_ "~a: missing Git URL~%") (derivation-file-name drv)))
> +    (unless commit
> +      (leave (G_ "~a: missing Git commit~%") (derivation-file-name drv)))
> +
> +    (let* ((output     (or output output*))
> 
> +           (url        (call-with-input-string url read))
> +           (recursive? (and recursive?
> +                            (call-with-input-string recursive? read)))
> +           (drv-output (assoc-ref (derivation-outputs drv) "out"))
> +           (algo       (derivation-output-hash-algo drv-output))
> +           (hash       (derivation-output-hash drv-output)))
> +      (git-fetch-with-fallback url commit output
> +                               #:recursive? recursive?))))
> +
>  (define (assert-low-privileges)
>    (when (zero? (getuid))
>      (leave (G_ "refusing to run with elevated privileges (UID ~a)~%")
> @@ -120,8 +144,20 @@ (define-command (guix-perform-download . args)
>      (match args
>        (((? derivation-path? drv) (? store-path? output))
>         (assert-low-privileges)
> -       (let ((drv (read-derivation-from-file drv)))
> -         (perform-download drv output #:print-build-trace? print-build-trace?)))
> +       (let* ((drv (read-derivation-from-file drv))
> +              (download (match (derivation-builder drv)
> +                          ("builtin:download" perform-download)
> +                          ("builtin:git-download" perform-git-download)
> +                          (unknown (leave (G_ "~a: unknown builtin builder")
> +                                          unknown))))
> +              (drv-output (assoc-ref (derivation-outputs drv) "out"))
> +              (algo       (derivation-output-hash-algo drv-output))
> +              (hash       (derivation-output-hash drv-output)))
> +         (unless (and hash algo)
> +           (leave (G_ "~a is not a fixed-output derivation~%")
> +                  (derivation-file-name drv)))
> +
> +         (download drv output #:print-build-trace? print-build-trace?)))
>        (("--version")
>         (show-version-and-exit))
>        (x
> diff --git a/nix/libstore/builtins.cc b/nix/libstore/builtins.cc
> index 4111ac4760..6bf467354a 100644
> --- a/nix/libstore/builtins.cc
> +++ b/nix/libstore/builtins.cc
> @@ -1,5 +1,5 @@
>  /* GNU Guix --- Functional package management for GNU
> -   Copyright (C) 2016, 2017, 2018, 2019 Ludovic Courtès <ludo <at> gnu.org>
> +   Copyright (C) 2016-2019, 2023 Ludovic Courtès <ludo <at> gnu.org>
>  
>     This file is part of GNU Guix.
>  
> @@ -58,7 +58,8 @@ static void builtinDownload(const Derivation &drv,
>  
>  static const std::map<std::string, derivationBuilder> builtins =
>  {
> -    { "download", builtinDownload }
> +    { "download", builtinDownload },
> +    { "git-download", builtinDownload }
>  };
>  
>  derivationBuilder lookupBuiltinBuilder(const std::string & name)
> diff --git a/tests/derivations.scm b/tests/derivations.scm
> index 66c777cfe7..e1312bd46b 100644
> --- a/tests/derivations.scm
> +++ b/tests/derivations.scm
> @@ -24,10 +24,15 @@ (define-module (test-derivations)
>    #:use-module (guix utils)
>    #:use-module ((gcrypt hash) #:prefix gcrypt:)
>    #:use-module (guix base32)
> +  #:use-module ((guix git) #:select (with-repository))
>    #:use-module (guix tests)
> +  #:use-module (guix tests git)
>    #:use-module (guix tests http)
>    #:use-module ((guix packages) #:select (package-derivation base32))
> -  #:use-module ((guix build utils) #:select (executable-file?))
> +  #:use-module ((guix build utils) #:select (executable-file? which))
> +  #:use-module ((guix hash) #:select (file-hash*))
> +  #:use-module ((git oid) #:select (oid->string))
> +  #:use-module ((git reference) #:select (reference-name->oid))
>    #:use-module (gnu packages bootstrap)
>    #:use-module ((gnu packages guile) #:select (guile-1.8))
>    #:use-module (srfi srfi-1)
> @@ -195,7 +200,7 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
>                     (stat:ino (lstat file2))))))))
>  
>  (test-equal "built-in-builders"
> -  '("download")
> +  '("download" "git-download")
>    (built-in-builders %store))
>  
>  (test-assert "unknown built-in builder"
> @@ -290,6 +295,97 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
>                           get-string-all)
>                         text))))))
>  
> +;; 'with-temporary-git-repository' relies on the 'git' command.
> +(unless (which (git-command)) (test-skip 1))

I'd expect the 'git' command to now be required by Autoconf at build
time, which should mean checking it here is not useful/required?

> +(test-equal "'git-download' built-in builder"
> +  `(("/a.txt" . "AAA")
> +    ("/b.scm" . "#t"))
> +  (let ((nonce (random-text)))
> +    (with-temporary-git-repository directory
> +        `((add "a.txt" "AAA")
> +          (add "b.scm" "#t")
> +          (commit ,nonce))
> +      (let* ((commit (with-repository directory repository
> +                       (oid->string
> +                        (reference-name->oid repository "HEAD"))))
> +             (drv (derivation %store "git-download"
> +                              "builtin:git-download" '()
> +                              #:env-vars
> +                              `(("url"
> +                                 . ,(object->string
> +                                     (string-append "file://" directory)))
> +                                ("commit" . ,commit))
> +                              #:hash-algo 'sha256
> +                              #:hash (file-hash* directory
> +                                                 #:algorithm
> +                                                 (gcrypt:hash-algorithm
> +                                                  gcrypt:sha256)
> +                                                 #:recursive? #t)
> +                              #:recursive? #t)))
> +        (build-derivations %store (list drv))
> +        (directory-contents (derivation->output-path drv) get-string-all)))))
> +
> +(unless (which (git-command)) (test-skip 1))
> +(test-assert "'git-download' built-in builder, invalid hash"
> +  (with-temporary-git-repository directory
> +      `((add "a.txt" "AAA")
> +        (add "b.scm" "#t")
> +        (commit "Commit!"))
> +    (let* ((commit (with-repository directory repository
> +                     (oid->string
> +                      (reference-name->oid repository "HEAD"))))
> +           (drv (derivation %store "git-download"
> +                            "builtin:git-download" '()
> +                            #:env-vars
> +                            `(("url"
> +                               . ,(object->string
> +                                   (string-append "file://" directory)))
> +                              ("commit" . ,commit))
> +                            #:hash-algo 'sha256
> +                            #:hash (gcrypt:sha256 #vu8())
> +                            #:recursive? #t)))
> +      (guard (c ((store-protocol-error? c)
> +                 (string-contains (store-protocol-error-message c) "failed")))
> +        (build-derivations %store (list drv))
> +        #f))))
> +
> +(unless (which (git-command)) (test-skip 1))
> +(test-assert "'git-download' built-in builder, invalid commit"
> +  (with-temporary-git-repository directory
> +      `((add "a.txt" "AAA")
> +        (add "b.scm" "#t")
> +        (commit "Commit!"))
> +    (let* ((drv (derivation %store "git-download"
> +                            "builtin:git-download" '()
> +                            #:env-vars
> +                            `(("url"
> +                               . ,(object->string
> +                                   (string-append "file://" directory)))
> +                              ("commit"
> +                               . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
> +                            #:hash-algo 'sha256
> +                            #:hash (gcrypt:sha256 #vu8())
> +                            #:recursive? #t)))
> +      (guard (c ((store-protocol-error? c)
> +                 (string-contains (store-protocol-error-message c) "failed")))
> +        (build-derivations %store (list drv))
> +        #f))))
> +
> +(test-assert "'git-download' built-in builder, not found"
> +  (let* ((drv (derivation %store "git-download"
> +                          "builtin:git-download" '()
> +                          #:env-vars
> +                          `(("url" . "file:///does-not-exist.git")
> +                            ("commit"
> +                             . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
> +                          #:hash-algo 'sha256
> +                          #:hash (gcrypt:sha256 #vu8())
> +                          #:recursive? #t)))
> +    (guard (c ((store-protocol-error? c)
> +               (string-contains (store-protocol-error-message c) "failed")))
> +      (build-derivations %store (list drv))
> +      #f)))
> +

Maybe the error message compared could be more precised, if it already
contains the necessary details?

Otherwise, well done!  LGTM with my above comments.

-- 
Thanks,
Maxim

Message #74 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 13:34:00 -0400

Ludovic Courtès <ludo <at> gnu.org> writes:

> * guix/scripts/perform-download.scm (perform-git-download): Pass #:git-command
> to ‘git-fetch-with-fallback’.
> ---
>  guix/scripts/perform-download.scm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

LGTM!

-- 
Thanks,
Maxim

Message #77 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 13:50:09 -0400

Hello!

Ludovic Courtès <ludo <at> gnu.org> writes:

> Fixes <https://issues.guix.gnu.org/63331>.
>
> Longer-term this will remove Git from the derivation graph when its sole
> use is to perform a checkout for a fixed-output derivation, thereby
> breaking dependency cycles that can arise in these situations.
>
> * guix/git-download.scm (git-fetch): Rename to…
> (git-fetch/in-band): … this.  Deal with GIT or GUILE being #f.

Nitpick, but I find this usage of dynamic default argument on top of
default arguments inelegant; see my comments below for an
alternative.

> (git-fetch/built-in, built-in-builders*, git-fetch): New procedures.
> * tests/builders.scm ("git-fetch, file URI"): New test.
> ---
>  guix/git-download.scm | 68 +++++++++++++++++++++++++++++++++++++------
>  tests/builders.scm    | 29 +++++++++++++++++-
>  2 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/guix/git-download.scm b/guix/git-download.scm
> index f1f19397c6..505dff0a89 100644
> --- a/guix/git-download.scm
> +++ b/guix/git-download.scm
> @@ -27,6 +27,7 @@ (define-module (guix git-download)
>    #:use-module (guix records)
>    #:use-module (guix packages)
>    #:use-module (guix modules)
> +  #:use-module ((guix derivations) #:select (raw-derivation))
>    #:autoload   (guix build-system gnu) (standard-packages)
>    #:autoload   (guix download) (%download-fallback-test)
>    #:autoload   (git bindings)   (libgit2-init!)
> @@ -78,15 +79,19 @@ (define (git-package)
>    (let ((distro (resolve-interface '(gnu packages version-control))))
>      (module-ref distro 'git-minimal)))
>  
> -(define* (git-fetch ref hash-algo hash
> -                    #:optional name
> -                    #:key (system (%current-system)) (guile (default-guile))
> -                    (git (git-package)))
> -  "Return a fixed-output derivation that fetches REF, a <git-reference>
> -object.  The output is expected to have recursive hash HASH of type
> -HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
> +(define* (git-fetch/in-band ref hash-algo hash
> +                            #:optional name
> +                            #:key (system (%current-system))
> +                            (guile (default-guile))
> +                            (git (git-package)))
> +  "Return a fixed-output derivation that performs a Git checkout of REF, using
> +GIT and GUILE (thus, said derivation depends on GIT and GUILE).
> +
> +This method is deprecated in favor of the \"builtin:git-download\" builder.
> +It will be removed when versions of guix-daemon implementing
> +\"builtin:git-download\" will be sufficiently widespread."
>    (define inputs
> -    `(("git" ,git)
> +    `(("git" ,(or git (git-package)))

Instead of using 'or' here to ensure git has a value, the default values
should have been copied to the new definition of git-fetch.

>  
>        ;; When doing 'git clone --recursive', we need sed, grep, etc. to be
>        ;; available so that 'git submodule' works.
> @@ -154,7 +159,8 @@ (define* (git-fetch ref hash-algo hash
>                                       #:recursive? recursive?
>                                       #:git-command "git")))))
>  
> -  (mlet %store-monad ((guile (package->derivation guile system)))
> +  (mlet %store-monad ((guile (package->derivation (or guile (default-guile))
> +                                                  system)))
>      (gexp->derivation (or name "git-checkout") build
>  
>                        ;; Use environment variables and a fixed script name so
> @@ -181,6 +187,50 @@ (define* (git-fetch ref hash-algo hash
>                        #:recursive? #t
>                        #:guile-for-build guile)))
>  
> +(define* (git-fetch/built-in ref hash-algo hash
> +                             #:optional name
> +                             #:key (system (%current-system)))
> +  "Return a fixed-output derivation without any dependency that performs a Git
> +checkout of REF, using the \"builtin:git-download\" derivation builder."
> +  (raw-derivation (or name "git-checkout") "builtin:git-download" '()
> +                  #:system system
> +                  #:hash-algo hash-algo
> +                  #:hash hash
> +                  #:recursive? #t
> +                  #:env-vars
> +                  `(("url" . ,(object->string
> +                               (match (%download-fallback-test)
> +                                 ('content-addressed-mirrors
> +                                  "https://example.org/does-not-exist")
> +                                 (_
> +                                  (git-reference-url ref)))))
> +                    ("commit" . ,(git-reference-commit ref))
> +                    ("recursive?" . ,(object->string
> +                                      (git-reference-recursive? ref))))
> +                  #:leaked-env-vars '("http_proxy" "https_proxy"
> +                                      "LC_ALL" "LC_MESSAGES" "LANG"
> +                                      "COLUMNS")
> +                  #:local-build? #t))
> +
> +(define built-in-builders*
> +  (store-lift built-in-builders))
> +
> +(define* (git-fetch ref hash-algo hash
> +                    #:optional name
> +                    #:key (system (%current-system))
> +                    guile git)

As mentioned above, I'd have kept the default values for guile and git
here.

> +  "Return a fixed-output derivation that fetches REF, a <git-reference>
> +object.  The output is expected to have recursive hash HASH of type
> +HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
> +  (mlet %store-monad ((builtins (built-in-builders*)))
> +    (if (member "git-download" builtins)
> +        (git-fetch/built-in ref hash-algo hash name
> +                            #:system system)
> +        (git-fetch/in-band ref hash-algo hash name
> +                           #:system system
> +                           #:guile guile
> +                           #:git git))))
> +
>  (define (git-version version revision commit)
>    "Return the version string for packages using git-download."
>    ;; git-version is almost exclusively executed while modules are being loaded.
> diff --git a/tests/builders.scm b/tests/builders.scm
> index 0b5577c7a3..619caa5f31 100644
> --- a/tests/builders.scm
> +++ b/tests/builders.scm
> @@ -1,5 +1,5 @@
>  ;;; GNU Guix --- Functional package management for GNU
> -;;; Copyright © 2012, 2013, 2014, 2015, 2018, 2019, 2021 Ludovic Courtès <ludo <at> gnu.org>
> +;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo <at> gnu.org>
>  ;;; Copyright © 2021 Lars-Dominik Braun <lars <at> 6xq.net>
>  ;;;
>  ;;; This file is part of GNU Guix.
> @@ -20,6 +20,7 @@
>  
>  (define-module (tests builders)
>    #:use-module (guix download)
> +  #:use-module (guix git-download)
>    #:use-module (guix build-system)
>    #:use-module (guix build-system gnu)
>    #:use-module (guix build gnu-build-system)
> @@ -31,9 +32,12 @@ (define-module (tests builders)
>    #:use-module (guix base32)
>    #:use-module (guix derivations)
>    #:use-module (gcrypt hash)
> +  #:use-module ((guix hash) #:select (file-hash*))
>    #:use-module (guix tests)
> +  #:use-module (guix tests git)
>    #:use-module (guix packages)
>    #:use-module (gnu packages bootstrap)
> +  #:use-module ((ice-9 ftw) #:select (scandir))
>    #:use-module (ice-9 match)
>    #:use-module (ice-9 textual-ports)
>    #:use-module (srfi srfi-1)
> @@ -84,6 +88,29 @@ (define url-fetch*
>      (and (file-exists? out)
>           (valid-path? %store out))))
>  
> +(test-equal "git-fetch, file URI"
> +  '("." ".." "a.txt" "b.scm")
> +  (let ((nonce (random-text)))
> +    (with-temporary-git-repository directory
> +        `((add "a.txt" ,nonce)
> +          (add "b.scm" "#t")
> +          (commit "Commit.")
> +          (tag "v1.0.0" "The tag."))
> +      (run-with-store %store
> +        (mlet* %store-monad ((hash
> +                              -> (file-hash* directory
> +                                             #:algorithm (hash-algorithm sha256)
> +                                             #:recursive? #t))
> +                             (drv (git-fetch
> +                                   (git-reference
> +                                    (url (string-append "file://" directory))
> +                                    (commit "v1.0.0"))
> +                                   'sha256 hash
> +                                   "git-fetch-test")))
> +          (mbegin %store-monad
> +            (built-derivations (list drv))
> +            (return (scandir (derivation->output-path drv)))))))))
> +
>  (test-assert "gnu-build-system"
>    (build-system? gnu-build-system))

Pretty neat test!  LGTM.  You can add a 'Reviewed-by:' git trailer in
Magit easily with 'C-u C-c C-r' :-)

-- 
Thanks,
Maxim

Message #80 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 13:57:45 -0400

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> * configure.ac: Check for ‘git’ and substitute ‘GIT’.
> * guix/config.scm.in (%git): New variable.
> * guix/self.scm (compiled-guix): Define ‘git’ and pass it to
> ‘make-config.scm’.
> (make-config.scm): Add #:git; emit a ‘%git’ variable.
> * doc/guix.texi (Requirements): Add it.

I'm a bit confused; we *both* capture git from the build environment,
and reference git from the git-minimal Guix package -- why can't we
strictly rely on the captured Git from the environment?

Nitpick: this commit should be ordered before the daemon changes that
requires it.

> ---
>  configure.ac       |  7 +++++++
>  doc/guix.texi      |  1 +
>  guix/config.scm.in |  6 +++++-
>  guix/self.scm      | 10 +++++++++-
>  4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 92dede8014..d817f620cf 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -201,6 +201,13 @@ AC_SUBST([GZIP])
>  AC_SUBST([BZIP2])
>  AC_SUBST([XZ])
>  
> +dnl Git is now required for the "builtin:git-download" derivation builder.
> +AC_PATH_PROG([GIT], [git])
> +if test "x$GIT" = "x"; then
> +  AC_MSG_ERROR([Git is missing; please install it.])
> +fi
> +AC_SUBST([GIT])

Since git is now a hard requirement, the conditional checks to disable
tests relying on git in the test suite, as added in previous commits of
this series are no longer needed and should be removed.

-- 
Thanks,
Maxim

Message #83 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 65866 <at> debbugs.gnu.org
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Wed, 20 Sep 2023 13:59:15 -0400

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> * tests/channels.scm (gpg+git-available?): Check for ‘gpg-command’
> only.
> Remove all ‘test-skip’ statements.
> * tests/derivations.scm: Likewise.
> * tests/git-authenticate.scm: Likewise.
> * tests/git.scm: Likewise.
> * tests/import-git.scm: Likewise.

Ah!  This invalidates my previous comment about doing what this commit
does :-).

LGTM.

-- 
Thanks,
Maxim

Message #86 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Thu, 21 Sep 2023 09:42:30 +0200

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

>> +(define* (perform-git-download drv #:optional output
>> +                               #:key print-build-trace?)
>> +  "Perform the download described by DRV, a fixed-output derivation, to
>> +OUTPUT.
>> +
>> +Note: Unless OUTPUT is #f, we don't read the value of 'out' in DRV since the
>> +actual output is different from that when we're doing a 'bmCheck' or
>
> I'd drop the 'we's and use impersonal imperative tense or at least
> 's/when we're doing/when doing/'.

Noted.  (That’s actually copied from ‘perform-download’; I’ll fix it
there as well.)

>> +'bmRepair' build."
>> +  (derivation-let drv ((output* "out")
>
> I'd name this variable just 'out', for consistency with the others.

No because there’s also a parameter called ‘output’ and there’s
(or output output*).  But lemme see, I should remove this optional
‘output’ parameter.

>> +;; 'with-temporary-git-repository' relies on the 'git' command.
>> +(unless (which (git-command)) (test-skip 1))
>
> I'd expect the 'git' command to now be required by Autoconf at build
> time, which should mean checking it here is not useful/required?

That comes in a subsequent patch.

>> +(test-assert "'git-download' built-in builder, not found"
>> +  (let* ((drv (derivation %store "git-download"
>> +                          "builtin:git-download" '()
>> +                          #:env-vars
>> +                          `(("url" . "file:///does-not-exist.git")
>> +                            ("commit"
>> +                             . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
>> +                          #:hash-algo 'sha256
>> +                          #:hash (gcrypt:sha256 #vu8())
>> +                          #:recursive? #t)))
>> +    (guard (c ((store-protocol-error? c)
>> +               (string-contains (store-protocol-error-message c) "failed")))
>> +      (build-derivations %store (list drv))
>> +      #f)))
>> +
>
> Maybe the error message compared could be more precised, if it already
> contains the necessary details?

Unfortunately it doesn’t (same strategy as with the existing
“builtin:download” tests.)

Thanks for your feedback!

Ludo’.

Message #89 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Fri, 22 Sep 2023 23:53:31 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

>> +(define* (git-fetch-with-fallback url commit directory
>> +                                  #:key (git-command "git") recursive?)
>> +  "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
>> +alternative methods when fetching from URL fails: attempt to download a nar,
>> +and if that also fails, download from the Software Heritage archive."
>> +  (or (git-fetch url commit directory
>> +                 #:recursive? recursive?
>> +                 #:git-command git-command)
>> +      (download-nar directory)
>> +
>> +      ;; As a last resort, attempt to download from Software Heritage.
>> +      ;; Disable X.509 certificate verification to avoid depending
>> +      ;; on nss-certs--we're authenticating the checkout anyway.
>> +      ;; XXX: Currently recursive checkouts are not supported.
>> +      (and (not recursive?)
>
> I know this is code moved from elsewhere, but it seems it'd be useful to
> fail hard here with a proper error instead of returning #f silently?  Or
> add support for recursive clones; was is missing to enable that?

Note that this is for the SWH fallback.  The SWH Vault doesn’t quite
support submodules; apparently there’s some work in that direction¹ but
it’s not there yet (though perhaps we could still implement it using
additional API endpoints, I’m not sure).

Ludo’.

¹ https://gitlab.softwareheritage.org/swh/devel/swh-vault/-/issues/4349

Message #92 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Fri, 22 Sep 2023 23:58:27 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Fixes <https://issues.guix.gnu.org/63331>.
>>
>> Longer-term this will remove Git from the derivation graph when its sole
>> use is to perform a checkout for a fixed-output derivation, thereby
>> breaking dependency cycles that can arise in these situations.
>>
>> * guix/git-download.scm (git-fetch): Rename to…
>> (git-fetch/in-band): … this.  Deal with GIT or GUILE being #f.
>
> Nitpick, but I find this usage of dynamic default argument on top of
> default arguments inelegant; see my comments below for an
> alternative.

Ah, let me explain…

>> +(define* (git-fetch/in-band ref hash-algo hash
>> +                            #:optional name
>> +                            #:key (system (%current-system))
>> +                            (guile (default-guile))
>> +                            (git (git-package)))
>> +  "Return a fixed-output derivation that performs a Git checkout of REF, using
>> +GIT and GUILE (thus, said derivation depends on GIT and GUILE).
>> +
>> +This method is deprecated in favor of the \"builtin:git-download\" builder.
>> +It will be removed when versions of guix-daemon implementing
>> +\"builtin:git-download\" will be sufficiently widespread."
>>    (define inputs
>> -    `(("git" ,git)
>> +    `(("git" ,(or git (git-package)))
>
> Instead of using 'or' here to ensure git has a value, the default values
> should have been copied to the new definition of git-fetch.

[...]

>> +(define* (git-fetch ref hash-algo hash
>> +                    #:optional name
>> +                    #:key (system (%current-system))
>> +                    guile git)
>
> As mentioned above, I'd have kept the default values for guile and git
> here.

The reason ‘guile’ and ‘git’ default to #f here is because we don’t need
them in what we expect to be the common case eventually:

>> +  "Return a fixed-output derivation that fetches REF, a <git-reference>
>> +object.  The output is expected to have recursive hash HASH of type
>> +HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
>> +  (mlet %store-monad ((builtins (built-in-builders*)))
>> +    (if (member "git-download" builtins)
>> +        (git-fetch/built-in ref hash-algo hash name
>> +                            #:system system)

So it’s an optimization to avoid module lookups when they’re
unnecessary.

I hope that makes sense!

Ludo’.

Message #95 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Sat, 23 Sep 2023 00:00:35 +0200

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> * configure.ac: Check for ‘git’ and substitute ‘GIT’.
>> * guix/config.scm.in (%git): New variable.
>> * guix/self.scm (compiled-guix): Define ‘git’ and pass it to
>> ‘make-config.scm’.
>> (make-config.scm): Add #:git; emit a ‘%git’ variable.
>> * doc/guix.texi (Requirements): Add it.
>
> I'm a bit confused; we *both* capture git from the build environment,
> and reference git from the git-minimal Guix package -- why can't we
> strictly rely on the captured Git from the environment?

That’s because we have two build systems: Autotools and (guix self).
It’s the same change semantically, but for each of these build systems.

> Nitpick: this commit should be ordered before the daemon changes that
> requires it.

I believe that’s the case.

Ludo’.

Message #98 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 1/8] git-download: Move fallback code to (guix build git).
Date: Sat, 23 Sep 2023 00:27:57 +0200

* guix/build/git.scm (git-fetch-with-fallback): New procedure, with code
taken from…
* guix/git-download.scm (git-fetch): … here.
[modules]: Remove modules that are no longer directly used in ‘build’.
[build]: Use ‘git-fetch-with-fallback’.
---
 guix/build/git.scm    | 44 ++++++++++++++++++++++++++++++++++++++--
 guix/git-download.scm | 47 ++++++++-----------------------------------
 2 files changed, 50 insertions(+), 41 deletions(-)

diff --git a/guix/build/git.scm b/guix/build/git.scm
index deda10fee8..0ff263c81b 100644
--- a/guix/build/git.scm
+++ b/guix/build/git.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2014, 2016, 2019 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2014, 2016, 2019, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -18,9 +18,12 @@
 
 (define-module (guix build git)
   #:use-module (guix build utils)
+  #:autoload   (guix build download-nar) (download-nar)
+  #:autoload   (guix swh) (%verify-swh-certificate? swh-download)
   #:use-module (srfi srfi-34)
   #:use-module (ice-9 format)
-  #:export (git-fetch))
+  #:export (git-fetch
+            git-fetch-with-fallback))
 
 ;;; Commentary:
 ;;;
@@ -76,4 +79,41 @@ (define* (git-fetch url commit directory
       (delete-file-recursively ".git")
       #t)))
 
+
+(define* (git-fetch-with-fallback url commit directory
+                                  #:key (git-command "git") recursive?)
+  "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
+alternative methods when fetching from URL fails: attempt to download a nar,
+and if that also fails, download from the Software Heritage archive."
+  (or (git-fetch url commit directory
+                 #:recursive? recursive?
+                 #:git-command git-command)
+      (download-nar directory)
+
+      ;; As a last resort, attempt to download from Software Heritage.
+      ;; Disable X.509 certificate verification to avoid depending
+      ;; on nss-certs--we're authenticating the checkout anyway.
+      ;; XXX: Currently recursive checkouts are not supported.
+      (and (not recursive?)
+           (parameterize ((%verify-swh-certificate? #f))
+             (format (current-error-port)
+                     "Trying to download from Software Heritage...~%")
+
+             (swh-download url commit directory)
+             (when (file-exists?
+                    (string-append directory "/.gitattributes"))
+               ;; Perform CR/LF conversion and other changes
+               ;; specificied by '.gitattributes'.
+               (invoke git-command "-C" directory "init")
+               (invoke git-command "-C" directory "config" "--local"
+                       "user.email" "you <at> example.org")
+               (invoke git-command "-C" directory "config" "--local"
+                       "user.name" "Your Name")
+               (invoke git-command "-C" directory "add" ".")
+               (invoke git-command "-C" directory "commit" "-am" "init")
+               (invoke git-command "-C" directory "read-tree" "--empty")
+               (invoke git-command "-C" directory "reset" "--hard")
+               (delete-file-recursively
+                (string-append directory "/.git")))))))
+
 ;;; git.scm ends here
diff --git a/guix/git-download.scm b/guix/git-download.scm
index d88f4c40ee..8989b1b463 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -116,19 +116,16 @@ (define* (git-fetch ref hash-algo hash
   (define modules
     (delete '(guix config)
             (source-module-closure '((guix build git)
-                                     (guix build utils)
-                                     (guix build download-nar)
-                                     (guix swh)))))
+                                     (guix build utils)))))
 
   (define build
     (with-imported-modules modules
-      (with-extensions (list guile-json gnutls   ;for (guix swh)
+      (with-extensions (list guile-json gnutls    ;for (guix swh)
                              guile-lzlib)
         #~(begin
             (use-modules (guix build git)
-                         (guix build utils)
-                         (guix build download-nar)
-                         (guix swh)
+                         ((guix build utils)
+                          #:select (set-path-environment-variable))
                          (ice-9 match))
 
             (define recursive?
@@ -151,38 +148,10 @@ (define* (git-fetch ref hash-algo hash
             (setvbuf (current-output-port) 'line)
             (setvbuf (current-error-port) 'line)
 
-            (or (git-fetch (getenv "git url") (getenv "git commit")
-                           #$output
-                           #:recursive? recursive?
-                           #:git-command "git")
-                (download-nar #$output)
-
-                ;; As a last resort, attempt to download from Software Heritage.
-                ;; Disable X.509 certificate verification to avoid depending
-                ;; on nss-certs--we're authenticating the checkout anyway.
-                ;; XXX: Currently recursive checkouts are not supported.
-                (and (not recursive?)
-                     (parameterize ((%verify-swh-certificate? #f))
-                       (format (current-error-port)
-                               "Trying to download from Software Heritage...~%")
-
-                       (swh-download (getenv "git url") (getenv "git commit")
-                                     #$output)
-                       (when (file-exists?
-                              (string-append #$output "/.gitattributes"))
-                         ;; Perform CR/LF conversion and other changes
-                         ;; specificied by '.gitattributes'.
-                         (invoke "git" "-C" #$output "init")
-                         (invoke "git" "-C" #$output "config" "--local"
-                                 "user.email" "you <at> example.org")
-                         (invoke "git" "-C" #$output "config" "--local"
-                                 "user.name" "Your Name")
-                         (invoke "git" "-C" #$output "add" ".")
-                         (invoke "git" "-C" #$output "commit" "-am" "init")
-                         (invoke "git" "-C" #$output "read-tree" "--empty")
-                         (invoke "git" "-C" #$output "reset" "--hard")
-                         (delete-file-recursively
-                          (string-append #$output "/.git"))))))))))
+            (git-fetch-with-fallback (getenv "git url") (getenv "git commit")
+                                     #$output
+                                     #:recursive? recursive?
+                                     #:git-command "git")))))
 
   (mlet %store-monad ((guile (package->derivation guile system)))
     (gexp->derivation (or name "git-checkout") build
-- 
2.41.0

Message #101 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 2/8] git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment variable.
Date: Sat, 23 Sep 2023 00:27:58 +0200

* guix/git-download.scm (git-fetch): Honor ‘%download-fallback-test’.
---
 guix/git-download.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/guix/git-download.scm b/guix/git-download.scm
index 8989b1b463..f1f19397c6 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -28,6 +28,7 @@ (define-module (guix git-download)
   #:use-module (guix packages)
   #:use-module (guix modules)
   #:autoload   (guix build-system gnu) (standard-packages)
+  #:autoload   (guix download) (%download-fallback-test)
   #:autoload   (git bindings)   (libgit2-init!)
   #:autoload   (git repository) (repository-open
                                  repository-close!
@@ -161,7 +162,11 @@ (define* (git-fetch ref hash-algo hash
                       ;; downloads.
                       #:script-name "git-download"
                       #:env-vars
-                      `(("git url" . ,(git-reference-url ref))
+                      `(("git url" . ,(match (%download-fallback-test)
+                                        ('content-addressed-mirrors
+                                         "https://example.org/does-not-exist")
+                                        (_
+                                         (git-reference-url ref))))
                         ("git commit" . ,(git-reference-commit ref))
                         ("git recursive?" . ,(object->string
                                               (git-reference-recursive? ref))))
-- 
2.41.0

Message #104 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH v2 0/8] Add built-in builder for Git checkouts
Date: Sat, 23 Sep 2023 00:27:56 +0200

Hello,

Changes compared to v1:

  • The ‘output’ parameter of the ‘perform-download’ and
    ‘perform-git-download’ procedures is now mandatory.
    Consequently, the confusing ‘output*’ variable is gone.

  • The docstring of these two procedures has been clarified
    accordingly.

  • I think there’s no third item.

Let me know if I missed something!

Thanks,
Ludo’.

Ludovic Courtès (8):
  git-download: Move fallback code to (guix build git).
  git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment
    variable.
  perform-download: Remove unused one-argument clause.
  daemon: Add “git-download” built-in builder.
  build: Add dependency on Git.
  perform-download: Use the ‘git’ command captured at configure time.
  git-download: Use “builtin:git-download” when available.
  tests: Assume ‘git’ is always available.

 configure.ac                      |   7 ++
 doc/guix.texi                     |   1 +
 guix/build/git.scm                |  44 ++++++++++-
 guix/config.scm.in                |   6 +-
 guix/git-download.scm             | 122 ++++++++++++++++++------------
 guix/scripts/perform-download.scm |  67 +++++++++++-----
 guix/self.scm                     |  10 ++-
 nix/libstore/builtins.cc          |   5 +-
 tests/builders.scm                |  29 ++++++-
 tests/channels.scm                |   7 +-
 tests/derivations.scm             |  94 ++++++++++++++++++++++-
 tests/git-authenticate.scm        |   1 -
 tests/git.scm                     |  10 ---
 tests/import-git.scm              |  18 -----
 14 files changed, 309 insertions(+), 112 deletions(-)


base-commit: 3d8d67ef6928f5d81118c97f03372cd341eab8b0
-- 
2.41.0

Message #107 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 3/8] perform-download: Remove unused one-argument clause.
Date: Sat, 23 Sep 2023 00:27:59 +0200

Code in ‘builtins.cc’ only ever invokes ‘guix perform-download’ with two
arguments.

* guix/scripts/perform-download.scm (guix-perform-download): Remove
unused one-argument clause.
(perform-download): Make ‘output’ parameter mandatory; remove ‘output*’
variable.
---
 guix/scripts/perform-download.scm | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index 6889bcef79..3b29a3c81d 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -42,16 +42,14 @@ (define %user-module
     (module-use! module (resolve-interface '(guix base32)))
     module))
 
-(define* (perform-download drv #:optional output
+(define* (perform-download drv output
                            #:key print-build-trace?)
   "Perform the download described by DRV, a fixed-output derivation, to
 OUTPUT.
 
-Note: Unless OUTPUT is #f, we don't read the value of 'out' in DRV since the
-actual output is different from that when we're doing a 'bmCheck' or
-'bmRepair' build."
+Note: OUTPUT may differ from the 'out' value of DRV, notably for 'bmCheck' or
+'bmRepair' builds."
   (derivation-let drv ((url "url")
-                       (output* "out")
                        (executable "executable")
                        (mirrors "mirrors")
                        (content-addressed-mirrors "content-addressed-mirrors")
@@ -59,8 +57,7 @@ (define* (perform-download drv #:optional output
     (unless url
       (leave (G_ "~a: missing URL~%") (derivation-file-name drv)))
 
-    (let* ((output     (or output output*))
-           (url        (call-with-input-string url read))
+    (let* ((url        (call-with-input-string url read))
            (drv-output (assoc-ref (derivation-outputs drv) "out"))
            (algo       (derivation-output-hash-algo drv-output))
            (hash       (derivation-output-hash drv-output)))
@@ -120,13 +117,8 @@ (define-command (guix-perform-download . args)
     (match args
       (((? derivation-path? drv) (? store-path? output))
        (assert-low-privileges)
-       (perform-download (read-derivation-from-file drv)
-                         output
-                         #:print-build-trace? print-build-trace?))
-      (((? derivation-path? drv))                 ;backward compatibility
-       (assert-low-privileges)
-       (perform-download (read-derivation-from-file drv)
-                         #:print-build-trace? print-build-trace?))
+       (let ((drv (read-derivation-from-file drv)))
+         (perform-download drv output #:print-build-trace? print-build-trace?)))
       (("--version")
        (show-version-and-exit))
       (x
-- 
2.41.0

Message #110 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 6/8] perform-download: Use the ‘git’ command captured at configure time.
Date: Sat, 23 Sep 2023 00:28:02 +0200

* guix/scripts/perform-download.scm (perform-git-download): Pass #:git-command
to ‘git-fetch-with-fallback’.
---
 guix/scripts/perform-download.scm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index bb1e51aa30..045dd84ad6 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -23,6 +23,7 @@ (define-module (guix scripts perform-download)
   #:use-module ((guix store) #:select (derivation-path? store-path?))
   #:autoload   (guix build download) (url-fetch)
   #:autoload   (guix build git) (git-fetch-with-fallback)
+  #:autoload   (guix config) (%git)
   #:use-module (ice-9 match)
   #:export (guix-perform-download))
 
@@ -108,7 +109,8 @@ (define* (perform-git-download drv output
            (algo       (derivation-output-hash-algo drv-output))
            (hash       (derivation-output-hash drv-output)))
       (git-fetch-with-fallback url commit output
-                               #:recursive? recursive?))))
+                               #:recursive? recursive?
+                               #:git-command %git))))
 
 (define (assert-low-privileges)
   (when (zero? (getuid))
-- 
2.41.0

Message #113 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludovic.courtes <at> inria.fr>
Subject: [PATCH v2 4/8] daemon: Add “git-download” built-in builder.
Date: Sat, 23 Sep 2023 00:28:00 +0200

From: Ludovic Courtès <ludovic.courtes <at> inria.fr>

The new builder makes it possible to break cycles that occurs when the
fixed-output derivation for the source of a dependency of ‘git’ would
itself depend on ‘git’.

* guix/scripts/perform-download.scm (perform-git-download): New
procedure.
(perform-download): Move fixed-output derivation check to…
(guix-perform-download): … here.  Invoke ‘perform-download’ or
‘perform-git-download’ depending on what ‘derivation-builder’ returns.
* nix/libstore/builtins.cc (builtins): Add “git-download”.
* tests/derivations.scm ("built-in-builders"): Update.
("'git-download' built-in builder")
("'git-download' built-in builder, invalid hash")
("'git-download' built-in builder, invalid commit")
("'git-download' built-in builder, not found"): New tests.
---
 guix/scripts/perform-download.scm |  49 ++++++++++++---
 nix/libstore/builtins.cc          |   5 +-
 tests/derivations.scm             | 100 +++++++++++++++++++++++++++++-
 3 files changed, 142 insertions(+), 12 deletions(-)

diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm
index 3b29a3c81d..bb1e51aa30 100644
--- a/guix/scripts/perform-download.scm
+++ b/guix/scripts/perform-download.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2016, 2017, 2018, 2020 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2016-2018, 2020, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -21,7 +21,8 @@ (define-module (guix scripts perform-download)
   #:use-module (guix scripts)
   #:use-module (guix derivations)
   #:use-module ((guix store) #:select (derivation-path? store-path?))
-  #:use-module (guix build download)
+  #:autoload   (guix build download) (url-fetch)
+  #:autoload   (guix build git) (git-fetch-with-fallback)
   #:use-module (ice-9 match)
   #:export (guix-perform-download))
 
@@ -61,10 +62,6 @@ (define* (perform-download drv output
            (drv-output (assoc-ref (derivation-outputs drv) "out"))
            (algo       (derivation-output-hash-algo drv-output))
            (hash       (derivation-output-hash drv-output)))
-      (unless (and algo hash)
-        (leave (G_ "~a is not a fixed-output derivation~%")
-               (derivation-file-name drv)))
-
       ;; We're invoked by the daemon, which gives us write access to OUTPUT.
       (when (url-fetch url output
                        #:print-build-trace? print-build-trace?
@@ -89,6 +86,30 @@ (define* (perform-download drv output
         (when (and executable (string=? executable "1"))
           (chmod output #o755))))))
 
+(define* (perform-git-download drv output
+                               #:key print-build-trace?)
+  "Perform the download described by DRV, a fixed-output derivation, to
+OUTPUT.
+
+Note: OUTPUT may differ from the 'out' value of DRV, notably for 'bmCheck' or
+'bmRepair' builds."
+  (derivation-let drv ((url "url")
+                       (commit "commit")
+                       (recursive? "recursive?"))
+    (unless url
+      (leave (G_ "~a: missing Git URL~%") (derivation-file-name drv)))
+    (unless commit
+      (leave (G_ "~a: missing Git commit~%") (derivation-file-name drv)))
+
+    (let* ((url        (call-with-input-string url read))
+           (recursive? (and recursive?
+                            (call-with-input-string recursive? read)))
+           (drv-output (assoc-ref (derivation-outputs drv) "out"))
+           (algo       (derivation-output-hash-algo drv-output))
+           (hash       (derivation-output-hash drv-output)))
+      (git-fetch-with-fallback url commit output
+                               #:recursive? recursive?))))
+
 (define (assert-low-privileges)
   (when (zero? (getuid))
     (leave (G_ "refusing to run with elevated privileges (UID ~a)~%")
@@ -117,8 +138,20 @@ (define-command (guix-perform-download . args)
     (match args
       (((? derivation-path? drv) (? store-path? output))
        (assert-low-privileges)
-       (let ((drv (read-derivation-from-file drv)))
-         (perform-download drv output #:print-build-trace? print-build-trace?)))
+       (let* ((drv (read-derivation-from-file drv))
+              (download (match (derivation-builder drv)
+                          ("builtin:download" perform-download)
+                          ("builtin:git-download" perform-git-download)
+                          (unknown (leave (G_ "~a: unknown builtin builder")
+                                          unknown))))
+              (drv-output (assoc-ref (derivation-outputs drv) "out"))
+              (algo       (derivation-output-hash-algo drv-output))
+              (hash       (derivation-output-hash drv-output)))
+         (unless (and hash algo)
+           (leave (G_ "~a is not a fixed-output derivation~%")
+                  (derivation-file-name drv)))
+
+         (download drv output #:print-build-trace? print-build-trace?)))
       (("--version")
        (show-version-and-exit))
       (x
diff --git a/nix/libstore/builtins.cc b/nix/libstore/builtins.cc
index 4111ac4760..6bf467354a 100644
--- a/nix/libstore/builtins.cc
+++ b/nix/libstore/builtins.cc
@@ -1,5 +1,5 @@
 /* GNU Guix --- Functional package management for GNU
-   Copyright (C) 2016, 2017, 2018, 2019 Ludovic Courtès <ludo <at> gnu.org>
+   Copyright (C) 2016-2019, 2023 Ludovic Courtès <ludo <at> gnu.org>
 
    This file is part of GNU Guix.
 
@@ -58,7 +58,8 @@ static void builtinDownload(const Derivation &drv,
 
 static const std::map<std::string, derivationBuilder> builtins =
 {
-    { "download", builtinDownload }
+    { "download", builtinDownload },
+    { "git-download", builtinDownload }
 };
 
 derivationBuilder lookupBuiltinBuilder(const std::string & name)
diff --git a/tests/derivations.scm b/tests/derivations.scm
index 66c777cfe7..e1312bd46b 100644
--- a/tests/derivations.scm
+++ b/tests/derivations.scm
@@ -24,10 +24,15 @@ (define-module (test-derivations)
   #:use-module (guix utils)
   #:use-module ((gcrypt hash) #:prefix gcrypt:)
   #:use-module (guix base32)
+  #:use-module ((guix git) #:select (with-repository))
   #:use-module (guix tests)
+  #:use-module (guix tests git)
   #:use-module (guix tests http)
   #:use-module ((guix packages) #:select (package-derivation base32))
-  #:use-module ((guix build utils) #:select (executable-file?))
+  #:use-module ((guix build utils) #:select (executable-file? which))
+  #:use-module ((guix hash) #:select (file-hash*))
+  #:use-module ((git oid) #:select (oid->string))
+  #:use-module ((git reference) #:select (reference-name->oid))
   #:use-module (gnu packages bootstrap)
   #:use-module ((gnu packages guile) #:select (guile-1.8))
   #:use-module (srfi srfi-1)
@@ -195,7 +200,7 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
                    (stat:ino (lstat file2))))))))
 
 (test-equal "built-in-builders"
-  '("download")
+  '("download" "git-download")
   (built-in-builders %store))
 
 (test-assert "unknown built-in builder"
@@ -290,6 +295,97 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
                          get-string-all)
                        text))))))
 
+;; 'with-temporary-git-repository' relies on the 'git' command.
+(unless (which (git-command)) (test-skip 1))
+(test-equal "'git-download' built-in builder"
+  `(("/a.txt" . "AAA")
+    ("/b.scm" . "#t"))
+  (let ((nonce (random-text)))
+    (with-temporary-git-repository directory
+        `((add "a.txt" "AAA")
+          (add "b.scm" "#t")
+          (commit ,nonce))
+      (let* ((commit (with-repository directory repository
+                       (oid->string
+                        (reference-name->oid repository "HEAD"))))
+             (drv (derivation %store "git-download"
+                              "builtin:git-download" '()
+                              #:env-vars
+                              `(("url"
+                                 . ,(object->string
+                                     (string-append "file://" directory)))
+                                ("commit" . ,commit))
+                              #:hash-algo 'sha256
+                              #:hash (file-hash* directory
+                                                 #:algorithm
+                                                 (gcrypt:hash-algorithm
+                                                  gcrypt:sha256)
+                                                 #:recursive? #t)
+                              #:recursive? #t)))
+        (build-derivations %store (list drv))
+        (directory-contents (derivation->output-path drv) get-string-all)))))
+
+(unless (which (git-command)) (test-skip 1))
+(test-assert "'git-download' built-in builder, invalid hash"
+  (with-temporary-git-repository directory
+      `((add "a.txt" "AAA")
+        (add "b.scm" "#t")
+        (commit "Commit!"))
+    (let* ((commit (with-repository directory repository
+                     (oid->string
+                      (reference-name->oid repository "HEAD"))))
+           (drv (derivation %store "git-download"
+                            "builtin:git-download" '()
+                            #:env-vars
+                            `(("url"
+                               . ,(object->string
+                                   (string-append "file://" directory)))
+                              ("commit" . ,commit))
+                            #:hash-algo 'sha256
+                            #:hash (gcrypt:sha256 #vu8())
+                            #:recursive? #t)))
+      (guard (c ((store-protocol-error? c)
+                 (string-contains (store-protocol-error-message c) "failed")))
+        (build-derivations %store (list drv))
+        #f))))
+
+(unless (which (git-command)) (test-skip 1))
+(test-assert "'git-download' built-in builder, invalid commit"
+  (with-temporary-git-repository directory
+      `((add "a.txt" "AAA")
+        (add "b.scm" "#t")
+        (commit "Commit!"))
+    (let* ((drv (derivation %store "git-download"
+                            "builtin:git-download" '()
+                            #:env-vars
+                            `(("url"
+                               . ,(object->string
+                                   (string-append "file://" directory)))
+                              ("commit"
+                               . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
+                            #:hash-algo 'sha256
+                            #:hash (gcrypt:sha256 #vu8())
+                            #:recursive? #t)))
+      (guard (c ((store-protocol-error? c)
+                 (string-contains (store-protocol-error-message c) "failed")))
+        (build-derivations %store (list drv))
+        #f))))
+
+(test-assert "'git-download' built-in builder, not found"
+  (let* ((drv (derivation %store "git-download"
+                          "builtin:git-download" '()
+                          #:env-vars
+                          `(("url" . "file:///does-not-exist.git")
+                            ("commit"
+                             . "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
+                          #:hash-algo 'sha256
+                          #:hash (gcrypt:sha256 #vu8())
+                          #:recursive? #t)))
+    (guard (c ((store-protocol-error? c)
+               (string-contains (store-protocol-error-message c) "failed")))
+      (build-derivations %store (list drv))
+      #f)))
+
 (test-equal "derivation-name"
   "foo-0.0"
   (let ((drv (derivation %store "foo-0.0" %bash '())))
-- 
2.41.0

Message #116 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 7/8] git-download: Use “builtin:git-download” when available.
Date: Sat, 23 Sep 2023 00:28:03 +0200

Fixes <https://issues.guix.gnu.org/63331>.

Longer-term this will remove Git from the derivation graph when its sole
use is to perform a checkout for a fixed-output derivation, thereby
breaking dependency cycles that can arise in these situations.

* guix/git-download.scm (git-fetch): Rename to…
(git-fetch/in-band): … this.  Deal with GIT or GUILE being #f.
(git-fetch/built-in, built-in-builders*, git-fetch): New procedures.
* tests/builders.scm ("git-fetch, file URI"): New test.
---
 guix/git-download.scm | 68 +++++++++++++++++++++++++++++++++++++------
 tests/builders.scm    | 29 +++++++++++++++++-
 2 files changed, 87 insertions(+), 10 deletions(-)

diff --git a/guix/git-download.scm b/guix/git-download.scm
index f1f19397c6..505dff0a89 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -27,6 +27,7 @@ (define-module (guix git-download)
   #:use-module (guix records)
   #:use-module (guix packages)
   #:use-module (guix modules)
+  #:use-module ((guix derivations) #:select (raw-derivation))
   #:autoload   (guix build-system gnu) (standard-packages)
   #:autoload   (guix download) (%download-fallback-test)
   #:autoload   (git bindings)   (libgit2-init!)
@@ -78,15 +79,19 @@ (define (git-package)
   (let ((distro (resolve-interface '(gnu packages version-control))))
     (module-ref distro 'git-minimal)))
 
-(define* (git-fetch ref hash-algo hash
-                    #:optional name
-                    #:key (system (%current-system)) (guile (default-guile))
-                    (git (git-package)))
-  "Return a fixed-output derivation that fetches REF, a <git-reference>
-object.  The output is expected to have recursive hash HASH of type
-HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
+(define* (git-fetch/in-band ref hash-algo hash
+                            #:optional name
+                            #:key (system (%current-system))
+                            (guile (default-guile))
+                            (git (git-package)))
+  "Return a fixed-output derivation that performs a Git checkout of REF, using
+GIT and GUILE (thus, said derivation depends on GIT and GUILE).
+
+This method is deprecated in favor of the \"builtin:git-download\" builder.
+It will be removed when versions of guix-daemon implementing
+\"builtin:git-download\" will be sufficiently widespread."
   (define inputs
-    `(("git" ,git)
+    `(("git" ,(or git (git-package)))
 
       ;; When doing 'git clone --recursive', we need sed, grep, etc. to be
       ;; available so that 'git submodule' works.
@@ -154,7 +159,8 @@ (define* (git-fetch ref hash-algo hash
                                      #:recursive? recursive?
                                      #:git-command "git")))))
 
-  (mlet %store-monad ((guile (package->derivation guile system)))
+  (mlet %store-monad ((guile (package->derivation (or guile (default-guile))
+                                                  system)))
     (gexp->derivation (or name "git-checkout") build
 
                       ;; Use environment variables and a fixed script name so
@@ -181,6 +187,50 @@ (define* (git-fetch ref hash-algo hash
                       #:recursive? #t
                       #:guile-for-build guile)))
 
+(define* (git-fetch/built-in ref hash-algo hash
+                             #:optional name
+                             #:key (system (%current-system)))
+  "Return a fixed-output derivation without any dependency that performs a Git
+checkout of REF, using the \"builtin:git-download\" derivation builder."
+  (raw-derivation (or name "git-checkout") "builtin:git-download" '()
+                  #:system system
+                  #:hash-algo hash-algo
+                  #:hash hash
+                  #:recursive? #t
+                  #:env-vars
+                  `(("url" . ,(object->string
+                               (match (%download-fallback-test)
+                                 ('content-addressed-mirrors
+                                  "https://example.org/does-not-exist")
+                                 (_
+                                  (git-reference-url ref)))))
+                    ("commit" . ,(git-reference-commit ref))
+                    ("recursive?" . ,(object->string
+                                      (git-reference-recursive? ref))))
+                  #:leaked-env-vars '("http_proxy" "https_proxy"
+                                      "LC_ALL" "LC_MESSAGES" "LANG"
+                                      "COLUMNS")
+                  #:local-build? #t))
+
+(define built-in-builders*
+  (store-lift built-in-builders))
+
+(define* (git-fetch ref hash-algo hash
+                    #:optional name
+                    #:key (system (%current-system))
+                    guile git)
+  "Return a fixed-output derivation that fetches REF, a <git-reference>
+object.  The output is expected to have recursive hash HASH of type
+HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
+  (mlet %store-monad ((builtins (built-in-builders*)))
+    (if (member "git-download" builtins)
+        (git-fetch/built-in ref hash-algo hash name
+                            #:system system)
+        (git-fetch/in-band ref hash-algo hash name
+                           #:system system
+                           #:guile guile
+                           #:git git))))
+
 (define (git-version version revision commit)
   "Return the version string for packages using git-download."
   ;; git-version is almost exclusively executed while modules are being loaded.
diff --git a/tests/builders.scm b/tests/builders.scm
index 0b5577c7a3..619caa5f31 100644
--- a/tests/builders.scm
+++ b/tests/builders.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2018, 2019, 2021 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2021 Lars-Dominik Braun <lars <at> 6xq.net>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -20,6 +20,7 @@
 
 (define-module (tests builders)
   #:use-module (guix download)
+  #:use-module (guix git-download)
   #:use-module (guix build-system)
   #:use-module (guix build-system gnu)
   #:use-module (guix build gnu-build-system)
@@ -31,9 +32,12 @@ (define-module (tests builders)
   #:use-module (guix base32)
   #:use-module (guix derivations)
   #:use-module (gcrypt hash)
+  #:use-module ((guix hash) #:select (file-hash*))
   #:use-module (guix tests)
+  #:use-module (guix tests git)
   #:use-module (guix packages)
   #:use-module (gnu packages bootstrap)
+  #:use-module ((ice-9 ftw) #:select (scandir))
   #:use-module (ice-9 match)
   #:use-module (ice-9 textual-ports)
   #:use-module (srfi srfi-1)
@@ -84,6 +88,29 @@ (define url-fetch*
     (and (file-exists? out)
          (valid-path? %store out))))
 
+(test-equal "git-fetch, file URI"
+  '("." ".." "a.txt" "b.scm")
+  (let ((nonce (random-text)))
+    (with-temporary-git-repository directory
+        `((add "a.txt" ,nonce)
+          (add "b.scm" "#t")
+          (commit "Commit.")
+          (tag "v1.0.0" "The tag."))
+      (run-with-store %store
+        (mlet* %store-monad ((hash
+                              -> (file-hash* directory
+                                             #:algorithm (hash-algorithm sha256)
+                                             #:recursive? #t))
+                             (drv (git-fetch
+                                   (git-reference
+                                    (url (string-append "file://" directory))
+                                    (commit "v1.0.0"))
+                                   'sha256 hash
+                                   "git-fetch-test")))
+          (mbegin %store-monad
+            (built-derivations (list drv))
+            (return (scandir (derivation->output-path drv)))))))))
+
 (test-assert "gnu-build-system"
   (build-system? gnu-build-system))
 
-- 
2.41.0

Message #119 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 5/8] build: Add dependency on Git.
Date: Sat, 23 Sep 2023 00:28:01 +0200

* configure.ac: Check for ‘git’ and substitute ‘GIT’.
* guix/config.scm.in (%git): New variable.
* guix/self.scm (compiled-guix): Define ‘git’ and pass it to
‘make-config.scm’.
(make-config.scm): Add #:git; emit a ‘%git’ variable.
* doc/guix.texi (Requirements): Add it.
---
 configure.ac       |  7 +++++++
 doc/guix.texi      |  1 +
 guix/config.scm.in |  6 +++++-
 guix/self.scm      | 10 +++++++++-
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 92dede8014..d817f620cf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -201,6 +201,13 @@ AC_SUBST([GZIP])
 AC_SUBST([BZIP2])
 AC_SUBST([XZ])
 
+dnl Git is now required for the "builtin:git-download" derivation builder.
+AC_PATH_PROG([GIT], [git])
+if test "x$GIT" = "x"; then
+  AC_MSG_ERROR([Git is missing; please install it.])
+fi
+AC_SUBST([GIT])
+
 LIBGCRYPT_LIBDIR="no"
 LIBGCRYPT_PREFIX="no"
 
diff --git a/doc/guix.texi b/doc/guix.texi
index 50c4984d71..8812e42e99 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -1011,6 +1011,7 @@ Requirements
 @item
 @uref{https://gitlab.com/guile-git/guile-git, Guile-Git}, version 0.5.0
 or later;
+@item @uref{https://git-scm.com, Git} (yes, both!);
 @item @uref{https://savannah.nongnu.org/projects/guile-json/, Guile-JSON}
 4.3.0 or later;
 @item @url{https://www.gnu.org/software/make/, GNU Make}.
diff --git a/guix/config.scm.in b/guix/config.scm.in
index d582d91d74..62e15dd713 100644
--- a/guix/config.scm.in
+++ b/guix/config.scm.in
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2018, 2019, 2021 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2012-2016, 2018-2019, 2021, 2023 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2017 Caleb Ristvedt <caleb.ristvedt <at> cune.org>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -35,6 +35,7 @@ (define-module (guix config)
             %config-directory
 
             %system
+            %git
             %gzip
             %bzip2
             %xz))
@@ -109,6 +110,9 @@ (define %config-directory
 (define %system
   "@guix_system@")
 
+(define %git
+  "@GIT@")
+
 (define %gzip
   "@GZIP@")
 
diff --git a/guix/self.scm b/guix/self.scm
index d2300052d8..9eaddc7a29 100644
--- a/guix/self.scm
+++ b/guix/self.scm
@@ -69,6 +69,7 @@ (define %packages
       ("gzip"               . ,(ref 'compression 'gzip))
       ("bzip2"              . ,(ref 'compression 'bzip2))
       ("xz"                 . ,(ref 'compression 'xz))
+      ("git-minimal"        . ,(ref 'version-control 'git-minimal))
       ("po4a"               . ,(ref 'gettext 'po4a))
       ("gettext-minimal"    . ,(ref 'gettext 'gettext-minimal))
       ("gcc-toolchain"      . ,(ref 'commencement 'gcc-toolchain))
@@ -826,6 +827,9 @@ (define* (compiled-guix source #:key
   (define guile-lzma
     (specification->package "guile-lzma"))
 
+  (define git
+    (specification->package "git-minimal"))
+
   (define dependencies
     (append-map transitive-package-dependencies
                 (list guile-gcrypt guile-gnutls guile-git guile-avahi
@@ -999,6 +1003,7 @@ (define* (compiled-guix source #:key
                     => ,(make-config.scm #:gzip gzip
                                          #:bzip2 bzip2
                                          #:xz xz
+                                         #:git git
                                          #:package-name
                                          %guix-package-name
                                          #:package-version
@@ -1104,7 +1109,7 @@ (define %default-config-variables
     (%storedir . "/gnu/store")
     (%sysconfdir . "/etc")))
 
-(define* (make-config.scm #:key gzip xz bzip2
+(define* (make-config.scm #:key gzip xz bzip2 git
                           (package-name "GNU Guix")
                           (package-version "0")
                           (channel-metadata #f)
@@ -1134,6 +1139,7 @@ (define* (make-config.scm #:key gzip xz bzip2
                                %state-directory
                                %store-database-directory
                                %config-directory
+                               %git
                                %gzip
                                %bzip2
                                %xz))
@@ -1176,6 +1182,8 @@ (define* (make-config.scm #:key gzip xz bzip2
                      ;; information is used by (guix describe).
                      '#$channel-metadata)
 
+                   (define %git
+                     #+(and git (file-append git "/bin/git")))
                    (define %gzip
                      #+(and gzip (file-append gzip "/bin/gzip")))
                    (define %bzip2
-- 
2.41.0

Message #122 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 65866 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH v2 8/8] tests: Assume ‘git’ is always available.
Date: Sat, 23 Sep 2023 00:28:04 +0200

* tests/channels.scm (gpg+git-available?): Check for ‘gpg-command’
only.
Remove all ‘test-skip’ statements.
* tests/derivations.scm: Likewise.
* tests/git-authenticate.scm: Likewise.
* tests/git.scm: Likewise.
* tests/import-git.scm: Likewise.
---
 tests/channels.scm         |  7 +------
 tests/derivations.scm      |  6 +-----
 tests/git-authenticate.scm |  1 -
 tests/git.scm              | 10 ----------
 tests/import-git.scm       | 18 ------------------
 5 files changed, 2 insertions(+), 40 deletions(-)

diff --git a/tests/channels.scm b/tests/channels.scm
index 62312e240c..6c4276deb4 100644
--- a/tests/channels.scm
+++ b/tests/channels.scm
@@ -50,7 +50,7 @@ (define-module (test-channels)
   #:use-module (ice-9 match))
 
 (define (gpg+git-available?)
-  (and (which (git-command))
+  (and #t                                         ;'git' is always available
        (which (gpg-command)) (which (gpgconf-command))))
 
 (define commit-id-string
@@ -196,7 +196,6 @@ (define channel-metadata-dependencies
                                           "abc1234")))
                          instances)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-channel-instances #:validate-pull"
   'descendant
 
@@ -306,7 +305,6 @@ (define channel-metadata-dependencies
                (depends? drv3
                          (list drv2 drv0) (list))))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "channel-news, no news"
   '()
   (with-temporary-git-repository directory
@@ -318,7 +316,6 @@ (define channel-metadata-dependencies
             (latest  (reference-name->oid repository "HEAD")))
         (channel-news-for-commit channel (oid->string latest))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "channel-news, one entry"
   (with-temporary-git-repository directory
       `((add ".guix-channel"
@@ -406,7 +403,6 @@ (define channel-metadata-dependencies
                          (channel-news-for-commit channel commit5 commit1))
                     '(#f "tag-for-first-news-entry")))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "channel-news, annotated tag"
   (with-temporary-git-repository directory
       `((add ".guix-channel"
@@ -453,7 +449,6 @@ (define channel-metadata-dependencies
                          (channel-news-for-commit channel commit2))
                     (list commit1)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "latest-channel-instances, missing introduction for 'guix'"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
diff --git a/tests/derivations.scm b/tests/derivations.scm
index e1312bd46b..0e87778981 100644
--- a/tests/derivations.scm
+++ b/tests/derivations.scm
@@ -29,7 +29,7 @@ (define-module (test-derivations)
   #:use-module (guix tests git)
   #:use-module (guix tests http)
   #:use-module ((guix packages) #:select (package-derivation base32))
-  #:use-module ((guix build utils) #:select (executable-file? which))
+  #:use-module ((guix build utils) #:select (executable-file?))
   #:use-module ((guix hash) #:select (file-hash*))
   #:use-module ((git oid) #:select (oid->string))
   #:use-module ((git reference) #:select (reference-name->oid))
@@ -295,8 +295,6 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
                          get-string-all)
                        text))))))
 
-;; 'with-temporary-git-repository' relies on the 'git' command.
-(unless (which (git-command)) (test-skip 1))
 (test-equal "'git-download' built-in builder"
   `(("/a.txt" . "AAA")
     ("/b.scm" . "#t"))
@@ -325,7 +323,6 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
         (build-derivations %store (list drv))
         (directory-contents (derivation->output-path drv) get-string-all)))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "'git-download' built-in builder, invalid hash"
   (with-temporary-git-repository directory
       `((add "a.txt" "AAA")
@@ -349,7 +346,6 @@ (define* (directory-contents dir #:optional (slurp get-bytevector-all))
         (build-derivations %store (list drv))
         #f))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "'git-download' built-in builder, invalid commit"
   (with-temporary-git-repository directory
       `((add "a.txt" "AAA")
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index c063920c12..4de223d422 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -44,7 +44,6 @@ (define (gpg+git-available?)
 
 (test-begin "git-authenticate")
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "unsigned commits"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
diff --git a/tests/git.scm b/tests/git.scm
index 9c944d65b1..ad43435b67 100644
--- a/tests/git.scm
+++ b/tests/git.scm
@@ -21,7 +21,6 @@ (define-module (test-git)
   #:use-module (git)
   #:use-module (guix git)
   #:use-module (guix tests git)
-  #:use-module (guix build utils)
   #:use-module ((guix utils) #:select (call-with-temporary-directory))
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-64)
@@ -33,8 +32,6 @@ (define-module (test-git)
 
 (test-begin "git")
 
-;; 'with-temporary-git-repository' relies on the 'git' command.
-(unless (which (git-command)) (test-skip 1))
 (test-assert "commit-difference, linear history"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
@@ -61,7 +58,6 @@ (define-module (test-git)
              ;; empty list.
              (null? (commit-difference commit1 commit4)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "commit-difference, fork"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
@@ -101,7 +97,6 @@ (define-module (test-git)
              (lset= eq? (commit-difference master4 master2)
                     (list master4 merge master3 devel1 devel2)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "commit-difference, excluded commits"
   (with-temporary-git-repository directory
       '((add "a.txt" "A")
@@ -126,7 +121,6 @@ (define-module (test-git)
                     (list commit4))
              (null? (commit-difference commit4 commit1 (list commit5))))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "commit-relation"
   '(self                                          ;master3 master3
     ancestor                                      ;master1 master3
@@ -166,7 +160,6 @@ (define-module (test-git)
               (commit-relation master1 merge)
               (commit-relation merge master1))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "commit-descendant?"
   '((master3 master3 => #t)
     (master1 master3 => #f)
@@ -216,7 +209,6 @@ (define-module (test-git)
                   (master1 merge)
                   (merge master1)))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "remote-refs"
   '("refs/heads/develop" "refs/heads/master"
     "refs/tags/v1.0" "refs/tags/v1.1")
@@ -231,7 +223,6 @@ (define-module (test-git)
         (tag "v1.1" "release-1.1"))
     (remote-refs directory)))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "remote-refs: only tags"
  '("refs/tags/v1.0" "refs/tags/v1.1")
   (with-temporary-git-repository directory
@@ -243,7 +234,6 @@ (define-module (test-git)
         (tag "v1.1" "Release 1.1"))
     (remote-refs directory #:tags? #t)))
 
-(unless (which (git-command)) (test-skip 1))
 (test-assert "update-cached-checkout, tag"
   (call-with-temporary-directory
    (lambda (cache)
diff --git a/tests/import-git.scm b/tests/import-git.scm
index f1bce154bb..20255dedb3 100644
--- a/tests/import-git.scm
+++ b/tests/import-git.scm
@@ -24,7 +24,6 @@ (define-module (test-import-git)
   #:use-module (guix import git)
   #:use-module (guix git-download)
   #:use-module (guix tests git)
-  #:use-module (guix build utils)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-64))
 
@@ -46,7 +45,6 @@ (define* (make-package directory version #:optional (properties '()))
         (base32
          "0000000000000000000000000000000000000000000000000000"))))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: no custom prefix, suffix, and delimiter"
   "1.0.1"
   (with-temporary-git-repository directory
@@ -56,7 +54,6 @@ (define* (make-package directory version #:optional (properties '()))
     (let ((package (make-package directory "1.0.0")))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom prefix, no suffix and delimiter"
   "1.0.1"
   (with-temporary-git-repository directory
@@ -67,7 +64,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-prefix . "prefix-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom suffix, no prefix and delimiter"
   "1.0.1"
   (with-temporary-git-repository directory
@@ -78,7 +74,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-suffix . "-suffix-[0-9]*")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom delimiter, no prefix and suffix"
   "2021.09.07"
   (with-temporary-git-repository directory
@@ -89,7 +84,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-version-delimiter . "-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: empty delimiter, no prefix and suffix"
   "20210907"
   (with-temporary-git-repository directory
@@ -100,7 +94,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((release-tag-version-delimiter . "")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom prefix and suffix, no delimiter"
   "2.0.0"
   (with-temporary-git-repository directory
@@ -112,7 +105,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-suffix . "suffix-[0-9]")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: custom prefix, suffix, and delimiter"
   "2.0.0"
   (with-temporary-git-repository directory
@@ -125,7 +117,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "_")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: only pre-releases available"
   #f
   (with-temporary-git-repository directory
@@ -135,7 +126,6 @@ (define* (make-package directory version #:optional (properties '()))
     (let ((package (make-package directory "1.0.0")))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases"
   "2.0.0-rc1"
   (with-temporary-git-repository directory
@@ -146,7 +136,6 @@ (define* (make-package directory version #:optional (properties '()))
                                  '((accept-pre-releases? . #t)))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom prefix"
   "2.0.0-rc1"
   (with-temporary-git-repository directory
@@ -158,7 +147,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-prefix . "version-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom suffix"
   "2.0.0-rc1"
   (with-temporary-git-repository directory
@@ -170,7 +158,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-suffix . "-suffix")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, delimiter conflicts with pre-release part"
   "2.0.0_alpha"
   (with-temporary-git-repository directory
@@ -182,7 +169,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "_")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom suffix and prefix"
   "2.0.0-alpha"
   (with-temporary-git-repository directory
@@ -195,7 +181,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-suffix . "-suffix")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, and custom suffix, prefix, and delimiter"
   "2.0.0-alpha"
   (with-temporary-git-repository directory
@@ -209,7 +194,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "-")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: accept pre-releases, no delimiter, and custom suffix, prefix"
   "2alpha"
   (with-temporary-git-repository directory
@@ -223,7 +207,6 @@ (define* (make-package directory version #:optional (properties '()))
                                    (release-tag-version-delimiter . "")))))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: no tags found"
   #f
   (with-temporary-git-repository directory
@@ -232,7 +215,6 @@ (define* (make-package directory version #:optional (properties '()))
     (let ((package (make-package directory "1.0.0")))
       (latest-git-tag-version package))))
 
-(unless (which (git-command)) (test-skip 1))
 (test-equal "latest-git-tag-version: no valid tags found"
   #f
   (with-temporary-git-repository directory
-- 
2.41.0

Message #125 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 65866 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] [PATCH v2 1/8] git-download: Move fallback code to
 (guix build git).
Date: Mon, 25 Sep 2023 10:15:21 +0200

Hi Ludo,

On Sat, 23 Sep 2023 at 00:27, Ludovic Courtès <ludo <at> gnu.org> wrote:

> +      ;; XXX: Currently recursive checkouts are not supported.
> +      (and (not recursive?)

Naively, and similarly as Maxim’s remark, maybe we could raise a warning
when the case is not supported instead of silently return #f.

If raising a warning here is not appropriate, maybe document in the
docstring the returned value as #f.
                                 

> -      (with-extensions (list guile-json gnutls   ;for (guix swh)
> +      (with-extensions (list guile-json gnutls    ;for (guix swh)

Nitpick: Since the change is complex, I would suggest to avoid this
cosmetic.


Cheers,
simon

Message #128 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 65866 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] [PATCH v2 7/8] git-download: Use
 “builtin:git-download” when available.
Date: Mon, 25 Sep 2023 10:33:33 +0200

Hi Ludo,

On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo <at> gnu.org> wrote:

> +(define* (git-fetch/built-in ref hash-algo hash
> +                             #:optional name
> +                             #:key (system (%current-system)))
> +  "Return a fixed-output derivation without any dependency that performs a Git
> +checkout of REF, using the \"builtin:git-download\" derivation builder."

I do not understand what means “without any dependency” here.  I would
drop it.

Cheers,
simon

Message #131 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 11:23:18 +0200

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> +(define* (git-fetch/built-in ref hash-algo hash
>> +                             #:optional name
>> +                             #:key (system (%current-system)))
>> +  "Return a fixed-output derivation without any dependency that performs a Git
>> +checkout of REF, using the \"builtin:git-download\" derivation builder."
>
> I do not understand what means “without any dependency” here.

It means the derivation does not depend on anything (it has zero
“sources” and zero “input derivations”).  That’s fundamental here, which
is why I made it explicit.

Ludo’.

Message #134 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 11:24:33 +0200

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Sat, 23 Sep 2023 at 00:27, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> +      ;; XXX: Currently recursive checkouts are not supported.
>> +      (and (not recursive?)
>
> Naively, and similarly as Maxim’s remark, maybe we could raise a warning
> when the case is not supported instead of silently return #f.
>
> If raising a warning here is not appropriate, maybe document in the
> docstring the returned value as #f.

I agree, but let’s discuss that separately from this review if you don’t
mind (the code here is not new).

Ludo’.

Message #137 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 14:13:46 +0200

Hi,

On Mon, 25 Sep 2023 at 11:24, Ludovic Courtès <ludo <at> gnu.org> wrote:

>> If raising a warning here is not appropriate, maybe document in the
>> docstring the returned value as #f.
>
> I agree, but let’s discuss that separately from this review if you don’t
> mind (the code here is not new).

Since this change introduces a new procedure publicly exposed, maybe a
line in the docstring about the “contract” would help.

--8<---------------cut here---------------start------------->8---
  "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
alternative methods when fetching from URL fails: attempt to download a nar,
and if that also fails, download from the Software Heritage archive;
recursive checkouts are not supported when falling back to Software
Heritage, return #false if all methods fail."
--8<---------------cut here---------------end--------------->8---

Well, if that’s inappropriate, let address it separately. :-)

Cheers,
simon

Message #140 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 14:37:17 +0200

Hi,

On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo <at> gnu.org> wrote:
> Simon Tournier <zimon.toutoune <at> gmail.com> skribis:
>
>> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo <at> gnu.org> wrote:
>>
>>> +(define* (git-fetch/built-in ref hash-algo hash
>>> +                             #:optional name
>>> +                             #:key (system (%current-system)))
>>> +  "Return a fixed-output derivation without any dependency that performs a Git
>>> +checkout of REF, using the \"builtin:git-download\" derivation builder."
>>
>> I do not understand what means “without any dependency” here.
>
> It means the derivation does not depend on anything (it has zero
> “sources” and zero “input derivations”).  That’s fundamental here, which
> is why I made it explicit.

Aaah :-)  Yeah that makes sense.

Well, from my understanding, « fixed-output derivation without any
dependency » is not clear at first (we are always dependent on something
else ;-)) and I am sure that weeks or months later, I will not remember
that it means: « zero “sources” and zero “input derivations” ».

Hum, once it is known, it is hard to find a better wording though. ;-)

I do not know if it is better, at least, it appears clearer from my
point of view:

      "Return a fixed-output derivation that performs a Git checkout of REF,
    using the \"builtin:git-download\" derivation builder, thus without any
    dependency other than from builtin."


Cheers,
simon

Message #143 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 14:48:04 +0200

Re,

On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo <at> gnu.org> wrote:

>>> +  "Return a fixed-output derivation without any dependency that performs a Git
>>> +checkout of REF, using the \"builtin:git-download\" derivation builder."
>>
>> I do not understand what means “without any dependency” here.
>
> It means the derivation does not depend on anything (it has zero
> “sources” and zero “input derivations”).  That’s fundamental here, which
> is why I made it explicit.

For instance, in ’built-in-download’, the docstring reads:

--8<---------------cut here---------------start------------->8---
This is an \"out-of-band\" download in that the returned derivation does not
explicitly depend on Guile, GnuTLS, etc.  Instead, the daemon performs the
download by itself using its own dependencies.
--8<---------------cut here---------------end--------------->8---

which I find “clearer” than “without any dependency”.

Well, enough for nitpicking. :-)

Cheers,
simon

Message #146 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 65866 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] [PATCH v2 5/8] build: Add dependency on Git.
Date: Mon, 25 Sep 2023 15:59:17 +0200

Hi Ludo,

On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo <at> gnu.org> wrote:
> * configure.ac: Check for ‘git’ and substitute ‘GIT’.
> * guix/config.scm.in (%git): New variable.
> * guix/self.scm (compiled-guix): Define ‘git’ and pass it to
> ‘make-config.scm’.
> (make-config.scm): Add #:git; emit a ‘%git’ variable.
> * doc/guix.texi (Requirements): Add it.

Moving the Git dependency to a daemon dependency tweaks a bit what we
control when “bootstrapping”, no?  Maybe I misread or misunderstand a
point.

Currently, the full bootstrap story requires the binary seed (well
documented in the manual :-)), running a Linux kernel and a Guix daemon.
And then, everything is built one after the other, resolving the chain
of dependencies.

And obviously, there is another chicken-or-the-egg problem barely
discussed.  For building something, we first need to communicate with
the world for fetching the source code to build. :-)

It is not Git specific and also happens with ’url-fetch’ – as reported
very early (see #22774 from 2016).  For instance, TLS is required before
Guix has built GnuTLS.  The chicken-or-the-egg had been solved by hiding
this dependency as a dependency of the daemon.  A hack.

Therefore, it means that the Reduced Binary Seed bootstrap is somehow
enlarged by what the Guix daemon depends on.

I mean, breaking the dependency cycles by introducing a dependency on
Git (as for fixing #63331) is not free of other dependencies.  It is
another hack when it could be avoided since the issue is about
Guile-GnuTLS.

It means we are doing a step back for a full bootstrap story, IMHO,
having a “builtin:git-download” makes less clear what are the hard
dependencies to what Guix is able to build from source starting from
almost nothing.

Cheers,
simon

Message #149 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 11:49:44 -0400

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

> Re,
>
> On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>>>> +  "Return a fixed-output derivation without any dependency that performs a Git
>>>> +checkout of REF, using the \"builtin:git-download\" derivation builder."
>>>
>>> I do not understand what means “without any dependency” here.
>>
>> It means the derivation does not depend on anything (it has zero
>> “sources” and zero “input derivations”).  That’s fundamental here, which
>> is why I made it explicit.
>
> For instance, in ’built-in-download’, the docstring reads:
>
> This is an \"out-of-band\" download in that the returned derivation does not
> explicitly depend on Guile, GnuTLS, etc.  Instead, the daemon performs the
> download by itself using its own dependencies.
>
> which I find “clearer” than “without any dependency”.
>
> Well, enough for nitpicking. :-)

I also think the built-in-download's docstring explicitness is nicer.

-- 
Thanks,
Maxim

Message #152 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 11:56:37 -0400

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>
>>> Fixes <https://issues.guix.gnu.org/63331>.
>>>
>>> Longer-term this will remove Git from the derivation graph when its sole
>>> use is to perform a checkout for a fixed-output derivation, thereby
>>> breaking dependency cycles that can arise in these situations.
>>>
>>> * guix/git-download.scm (git-fetch): Rename to…
>>> (git-fetch/in-band): … this.  Deal with GIT or GUILE being #f.
>>
>> Nitpick, but I find this usage of dynamic default argument on top of
>> default arguments inelegant; see my comments below for an
>> alternative.
>
> Ah, let me explain…
>
>>> +(define* (git-fetch/in-band ref hash-algo hash
>>> +                            #:optional name
>>> +                            #:key (system (%current-system))
>>> +                            (guile (default-guile))
>>> +                            (git (git-package)))
>>> +  "Return a fixed-output derivation that performs a Git checkout of REF, using
>>> +GIT and GUILE (thus, said derivation depends on GIT and GUILE).
>>> +
>>> +This method is deprecated in favor of the \"builtin:git-download\" builder.
>>> +It will be removed when versions of guix-daemon implementing
>>> +\"builtin:git-download\" will be sufficiently widespread."
>>>    (define inputs
>>> -    `(("git" ,git)
>>> +    `(("git" ,(or git (git-package)))
>>
>> Instead of using 'or' here to ensure git has a value, the default values
>> should have been copied to the new definition of git-fetch.
>
> [...]
>
>>> +(define* (git-fetch ref hash-algo hash
>>> +                    #:optional name
>>> +                    #:key (system (%current-system))
>>> +                    guile git)
>>
>> As mentioned above, I'd have kept the default values for guile and git
>> here.
>
> The reason ‘guile’ and ‘git’ default to #f here is because we don’t need
> them in what we expect to be the common case eventually:
>
>>> +  "Return a fixed-output derivation that fetches REF, a <git-reference>
>>> +object.  The output is expected to have recursive hash HASH of type
>>> +HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
>>> +  (mlet %store-monad ((builtins (built-in-builders*)))
>>> +    (if (member "git-download" builtins)
>>> +        (git-fetch/built-in ref hash-algo hash name
>>> +                            #:system system)
>
> So it’s an optimization to avoid module lookups when they’re
> unnecessary.
>
> I hope that makes sense!

Oh!  I guess it does, but shouldn't git-fetch/in-band also not use guile
and git as default values then?  I'd like to see the same strategy used
in both places for consistency, with an added explanatory comment (in
the user-facing git-fetch) with what you explained here :-).

-- 
Thanks,
Maxim

Message #155 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Mon, 25 Sep 2023 11:59:18 -0400

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>
>>> * configure.ac: Check for ‘git’ and substitute ‘GIT’.
>>> * guix/config.scm.in (%git): New variable.
>>> * guix/self.scm (compiled-guix): Define ‘git’ and pass it to
>>> ‘make-config.scm’.
>>> (make-config.scm): Add #:git; emit a ‘%git’ variable.
>>> * doc/guix.texi (Requirements): Add it.
>>
>> I'm a bit confused; we *both* capture git from the build environment,
>> and reference git from the git-minimal Guix package -- why can't we
>> strictly rely on the captured Git from the environment?
>
> That’s because we have two build systems: Autotools and (guix self).
> It’s the same change semantically, but for each of these build systems.

Oof, thanks for explaining.

>> Nitpick: this commit should be ordered before the daemon changes that
>> requires it.
>
> I believe that’s the case.

Indeed, I should sort by subject in Gnus when reviewing to get the
correct ordering!

The series LGTM with the comments from Simon and myself in other replies
taken into account.

-- 
Thanks,
Maxim

Message #158 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Bootstrapping without the daemon and all that
Date: Tue, 26 Sep 2023 16:05:30 +0200

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo <at> gnu.org> wrote:
>> * configure.ac: Check for ‘git’ and substitute ‘GIT’.
>> * guix/config.scm.in (%git): New variable.
>> * guix/self.scm (compiled-guix): Define ‘git’ and pass it to
>> ‘make-config.scm’.
>> (make-config.scm): Add #:git; emit a ‘%git’ variable.
>> * doc/guix.texi (Requirements): Add it.
>
> Moving the Git dependency to a daemon dependency tweaks a bit what we
> control when “bootstrapping”, no?  Maybe I misread or misunderstand a
> point.

The model in Guix is that there’s a daemon to “emulate” a build “from
scratch”.

One can question whether the model matches reality, and this is what I
and fellow hackers looked at during the 2019 R-B Summit:

  https://guix.gnu.org/en/blog/2019/reproducible-builds-summit-5th-edition/
  (under “Extreme Bootstrapping”)

(The ‘wip-system-bootstrap’ branch still exists!)

Anyway, we’re drifting away from this patch series!

Ludo’.

Message #163 received at 65866-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866-done <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Tue, 26 Sep 2023 17:44:22 +0200

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> Re,
>
> On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>>>> +  "Return a fixed-output derivation without any dependency that performs a Git
>>>> +checkout of REF, using the \"builtin:git-download\" derivation builder."
>>>
>>> I do not understand what means “without any dependency” here.
>>
>> It means the derivation does not depend on anything (it has zero
>> “sources” and zero “input derivations”).  That’s fundamental here, which
>> is why I made it explicit.
>
> For instance, in ’built-in-download’, the docstring reads:
>
> This is an \"out-of-band\" download in that the returned derivation does not
> explicitly depend on Guile, GnuTLS, etc.  Instead, the daemon performs the
> download by itself using its own dependencies.
>
> which I find “clearer” than “without any dependency”.

Yes, good idea.

I changed the docstring as you suggest and pushed the whole thing:

  ba21eeb565 tests: Assume ‘git’ is always available.
  13b0cf85eb git-download: Use “builtin:git-download” when available.
  c4a1d69a69 perform-download: Use the ‘git’ command captured at configure time.
  f651a35969 build: Add dependency on Git.
  95f2123135 daemon: Add “git-download” built-in builder.
  9d0e2002a5 perform-download: Remove unused one-argument clause.
  c7ed1e0160 git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment variable.
  811b249397 git-download: Move fallback code to (guix build git).

I’ll update the ‘guix’ package soon so we can start using the new
builtin.

Thanks to the two of you!

Ludo’.

Message #166 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: Bootstrapping without the daemon and all that
Date: Tue, 26 Sep 2023 19:04:33 +0200

Hi Ludo,

On Tue, 26 Sept 2023 at 16:05, Ludovic Courtès <ludo <at> gnu.org> wrote:

> > Moving the Git dependency to a daemon dependency tweaks a bit what we
> > control when “bootstrapping”, no?  Maybe I misread or misunderstand a
> > point.
>
> The model in Guix is that there’s a daemon to “emulate” a build “from
> scratch”.

Yes and that "emulate" will be bigger.

>   https://guix.gnu.org/en/blog/2019/reproducible-builds-summit-5th-edition/
>   (under “Extreme Bootstrapping”)

Thanks for the reference.  I have forgotten it.  Yes, that's it. :-)

Adding Git as dependency to the daemon is adding Git in the Trusted
Computing Base.  It appears to me important to raise and to not hide
under the carpet. :-)

> (The ‘wip-system-bootstrap’ branch still exists!)

Having a potential solution does not make pointless the current concern. ;-)

> Anyway, we’re drifting away from this patch series!

No, it is not drifting.  The addition of Git in the trusting trust
story cannot be dismissed, IMHO.

It is not drifting to discuss for reaching some consensus about the
"risk" of enlarging the trusting trust computing base.  For example,
is this "risk" worth the corner case of Guile-GnuTLS?

As I said elsewhere, adding something is often much easier than
removing something.  Here the addition of Git has some implications
(libgit2, trusted computing base, etc.) and it is always about the
right balance.  Do we have the right balance here?  The discussion
about concrete concerns for the addition of Git as dependency helps in
reinforcing the consensus that this change is worth despite the
downsides.

To make it explicit: is this series worth the Guile-GnuTLS/Git
circular dependency corner case?  Maybe it is already all clear for
you, and your answer is a big YES. :-)  And perhaps it is the only
answer. :-)  But it does not mean the answer is fully clear for
everybody, at least it is not necessary straightforward for me.
Somehow, do we have a consensus about the way that this series is
worth the Guile-GnuTLS/Git circular dependency corner case?  And a
consensus about the way that this series is The Right Thing for that
circular dependency?

Cheers,
simon

Message #169 received at 65866-done <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866-done <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Tue, 26 Sep 2023 19:13:31 +0200

On Tue, 26 Sept 2023 at 17:44, Ludovic Courtès <ludo <at> gnu.org> wrote:

> I changed the docstring as you suggest and pushed the whole thing:

I am not convinced that we reached a consensus about this series.
Because enlarging the "Trusting Computing Base" as this series does
cannot be dismissed as "drifting" and had not been discussed at all
before pushing although I raised the concern.

All the best,
simon

Message #172 received at 65866-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866-done <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: bug#65866: [PATCH 0/8] Add built-in builder for Git checkouts
Date: Sun, 01 Oct 2023 17:02:05 +0200

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Tue, 26 Sept 2023 at 17:44, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> I changed the docstring as you suggest and pushed the whole thing:
>
> I am not convinced that we reached a consensus about this series.
> Because enlarging the "Trusting Computing Base" as this series does
> cannot be dismissed as "drifting" and had not been discussed at all
> before pushing although I raised the concern.

The reasoning for “builtin:git-download” is the same as for
“builtin:download”, which was introduced in 2016¹, and which is itself a
logical followup to the notion of fixed-output derivations.

None of these increases the TCB because they’re about downloading data
whose contents are known in advance.

I think it’s important in these discussions to make sure we start from a
shared understanding so we can remain focused and productive.

Ludo’.

¹ https://issues.guix.gnu.org/22774

Message #175 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] Bootstrapping without the daemon and all that
Date: Thu, 12 Oct 2023 12:54:54 +0200

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> To make it explicit: is this series worth the Guile-GnuTLS/Git
> circular dependency corner case?  Maybe it is already all clear for
> you, and your answer is a big YES. :-)  And perhaps it is the only
> answer. :-)  But it does not mean the answer is fully clear for
> everybody, at least it is not necessary straightforward for me.
> Somehow, do we have a consensus about the way that this series is
> worth the Guile-GnuTLS/Git circular dependency corner case?  And a
> consensus about the way that this series is The Right Thing for that
> circular dependency?

One thing I probably didn’t explain clearly is that yes, the circular
dependency issue is one we have to solve.  For years, I hope we could
avoid it but experience has shown that no, it’s a problem we did have to
address.

One example is Guile-GnuTLS being built from a Git checkout.  Another
one is Hurd packages in commencement.scm built from a Git checkout.  We
had to go to great lengths to avoid ‘git-fetch’:

  https://issues.guix.gnu.org/64708#6

More and more software is built from Git checkouts rather than tarballs.
In the long run, we won’t be able to guarantee that none of the
dependencies of ‘git-minimal’ takes its source via ‘git-fetch’.

This is especially true if we want to move away from Autotools-generated
tarballs and instead run ‘autoreconf’ ourselves on pristine source.

Ludo’.

Message #178 received at 65866 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#65866] Bootstrapping without the daemon and all that
Date: Mon, 16 Oct 2023 10:46:06 +0200

Hi Ludo,

On Thu, 12 Oct 2023 at 12:54, Ludovic Courtès <ludo <at> gnu.org> wrote:

>> To make it explicit: is this series worth the Guile-GnuTLS/Git
>> circular dependency corner case?  Maybe it is already all clear for
>> you, and your answer is a big YES. :-)  And perhaps it is the only
>> answer. :-)  But it does not mean the answer is fully clear for
>> everybody, at least it is not necessary straightforward for me.
>> Somehow, do we have a consensus about the way that this series is
>> worth the Guile-GnuTLS/Git circular dependency corner case?  And a
>> consensus about the way that this series is The Right Thing for that
>> circular dependency?
>
> One thing I probably didn’t explain clearly is that yes, the circular
> dependency issue is one we have to solve.  For years, I hope we could
> avoid it but experience has shown that no, it’s a problem we did have to
> address.

There is different ways to solve this circular dependency.


> One example is Guile-GnuTLS being built from a Git checkout.  Another
> one is Hurd packages in commencement.scm built from a Git checkout.  We
> had to go to great lengths to avoid ‘git-fetch’:
>
>   https://issues.guix.gnu.org/64708#6

Somehow, I think it is the direction to explore: have some
’git-fetch-bootstrap’ which relies on some ’builtin:git-download’ and
guix-daemon side code, and then that being used to have the packages
required by some ’git-fetch’ without guix-daemon side code.

Doing so, we would minimize the opaque – hard to audit – code, which
means less power to guix-daemon, we keep the control, etc.  It appears
to me more consistent with the general approach elsewhere.  Anyway.

Rehashing all, your opinion was already made when you sent this patch.
You wrote since the very beginning on 6 May 2023 for Guile-GnuTLS [1]:

        The longer-term solution is to add a “builtin:git-download”
        derivation builder, just like we have “builtin:download”.  The
        implementation should be relatively easy, but we’ll have to be
        able to deal with daemons that lack this builtin possibly for
        several years.

        https://issues.guix.gnu.org/63331#0

Therefore, this patch was not an open discussion for some design but the
review of some code for fixing concrete issues.  No hard feeling, we
need to make progress after all; the issue is pending since months.
However, when giving a look at this patch, it was not my expectations.

It is my own mistake to have not been enough active before with the
issue.  I had months to discuss some design for the circular
dependencies of the fetching methods.  Since I am spending some time
reading about what is going on with Guix, I think we have a failure in
some process.

IMHO, we are missing a Request For Comments and I will send a proposal
for some implementation details this week.

Cheers,
simon

Message #181 received at 65866-done <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, GNU Guix maintainers
 <guix-maintainers <at> gnu.org>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 65866-done <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>
Subject: Toward RFC? (was Re: [bug#65866] [PATCH 0/8] Add built-in builder
 for Git checkouts)
Date: Mon, 16 Oct 2023 11:11:25 +0200

Hi Ludo,

We are not on the same wavelength about some technical parts.  It does
not really matter – we could tune our frequency separately on some
random occasions. :-)

However…

On Sun, 1 Oct 2023 at 17:02, Ludovic Courtès <ludo <at> gnu.org> wrote:

> I think it’s important in these discussions to make sure we start from a
> shared understanding so we can remain focused and productive.

…I am, between sad and upset, by this part.  It appears to me unfair or
uncalled for.

« A group using consensus is committed to finding solutions that
everyone actively supports, or at least can live with. » [1].  And I am
sorry to say that we have a failure here; at the root (Guile-GnuTLS bug
report).  And it is a group failure.

Elsewhere in this thread,, I have expressed « I am not convinced that we
reached a consensus about this series. ».  Why no one is asking if I am
blocking this patch?  Anyway!

Thinking more than twice about why it bothers me.  If I feel a failure
about the consensus, then it is because the process fails from my point
of view.  Why?  Because an implementation detail is missing.  It had
been expressed several times.  Notably:

        Time for a request-for-comments process?
        Ludovic Courtès <ludo <at> gnu.org>
        Wed, 27 Oct 2021 23:22:50 +0200
        id:87cznqb1sl.fsf <at> inria.fr
        https://lists.gnu.org/archive/html/guix-devel/2021-10
        https://yhetil.org/guix/87cznqb1sl.fsf <at> inria.fr

And this kind of change « Add Git as hard dependency » should have been
part of such process, IMHO.  Because considering how the current
decision process works, it is impossible to get this “shared
understanding” if you have not been here at the right moment.

How can I acquire this shared understanding?  For example, you said that
Git or TLS or etc. on daemon side are not part of the TCB because they
are used for fixed-output derivations, I disagree and I still think it
is incorrect.  The problem is not my disagreement – I can live with it,
as many others ;-) – no, the problem is that you refer to implicit
decision that I cannot digest, question or ask explanations.  Somehow, I
do not have some gauge for evaluating my own expectations.  It appears
to me that this patch falls under similar circumstances as an idea
expressed here:

        [bug#61894] [PATCH RFC] Team approval for patches
        Ludovic Courtès <ludo <at> gnu.org>
        Wed, 01 Mar 2023 17:13:28 +0100
        id:878rgga1qv.fsf <at> inria.fr
        https://lists.gnu.org/archive/html/guix-devel/2023-03
        https://yhetil.org/guix/878rgga1qv.fsf <at> inria.fr

Now, Guix is probably reaching a point where we deserve more structure
without much burden for making decisions about changes of some category.

Therefore, let turn my own frustration here into a concrete proposal, I
will send this week a Request-For-Comment process inspired by Rust, Nix
and Haskell ones.

Cheers,
simon

1: https://www.seedsforchange.org.uk/consensus
2: https://guix.gnu.org/blog/2019/gnu-guix-maintainer-collective-expands/

Message #184 received at 65866-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>,
 GNU Guix maintainers <guix-maintainers <at> gnu.org>,
 Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Ricardo Wurmus <rekado <at> elephly.net>, 65866-done <at> debbugs.gnu.org,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: Toward RFC? (was Re: [bug#65866] [PATCH 0/8] Add built-in
 builder for Git checkouts)
Date: Mon, 30 Oct 2023 16:12:02 +0100

Hi Simon and all,

This is again a long message but I’ll try to reply shortly.

First, my apologies as I failed to understand that you were rejecting
the change altogether—I interpreted your comments on details about the
patches as a sign that you were not opposing the idea in itself.

Second, the sentence of mine you quoted was poorly worded.  I felt with
your last messages in the thread that you were questioning
“builtin:download” itself, which was going way beyond the scope of this
patch.

Last, and probably more importantly: I’m all for an RFC process as you
propose!  I think this is something we desperately need anytime we make
non-trivial changes to the core, to command-line interfaces, etc.  I’ll
be there to support a move in that direction as much as I can.

So… thank you for making pushing this proposal.

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.