[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] bug#63331: Guile-GnuTLS/Git circular dependency and built-in git checkouts

[Vivien Kraus via Guix-patches via]

Hello!

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

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

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

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

Vivien

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

[Ludovic Courtès]

Hi,

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

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

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

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

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

Ludo’.

[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

[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 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 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

[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

[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 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

[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

[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

[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 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 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

[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

[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

[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 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

[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 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 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