[PATCH] guix: utils: Delete temporary output files.

  • Done
  • quality assurance status badge
Details
2 participants
  • Kierin Bell
  • Ludovic Courtès
Owner
unassigned
Submitted by
Kierin Bell
Severity
normal

Debbugs page

Kierin Bell wrote 2 months ago
(address . guix-patches@gnu.org)
87wmewdnq1.fsf@fernseed.me
* guix/build/utils.scm (call-with-temporary-output-file): Delete and
pass procedure the actual temporary file name, not the template name.

Change-Id: Id8e5da55444195fcee91517cf63ec8ebd20942e5
---

This change will trigger *many* rebuilds, I think because
'call-with-temporary-output-file' is used by 'download-to-store'.

This patch fixes a bug that causes: 1) the procedure passed to
'call-with-temporary-output-file' to always be called with the file name
template string "/guix-file.XXXXXX" rather than the name of the created
temporary file; and 2) the temporary file to persist rather than being
deleted.

Unless I'm missing something, Guix is creating temporary files without
deleting them and nobody is noticing.

Also, more importantly, it's a miracle that nothing has broken because
of issue #1. Apparently, in most applications in the Guix code base, the
procedures passed to this function discard their second argument (the
port) and only use the first argument (what should be the file name).
After a quick search, the only exceptions that I could find are some
unit tests and 'call-with-luks-key-file' in (gnu installer parted). I'm
not sure how this hasn't caused issues there.

I haven't had time to test this yet, because it triggers so many
rebuilds.

guix/build/utils.scm | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

Toggle diff (28 lines)
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 94714bf397..dda8fb9d82 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -287,15 +287,16 @@ (define (call-with-temporary-output-file proc)
call."
(let* ((directory (or (getenv "TMPDIR") "/tmp"))
(template (string-append directory "/guix-file.XXXXXX"))
- (out (mkstemp! template)))
+ (out (mkstemp! template))
+ (filename (port-filename out)))
(dynamic-wind
(lambda ()
#t)
(lambda ()
- (proc template out))
+ (proc filename out))
(lambda ()
(false-if-exception (close out))
- (false-if-exception (delete-file template))))))
+ (false-if-exception (delete-file filename))))))
(define (call-with-ascii-input-file file proc)
"Open FILE as an ASCII or binary file, and pass the resulting port to

base-commit: b696658ee8e0655b17f5d26e024956b5148e36d6
--
2.47.1
Kierin Bell wrote 2 months ago
Re: bug#75588: Acknowledgement ([PATCH] guix: utils: Delete temporary output files.)
(address . 75588@debbugs.gnu.org)
87h65zeugv.fsf@fernseed.me
help-debbugs@gnu.org (GNU bug Tracking System) writes:

So this bug should be canceled! It doesn't actually change the behavior
of the function in any way (though it may making it easier to read for
people not familiar with 'mkstemp!').

I did find a corner-case where temporary files created by
'call-with-temporary-output-file' are not deleted like I'd expect. It
involves using 'execl' to call a process from within
'call-with-temporary-output-file', where the temporary file is not
deleted, at least until after it the new process terminates. Though
confusing, this is probably the right behavior.

--
Kierin Bell
Kierin Bell wrote 2 months ago
control message for bug #75588
(address . control@debbugs.gnu.org)
87frljet5v.fsf@fernseed.me
close 75588
quit
Ludovic Courtès wrote 2 months ago
Re: [bug#75588] [PATCH] guix: utils: Delete temporary output files.
(name . Kierin Bell)(address . fernseed@fernseed.me)
87o7075efi.fsf@gnu.org
Hello,

Kierin Bell <fernseed@fernseed.me> skribis:

Toggle quote (9 lines)
> This patch fixes a bug that causes: 1) the procedure passed to
> 'call-with-temporary-output-file' to always be called with the file name
> template string "/guix-file.XXXXXX" rather than the name of the created
> temporary file; and 2) the temporary file to persist rather than being
> deleted.
>
> Unless I'm missing something, Guix is creating temporary files without
> deleting them and nobody is noticing.

[…]

Toggle quote (20 lines)
> --- a/guix/build/utils.scm
> +++ b/guix/build/utils.scm
> @@ -287,15 +287,16 @@ (define (call-with-temporary-output-file proc)
> call."
> (let* ((directory (or (getenv "TMPDIR") "/tmp"))
> (template (string-append directory "/guix-file.XXXXXX"))
> - (out (mkstemp! template)))
> + (out (mkstemp! template))
> + (filename (port-filename out)))
> (dynamic-wind
> (lambda ()
> #t)
> (lambda ()
> - (proc template out))
> + (proc filename out))
> (lambda ()
> (false-if-exception (close out))
> - (false-if-exception (delete-file template))))))
> + (false-if-exception (delete-file filename))))))

AFAICS the current code is fine because ‘template’ is modified by
‘mkstemp!’:

Toggle snippet (7 lines)
scheme@(guile-user)> (define template (string-copy "/tmp/guix-example-XXXXXX"))
scheme@(guile-user)> (mkstemp! template)
$15 = #<input-output: /tmp/guix-example-uZ5z4s 13>
scheme@(guile-user)> template
$16 = "/tmp/guix-example-uZ5z4s"

All good?

Thanks,
Ludo’.
Kierin Bell wrote 2 months ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
87zfjqdcp2.fsf@fernseed.me
Hi,

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

Toggle quote (3 lines)
> AFAICS the current code is fine because ‘template’ is modified by
> ‘mkstemp!’:

Yes, the code is fine --- this was a misunderstanding on my part.

I was doing something unusual that was causing temporary files to
persist, and blamed it on 'call-with-temporary-output-file':

Toggle snippet (4 lines)
(call-with-temporary-output-file (lambda (fn port) ... (execlp "swaymsg"
"swaymsg" "exec" "--" "touch" "foo")))

Sorry for the noise!

Thanks.

--
Kierin Bell
?
Your comment

This issue is archived.

To comment on this conversation send an email to 75588@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 75588
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help