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

Previous Next

Full log

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

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

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

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

Ah, let me explain…

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

[...]

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

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

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

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

I hope that makes sense!

Ludo’.

Previous Next

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