[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#45891] [PATCH] packages: 'patch-and-repack' returns a directory whe
From: |
Maxim Cournoyer |
Subject: |
[bug#45891] [PATCH] packages: 'patch-and-repack' returns a directory when given a directory. |
Date: |
Mon, 18 Jan 2021 01:24:02 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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