[bug#45891] [PATCH] packages: 'patch-and-repack' returns a directory when given a directory.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi Ludo,

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

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Previously, 'patch-and-repack' would always create a tar.xz archive as a
>>> result, even if the input was a directory (a checkout).  This change
>>> reduces gratuitous CPU and storage overhead.
>>
>> I like it!
>>
>> Note that on core-updates, xz compression is relatively fast on modern
>> machines as it can do multi-threading.  About space the savings; could
>> the 'temporary' pristine source be cleared from the store always?
>
> No, it’s not possible—the GC will remove what’s unreachable when it
> eventually runs.

OK.

>
>>>    (define (tarxz-name file-name)
>>>      ;; Return a '.tar.xz' file name based on FILE-NAME.
>>> -    (let ((base (cond ((numeric-extension? file-name)
>>> -                       original-file-name)
>>> -                      ((checkout? file-name)
>>> -                       (string-drop-right file-name 9))
>>> -                      (else (file-sans-extension file-name)))))
>>> +    (let ((base (if (numeric-extension? file-name)
>>> +                    original-file-name
>>> +                    (file-sans-extension file-name))))
>>
>> This is not new code, but I'm wondering what's the purpose of
>> numeric-extension?
>
> It’s for file names like “hello-2.10”, where you wouldn’t want to strip
> “.10”.  (Such file names should be rare, but it’s not impossible.)

Ah! Thanks for explaining.

>> What kind of files does it expect to catch?  Also, what happened to
>> stripping the '-checkout' suffix that used to be done?  It doesn't
>> seem like it will happen anymore.
>
> Stripping “-checkout” is no longer necessary because for these we keep
> the original name.
>
>>> -    (let ((name (tarxz-name original-file-name)))
>>> +    (let ((name (if (checkout? original-file-name)
>>> +                    original-file-name
>>> +                    (tarxz-name original-file-name))))
>>>        (gexp->derivation name build
>>>                          #:graft? #f
>>>                          #:system system
>>
>> Was these cases (tar archive source derivation, directory source
>> derivation) already covered by tests under tests/packages.cm?  How did
>> you otherwise test it?  World rebuilding changes are not fun to test
>> without unit tests.
>
> In this case I rebuilt the world and tested ‘guix build -S’ on a
> git-fetch package with a snippet, but I agree that’s super expensive (I
> tested the handful of commits I recently pushed to ‘core-updates’ at the
> same time.)
>
> There are no unit tests specifically for this procedure, but I think
> we’ll quickly find out if it doesn’t behave as intended.
>
> WDYT?

LGTM.  Feel free to push!  About my related change that we thought was
conflicting with this one; it at least applies on top of your change,
and only one test fails currently, I'm working on a fix.

Feel free to push to core-updates!

Thank you,

Maxim