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

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

[Ludovic Courtès]

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

[bug#65866] [PATCH 1/8] git-download: Move fallback code to (guix build git).

[Ludovic Courtès]

* 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@gnu.org>
+;;; Copyright © 2014, 2016, 2019, 2023 Ludovic Courtès <ludo@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@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@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

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

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès <ludo@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@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@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

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

[Simon Tournier]

Hi Maxim,

On Wed, 20 Sept 2023 at 18:05, Maxim Cournoyer
<maxim.cournoyer@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@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

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

[Ludovic Courtès]

Hi,

Maxim Cournoyer <maxim.cournoyer@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

[bug#65866] [PATCH 2/8] git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment variable.

[Ludovic Courtès]

* 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

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

[Maxim Cournoyer]

Hi,

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

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

LGTM!

-- 
Thanks,
Maxim

[bug#65866] [PATCH 3/8] perform-download: Remove unused one-argument clause.

[Ludovic Courtès]

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

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

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@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

[bug#65866] [PATCH 4/8] daemon: Add “git-download” built-in builder.

[Ludovic Courtès]

From: Ludovic Courtès <ludovic.courtes@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@gnu.org>
+;;; Copyright © 2016-2018, 2020, 2023 Ludovic Courtès <ludo@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@gnu.org>
+   Copyright (C) 2016-2019, 2023 Ludovic Courtès <ludo@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

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

[Maxim Cournoyer]

Hello,

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

> From: Ludovic Courtès <ludovic.courtes@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@gnu.org>
> +;;; Copyright © 2016-2018, 2020, 2023 Ludovic Courtès <ludo@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@gnu.org>
> +   Copyright (C) 2016-2019, 2023 Ludovic Courtès <ludo@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

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

[Ludovic Courtès]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@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’.

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

[Ludovic Courtès]

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

[bug#65866] [PATCH v2 1/8] git-download: Move fallback code to (guix build git).

[Ludovic Courtès]

* 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@gnu.org>
+;;; Copyright © 2014, 2016, 2019, 2023 Ludovic Courtès <ludo@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@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@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

[bug#65866] [PATCH v2 1/8] git-download: Move fallback code to (guix build git).

[Simon Tournier]

Hi Ludo,

On Sat, 23 Sep 2023 at 00:27, Ludovic Courtès <ludo@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

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

[Ludovic Courtès]

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

> On Sat, 23 Sep 2023 at 00:27, Ludovic Courtès <ludo@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’.

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

[Simon Tournier]

Hi,

On Mon, 25 Sep 2023 at 11:24, Ludovic Courtès <ludo@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

[bug#65866] [PATCH v2 2/8] git-download: Honor the ‘GUIX_DOWNLOAD_FALLBACK_TEST’ environment variable.

[Ludovic Courtès]

* 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

[bug#65866] [PATCH v2 3/8] perform-download: Remove unused one-argument clause.

[Ludovic Courtès]

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

[bug#65866] [PATCH v2 4/8] daemon: Add “git-download” built-in builder.

[Ludovic Courtès]

From: Ludovic Courtès <ludovic.courtes@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@gnu.org>
+;;; Copyright © 2016-2018, 2020, 2023 Ludovic Courtès <ludo@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@gnu.org>
+   Copyright (C) 2016-2019, 2023 Ludovic Courtès <ludo@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

[bug#65866] [PATCH v2 5/8] build: Add dependency on Git.

[Ludovic Courtès]

* 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@gnu.org>
+;;; Copyright © 2012-2016, 2018-2019, 2021, 2023 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2017 Caleb Ristvedt <caleb.ristvedt@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

[bug#65866] [PATCH v2 5/8] build: Add dependency on Git.

[Simon Tournier]

Hi Ludo,

On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@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

[bug#65866] Bootstrapping without the daemon and all that

[Ludovic Courtès]

Hi,

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

> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@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’.

[bug#65866] Bootstrapping without the daemon and all that

[Simon Tournier]

Hi Ludo,

On Tue, 26 Sept 2023 at 16:05, Ludovic Courtès <ludo@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

[bug#65866] Bootstrapping without the daemon and all that

[Ludovic Courtès]

Hi,

Simon Tournier <zimon.toutoune@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’.

[bug#65866] Bootstrapping without the daemon and all that

[Simon Tournier]

Hi Ludo,

On Thu, 12 Oct 2023 at 12:54, Ludovic Courtès <ludo@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

[bug#65866] [PATCH v2 6/8] perform-download: Use the ‘git’ command captured at configure time.

[Ludovic Courtès]

* 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

[bug#65866] [PATCH v2 7/8] git-download: Use “builtin:git-download” when available.

[Ludovic Courtès]

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@gnu.org>
+;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Lars-Dominik Braun <lars@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

[bug#65866] [PATCH v2 7/8] git-download: Use “builtin:git-download” when available.

[Simon Tournier]

Hi Ludo,

On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@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

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

[Ludovic Courtès]

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

> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@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’.

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

[Simon Tournier]

Hi,

On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@gnu.org> wrote:
> Simon Tournier <zimon.toutoune@gmail.com> skribis:
>
>> On Sat, 23 Sep 2023 at 00:28, Ludovic Courtès <ludo@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

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

[Simon Tournier]

Re,

On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@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

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

[Maxim Cournoyer]

Hi,

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

> Re,
>
> On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@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

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

[Ludovic Courtès]

Hi,

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

> Re,
>
> On Mon, 25 Sep 2023 at 11:23, Ludovic Courtès <ludo@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’.

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

[Simon Tournier]

On Tue, 26 Sept 2023 at 17:44, Ludovic Courtès <ludo@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

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

[Ludovic Courtès]

Hi,

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

> On Tue, 26 Sept 2023 at 17:44, Ludovic Courtès <ludo@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

[bug#65866] Toward RFC? (was Re: [bug#65866] [PATCH 0/8] Add built-in builder for Git checkouts)

[Simon Tournier]

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@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@gnu.org>
        Wed, 27 Oct 2021 23:22:50 +0200
        id:87cznqb1sl.fsf@inria.fr
        https://lists.gnu.org/archive/html/guix-devel/2021-10
        https://yhetil.org/guix/87cznqb1sl.fsf@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@gnu.org>
        Wed, 01 Mar 2023 17:13:28 +0100
        id:878rgga1qv.fsf@inria.fr
        https://lists.gnu.org/archive/html/guix-devel/2023-03
        https://yhetil.org/guix/878rgga1qv.fsf@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/

[bug#65866] Toward RFC? (was Re: [bug#65866] [PATCH 0/8] Add built-in builder for Git checkouts)

[Ludovic Courtès]

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’.

[bug#65866] [PATCH v2 8/8] tests: Assume ‘git’ is always available.

[Ludovic Courtès]

* 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?)
 \f
 (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

[bug#65866] [PATCH 5/8] build: Add dependency on Git.

[Ludovic Courtès]

* 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@gnu.org>
+;;; Copyright © 2012-2016, 2018-2019, 2021, 2023 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2017 Caleb Ristvedt <caleb.ristvedt@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

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

[Maxim Cournoyer]

Hello,

Ludovic Courtès <ludo@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

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

[Ludovic Courtès]

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

> Ludovic Courtès <ludo@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’.

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

[Maxim Cournoyer]

Hello,

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

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@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

[bug#65866] [PATCH 6/8] perform-git-download: Use the ‘git’ command captured at configure time.

[Ludovic Courtès]

* 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

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

[Maxim Cournoyer]

Ludovic Courtès <ludo@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

[bug#65866] [PATCH 7/8] git-download: Use “builtin:git-download” when available.

[Ludovic Courtès]

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@gnu.org>
+;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2021 Lars-Dominik Braun <lars@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

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

[Maxim Cournoyer]

Hello!

Ludovic Courtès <ludo@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@gnu.org>
> +;;; Copyright © 2012-2015, 2018-2019, 2021, 2023 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2021 Lars-Dominik Braun <lars@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

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

[Ludovic Courtès]

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

> Ludovic Courtès <ludo@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’.

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

[Maxim Cournoyer]

Hello,

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

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@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

[bug#65866] [PATCH 8/8] tests: Assume ‘git’ is always available.

[Ludovic Courtès]

* 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?)
 \f
 (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

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

[Maxim Cournoyer]

Hello,

Ludovic Courtès <ludo@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