GNU bug report logs - #32126
call-with-temporary-directory rarely cleans up after itself

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32126 in the body.
You can then email your comments to 32126 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Leo Famulari <leo <at> famulari.name>
To: bug-guix <at> gnu.org
Subject: call-with-temporary-directory rarely cleans up after itself
Date: Wed, 11 Jul 2018 14:59:37 -0400

[Message part 1 (text/plain, inline)]

While testing something, I noticed that temporary directories created
with ((guix utils) call-with-temporary-directory) were not being
deleted.

This procedure is documented to delete the directories after execution:

"Call PROC with a name of a temporary directory; close the directory and
delete it when leaving the dynamic extent of this call."

It uses rmdir, which is documented as follows: "Remove the existing
directory named by path. The directory must be empty for this to
succeed." [0]

I think this is a case where one expects the directory to be deleted as
with `rm -rf`, regardless of whether or not it is empty.

Should we alter the call-with-temporary-directory procedure to use
((guix build utils) delete-file-recursively)?

[0]
https://www.gnu.org/software/guile/manual/html_node/File-System.html#index-rmdir

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 32126 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Leo Famulari <leo <at> famulari.name>
Cc: 32126 <at> debbugs.gnu.org
Subject: Re: bug#32126: call-with-temporary-directory rarely cleans up after
 itself
Date: Thu, 12 Jul 2018 17:37:18 +0200

Hi Leo,

Leo Famulari <leo <at> famulari.name> skribis:

> Should we alter the call-with-temporary-directory procedure to use
> ((guix build utils) delete-file-recursively)?

Yes, definitely.  Note that you can alter (guix utils) without fear of a
full rebuild, so you can go ahead and do that in ‘master’.

Thanks,
Ludo’.

Message #11 received at 32126 <at> debbugs.gnu.org (full text, mbox):

From: Leo Famulari <leo <at> famulari.name>
To: 32126 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: RE: call-with-temporary-directory rarely cleans up after itself
Date: Thu, 12 Jul 2018 20:51:36 -0400

[Message part 1 (text/plain, inline)]

Here's a patch that copies the ((guix build utils)
delete-file-recursively) procedure and uses it in ((guix utils)
call-with-temporary-directory).

However, with the patch there is an error in the test 'gexp->script
#:module-path':

------
actual-value: #f
actual-error:
+ (system-error
+   "lstat"
+   "~A: ~S"
+   ("No such file or directory"
+    "/tmp/guix-directory.6CrC8B/guix/base32.scm")
+   (2))
result: FAIL
------

Continuing to investigate...

[0001-utils-Really-clean-up-temporary-directories.patch (text/plain, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 32126 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Leo Famulari <leo <at> famulari.name>
Cc: 32126 <at> debbugs.gnu.org
Subject: Re: call-with-temporary-directory rarely cleans up after itself
Date: Fri, 13 Jul 2018 10:27:35 +0200

[Message part 1 (text/plain, inline)]

Hello,

Leo Famulari <leo <at> famulari.name> skribis:

> However, with the patch there is an error in the test 'gexp->script
> #:module-path':
>
> ------
> actual-value: #f
> actual-error:
> + (system-error
> +   "lstat"
> +   "~A: ~S"
> +   ("No such file or directory"
> +    "/tmp/guix-directory.6CrC8B/guix/base32.scm")
> +   (2))
> result: FAIL
> ------

Funny.  That test turned out to work thanks to the brokenness of
‘call-with-temporary-directory’: since it’s a monadic return, the actual
code was executed after we’d left the ‘call-with-temporary-directory’
extent, yet it expected to be able to access files from that temporary
directory.

This change fixes it:

[Message part 2 (text/x-patch, inline)]

diff --git a/tests/gexp.scm b/tests/gexp.scm
index 83fe81154..31c7ce22f 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -948,7 +948,7 @@
       (return (and (zero? (close-pipe pipe))
                    (= (expt n 2) (string->number str)))))))
 
-(test-assertm "gexp->script #:module-path"
+(test-assert "gexp->script #:module-path"
   (call-with-temporary-directory
    (lambda (directory)
      (define str
@@ -961,23 +961,24 @@
                         (define-public %fake! ,str))
                 port)))
 
-     (mlet* %store-monad ((exp -> (with-imported-modules '((guix base32))
-                                    (gexp (begin
-                                            (use-modules (guix base32))
-                                            (write (list %load-path
-                                                         %fake!))))))
-                          (drv    (gexp->script "guile-thing" exp
-                                                #:guile %bootstrap-guile
-                                                #:module-path (list directory)))
-                          (out -> (derivation->output-path drv))
-                          (done   (built-derivations (list drv))))
-       (let* ((pipe  (open-input-pipe out))
-              (data  (read pipe)))
-         (return (and (zero? (close-pipe pipe))
-                      (match data
-                        ((load-path str*)
-                         (and (string=? str* str)
-                              (not (member directory load-path))))))))))))
+     (run-with-store %store
+       (mlet* %store-monad ((exp -> (with-imported-modules '((guix base32))
+                                      (gexp (begin
+                                              (use-modules (guix base32))
+                                              (write (list %load-path
+                                                           %fake!))))))
+                            (drv    (gexp->script "guile-thing" exp
+                                                  #:guile %bootstrap-guile
+                                                  #:module-path (list directory)))
+                            (out -> (derivation->output-path drv))
+                            (done   (built-derivations (list drv))))
+         (let* ((pipe  (open-input-pipe out))
+                (data  (read pipe)))
+           (return (and (zero? (close-pipe pipe))
+                        (match data
+                          ((load-path str*)
+                           (and (string=? str* str)
+                                (not (member directory load-path)))))))))))))
 
 (test-assertm "program-file"
   (let* ((n      (random (expt 2 50)))

[Message part 3 (text/plain, inline)]

> From e3181f30ca0711e79aab9d71d798344dfb4636b5 Mon Sep 17 00:00:00 2001
> From: Leo Famulari <leo <at> famulari.name>
> Date: Wed, 11 Jul 2018 20:24:29 -0400
> Subject: [PATCH] utils: Really clean up temporary directories.
>
> * guix/utils.scm (delete-file-recursively): New variable.
> (call-with-temporary-directory): Use DELETE-FILE-RECURSIVELY instead of
> RMDIR.

Instead of duplicating ‘delete-file-recursively’, you can take it
directly from (guix build utils).  There’s already a #:use-module clause
at the top.

Thanks,
Ludo’.

Message #19 received at 32126-done <at> debbugs.gnu.org (full text, mbox):

From: Leo Famulari <leo <at> famulari.name>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 32126-done <at> debbugs.gnu.org
Subject: Re: bug#32126: call-with-temporary-directory rarely cleans up after
 itself
Date: Fri, 13 Jul 2018 17:34:30 -0400

[Message part 1 (text/plain, inline)]

On Thu, Jul 12, 2018 at 05:37:18PM +0200, Ludovic Courtès wrote:
> Hi Leo,
> 
> Leo Famulari <leo <at> famulari.name> skribis:
> 
> > Should we alter the call-with-temporary-directory procedure to use
> > ((guix build utils) delete-file-recursively)?
> 
> Yes, definitely.  Note that you can alter (guix utils) without fear of a
> full rebuild, so you can go ahead and do that in ‘master’.

Great, pushed as 27f7cbc91d1963118e44b14d04fcc669c9618176.

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.