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