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

Previous Next

Full log

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

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

Hello,

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

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

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

-- 
Thanks,
Maxim

Previous Next

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