Hi Ludo, Ludovic Courtès writes: > Hi Maxim, > > Maxim Cournoyer skribis: > >> Ludovic Courtès 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