Package: guix-patches;
Reported by: Ludovic Courtès <ludo <at> gnu.org>
Date: Fri, 15 Jan 2021 13:17:01 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45891 in the body.
You can then email your comments to 45891 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix-patches <at> gnu.org:bug#45891; Package guix-patches.
(Fri, 15 Jan 2021 13:17:01 GMT) Full text and rfc822 format available.Ludovic Courtès <ludo <at> gnu.org>:guix-patches <at> gnu.org.
(Fri, 15 Jan 2021 13:17:01 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: guix-patches <at> gnu.org Cc: Ludovic Courtès <ludo <at> gnu.org> Subject: [PATCH] packages: 'patch-and-repack' returns a directory when given a directory. Date: Fri, 15 Jan 2021 14:15:48 +0100
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.
* guix/packages.scm (patch-and-repack)[tarxz-name]: Remove 'checkout?' case.
[build](repack): New procedure, with "tar" invocation formerly at the
top level.
If SOURCE is a directory, call 'copy-recursively'; otherwise, call
'repack'.
Change NAME to ORIGINAL-FILE-NAME when it matches 'checkout?'.
---
guix/packages.scm | 65 ++++++++++++++++++++++++++---------------------
1 file changed, 36 insertions(+), 29 deletions(-)
Hi!
This change is a followup to a recent IRC discussion: it makes
‘patch-and-repack’ preserve the “directoriness” of its input.
It conflicts with other changes Maxim posted at
<https://issues.guix.gnu.org/45773>, though that could be addressed.
Thoughts?
Ludo’.
diff --git a/guix/packages.scm b/guix/packages.scm
index 4caaa9cb79..cd2cded9ee 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -1,5 +1,5 @@
;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
;;; Copyright © 2014, 2015, 2017, 2018 Mark H Weaver <mhw <at> netris.org>
;;; Copyright © 2015 Eric Bavier <bavier <at> member.fsf.org>
;;; Copyright © 2016 Alex Kost <alezost <at> gmail.com>
@@ -635,11 +635,9 @@ specifies modules in scope when evaluating SNIPPET."
(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))))
(string-append base
(if (equal? (file-extension base) "tar")
".xz"
@@ -689,6 +687,29 @@ specifies modules in scope when evaluating SNIPPET."
(lambda (name)
(not (member name '("." "..")))))))
+ (define (repack directory output)
+ ;; Write to OUTPUT a compressed tarball containing DIRECTORY.
+ (unless tar-supports-sort?
+ (call-with-output-file ".file_list"
+ (lambda (port)
+ (for-each (lambda (name)
+ (format port "~a~%" name))
+ (find-files directory
+ #:directories? #t
+ #:fail-on-error? #t)))))
+
+ (apply invoke #+(file-append tar "/bin/tar")
+ "cvfa" output
+ ;; Avoid non-determinism in the archive. Set the mtime
+ ;; to 1 as is the case in the store (software like gzip
+ ;; behaves differently when it stumbles upon mtime = 0).
+ "--mtime=@1"
+ "--owner=root:0" "--group=root:0"
+ (if tar-supports-sort?
+ `("--sort=name" ,directory)
+ '("--no-recursion"
+ "--files-from=.file_list"))))
+
;; Encoding/decoding errors shouldn't be silent.
(fluid-set! %default-port-conversion-strategy 'error)
@@ -742,30 +763,16 @@ specifies modules in scope when evaluating SNIPPET."
(chdir "..")
- (unless tar-supports-sort?
- (call-with-output-file ".file_list"
- (lambda (port)
- (for-each (lambda (name)
- (format port "~a~%" name))
- (find-files directory
- #:directories? #t
- #:fail-on-error? #t)))))
- (apply invoke
- (string-append #+tar "/bin/tar")
- "cvfa" #$output
- ;; Avoid non-determinism in the archive. Set the mtime
- ;; to 1 as is the case in the store (software like gzip
- ;; behaves differently when it stumbles upon mtime = 0).
- "--mtime=@1"
- "--owner=root:0"
- "--group=root:0"
- (if tar-supports-sort?
- `("--sort=name"
- ,directory)
- '("--no-recursion"
- "--files-from=.file_list")))))))
+ ;; If SOURCE is a directory (such as a checkout), return a
+ ;; directory. Otherwise create a tarball.
+ (if (file-is-directory? #+source)
+ (copy-recursively directory #$output
+ #:log (%make-void-port "w"))
+ (repack directory #$output))))))
- (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
--
2.30.0
guix-patches <at> gnu.org:bug#45891; Package guix-patches.
(Fri, 15 Jan 2021 22:15:02 GMT) Full text and rfc822 format available.Message #8 received at 45891 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 45891 <at> debbugs.gnu.org Subject: Re: bug#45891: [PATCH] packages: 'patch-and-repack' returns a directory when given a directory. Date: Fri, 15 Jan 2021 17:14:32 -0500
Hello!
Ludovic Courtès <ludo <at> 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? This
would prevent keeping nonfree cruft under /gnu/store until the next
garbage collection run, for those sources that are cleaned up.
> * guix/packages.scm (patch-and-repack)[tarxz-name]: Remove 'checkout?' case.
> [build](repack): New procedure, with "tar" invocation formerly at the
> top level.
> If SOURCE is a directory, call 'copy-recursively'; otherwise, call
> 'repack'.
> Change NAME to ORIGINAL-FILE-NAME when it matches 'checkout?'.
> ---
> guix/packages.scm | 65 ++++++++++++++++++++++++++---------------------
> 1 file changed, 36 insertions(+), 29 deletions(-)
>
> Hi!
>
> This change is a followup to a recent IRC discussion: it makes
> ‘patch-and-repack’ preserve the “directoriness” of its input.
>
> It conflicts with other changes Maxim posted at
> <https://issues.guix.gnu.org/45773>, though that could be addressed.
>
> Thoughts?
>
> Ludo’.
>
> diff --git a/guix/packages.scm b/guix/packages.scm
> index 4caaa9cb79..cd2cded9ee 100644
> --- a/guix/packages.scm
> +++ b/guix/packages.scm
> @@ -1,5 +1,5 @@
> ;;; GNU Guix --- Functional package management for GNU
> -;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo <at> gnu.org>
> +;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
> ;;; Copyright © 2014, 2015, 2017, 2018 Mark H Weaver <mhw <at> netris.org>
> ;;; Copyright © 2015 Eric Bavier <bavier <at> member.fsf.org>
> ;;; Copyright © 2016 Alex Kost <alezost <at> gmail.com>
> @@ -635,11 +635,9 @@ specifies modules in scope when evaluating SNIPPET."
>
> (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? 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.
> (string-append base
> (if (equal? (file-extension base) "tar")
> ".xz"
> @@ -689,6 +687,29 @@ specifies modules in scope when evaluating SNIPPET."
> (lambda (name)
> (not (member name '("." "..")))))))
>
> + (define (repack directory output)
> + ;; Write to OUTPUT a compressed tarball containing DIRECTORY.
> + (unless tar-supports-sort?
> + (call-with-output-file ".file_list"
> + (lambda (port)
> + (for-each (lambda (name)
> + (format port "~a~%" name))
> + (find-files directory
> + #:directories? #t
> + #:fail-on-error? #t)))))
> +
> + (apply invoke #+(file-append tar "/bin/tar")
> + "cvfa" output
> + ;; Avoid non-determinism in the archive. Set the mtime
> + ;; to 1 as is the case in the store (software like gzip
> + ;; behaves differently when it stumbles upon mtime = 0).
> + "--mtime=@1"
> + "--owner=root:0" "--group=root:0"
> + (if tar-supports-sort?
> + `("--sort=name" ,directory)
> + '("--no-recursion"
> + "--files-from=.file_list"))))
> +
> ;; Encoding/decoding errors shouldn't be silent.
> (fluid-set! %default-port-conversion-strategy 'error)
>
> @@ -742,30 +763,16 @@ specifies modules in scope when evaluating SNIPPET."
>
> (chdir "..")
>
> - (unless tar-supports-sort?
> - (call-with-output-file ".file_list"
> - (lambda (port)
> - (for-each (lambda (name)
> - (format port "~a~%" name))
> - (find-files directory
> - #:directories? #t
> - #:fail-on-error? #t)))))
> - (apply invoke
> - (string-append #+tar "/bin/tar")
> - "cvfa" #$output
> - ;; Avoid non-determinism in the archive. Set the mtime
> - ;; to 1 as is the case in the store (software like gzip
> - ;; behaves differently when it stumbles upon mtime = 0).
> - "--mtime=@1"
> - "--owner=root:0"
> - "--group=root:0"
> - (if tar-supports-sort?
> - `("--sort=name"
> - ,directory)
> - '("--no-recursion"
> - "--files-from=.file_list")))))))
> + ;; If SOURCE is a directory (such as a checkout), return a
> + ;; directory. Otherwise create a tarball.
> + (if (file-is-directory? #+source)
> + (copy-recursively directory #$output
> + #:log (%make-void-port "w"))
> + (repack directory #$output))))))
>
> - (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.
I've run that test module like this:
$ make check TESTS=tests/packages.scm SCM_LOG_DRIVER_FLAGS=' --brief=no' VERBOSE=1
with your change and it passed.
Thanks,
Maxim
guix-patches <at> gnu.org:bug#45891; Package guix-patches.
(Sat, 16 Jan 2021 21:30:02 GMT) Full text and rfc822 format available.Message #11 received at 45891 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 45891 <at> debbugs.gnu.org Subject: Re: bug#45891: [PATCH] packages: 'patch-and-repack' returns a directory when given a directory. Date: Sat, 16 Jan 2021 22:28:54 +0100
Hi Maxim, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > Ludovic Courtès <ludo <at> 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. >> (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.) > 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? Thanks, Ludo’.
guix-patches <at> gnu.org:bug#45891; Package guix-patches.
(Mon, 18 Jan 2021 06:25:01 GMT) Full text and rfc822 format available.Message #14 received at 45891 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 45891 <at> debbugs.gnu.org Subject: Re: bug#45891: [PATCH] packages: 'patch-and-repack' returns a directory when given a directory. Date: Mon, 18 Jan 2021 01:24:02 -0500
Hi Ludo, Ludovic Courtès <ludo <at> gnu.org> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > >> Ludovic Courtès <ludo <at> 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
Ludovic Courtès <ludo <at> gnu.org>:Ludovic Courtès <ludo <at> gnu.org>:Message #19 received at 45891-done <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 45891-done <at> debbugs.gnu.org Subject: Re: bug#45891: [PATCH] packages: 'patch-and-repack' returns a directory when given a directory. Date: Mon, 18 Jan 2021 15:56:49 +0100
Hi! Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > 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. Alright. > Feel free to push to core-updates! Done, thanks! Ludo’.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Tue, 16 Feb 2021 12:24:09 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.