Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Stefan Kangas]

Juanma Barranquero <lekktu@gmail.com> writes:

> branch: master
> commit 2772ebe3667f28cefd0d6134204ce6a3c7a8c323
> Author: Juanma Barranquero <lekktu@gmail.com>
> Commit: Juanma Barranquero <lekktu@gmail.com>
>
>     Do not prune native-compiled system directories (bug#59658)
>
>     * lisp/emacs-lisp/comp.el (native-compile-prune-cache):
>     Skip last directory in `native-comp-eln-load-path'.

It seems like this leads to a failing test:

Running 3 tests (2022-11-30 18:26:07+0100, selector ‘(not (or (tag
:expensive-test) (tag :unstable)))’)
Cache cleared
Test test-native-compile-prune-cache backtrace:
  signal(ert-test-failed (((should-not (file-directory-p c2)) :form (f
  ert-fail(((should-not (file-directory-p c2)) :form (file-directory-p
  #f(compiled-function () #<bytecode -0x8ca32608ded3464>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name test-native-compile-prune-cache :docu
  ert-run-or-rerun-test(#s(ert--stats :selector (not (or (tag :expensi
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable))) #f(co
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable)))
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/comp-test
  command-line()
  normal-top-level()
Test test-native-compile-prune-cache condition:
    (ert-test-failed
     ((should-not
       (file-directory-p c2))
      :form
      (file-directory-p "/tmp/emacs-test-MHYUZK-comp/eln-cache/29.0.50-old")
      :value t))
   FAILED  1/3  test-native-compile-prune-cache (0.003363 sec) at
lisp/emacs-lisp/comp-tests.el:46
Cache cleared
   passed  2/3  test-native-compile-prune-cache/delete-only-eln (0.002607 sec)
Cache cleared
   passed  3/3
test-native-compile-prune-cache/dont-delete-in-parent-of-cache
(0.002566 sec)

Ran 3 tests, 2 results as expected, 1 unexpected (2022-11-30
18:26:07+0100, 0.132447 sec)

1 unexpected results:
   FAILED  test-native-compile-prune-cache

make[3]: *** [Makefile:174: lisp/emacs-lisp/comp-tests.log] Error 1

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

Yes, I see. We'll have to decide where's the error, or what's the intended
behavior.

test-native-compile-prune-cache sets native-comp-eln-load-path to a single
directory and then tests whether native-compile-prune-cache can delete a
stale cache dir inside it (and keep a non-stale one).

It is obviously going to fail with my fix, because with it installed,
native-compile-prune-cache is never going to delete anything in the last
(or, in this case, unique) directory in native-comp-eln-load-path, because,
as the docstring of native-comp-eln-load-path says, "[t]he last directory
of this list is assumed to be the system one" (i.e, the one containing the
preloaded/ subdirectory and its content).

So, alternatives:

1.- We fix the test so it sets native-comp-eln-load-path to a list of two
directories, and checks that the stale subdir is deleted in the first, and
nothing is deleted in the second ("system") one.

2.- We remove my change and allow that native-compile-prune-cache sometimes
deletes files in the "system" eln cache, but Eli argued that we don't know
if the user will have write access to it, and we're not sure we *want* to
allow native-compile-prune-cache to delete files there...

3.- Or we find a way to reliably decide whether a cache directory is, in
fact, the "system" one (which we can currently do, heuristically, because
it contains preloaded/), and fix *both* my patch and the test so it only
considers untouchable those cache directories that really are "system" ones.

[-- Attachment #2: Type: text/html, Size: 1915 bytes --]

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Stefan Kangas]

Juanma Barranquero <lekktu@gmail.com> writes:

> Yes, I see. We'll have to decide where's the error, or what's the intended
> behavior.

The point of the command is basically to free some disk space by getting
rid of (presumably unused) files from ~/.emacs.d/eln-cache.

> 1.- We fix the test so it sets native-comp-eln-load-path to a list of two
> directories, and checks that the stale subdir is deleted in the first, and
> nothing is deleted in the second ("system") one.

I don't really have an opinion on the rest, so just fixing the test
sounds like an okay outcome from my perspective.

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On Thu, Dec 1, 2022 at 4:49 AM Stefan Kangas <stefankangas@gmail.com> wrote:

> The point of the command is basically to free some disk space by getting
> rid of (presumably unused) files from ~/.emacs.d/eln-cache.

That's why I reported and fixed a bug: it was removing files from
[installation-directory]/native-lisp/VERSION-xxxxxxxx

> I don't really have an opinion on the rest, so just fixing the test
> sounds like an okay outcome from my perspective.

Ok. I'll do it soonish (I hope).

[-- Attachment #2: Type: text/html, Size: 758 bytes --]

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Wed, 30 Nov 2022 19:49:25 -0800
> Cc: emacs-devel@gnu.org
> 
> Juanma Barranquero <lekktu@gmail.com> writes:
> 
> > Yes, I see. We'll have to decide where's the error, or what's the intended
> > behavior.
> 
> The point of the command is basically to free some disk space by getting
> rid of (presumably unused) files from ~/.emacs.d/eln-cache.
> 
> > 1.- We fix the test so it sets native-comp-eln-load-path to a list of two
> > directories, and checks that the stale subdir is deleted in the first, and
> > nothing is deleted in the second ("system") one.
> 
> I don't really have an opinion on the rest, so just fixing the test
> sounds like an okay outcome from my perspective.

I agree that we should fix the test, not what the command does.

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 44 bytes --]

I'll install this if everyone's ok with it.

[-- Attachment #1.2: Type: text/html, Size: 143 bytes --]

[-- Attachment #2: comp-tests.patch --]
[-- Type: application/octet-stream, Size: 3871 bytes --]

diff --git i/test/lisp/emacs-lisp/comp-tests.el w/test/lisp/emacs-lisp/comp-tests.el
index 082b641fe3..418c729694 100644
--- i/test/lisp/emacs-lisp/comp-tests.el
+++ w/test/lisp/emacs-lisp/comp-tests.el
@@ -32,15 +32,19 @@ with-test-native-compile-prune-cache
   (declare (indent 0) (debug t))
   `(ert-with-temp-directory testdir
-     (setq testdir (expand-file-name "eln-cache" testdir))
-     (make-directory testdir)
-     (let* ((c1 (expand-file-name "29.0.50-cur" testdir))
-            (c2 (expand-file-name "29.0.50-old" testdir))
-            (native-comp-eln-load-path (list testdir))
-            (comp-native-version-dir "29.0.50-cur"))
-       (dolist (d (list c1 c2))
-         (make-directory d)
-         (with-temp-file (expand-file-name "some.eln" d) (insert "foo"))
-         (with-temp-file (expand-file-name "some.eln.tmp" d) (insert "foo")))
-       ,@body)))
+     (let ((usr-cache (expand-file-name "eln-usr-cache" testdir))
+	   (sys-cache (expand-file-name "eln-sys-cache" testdir)))
+       (make-directory usr-cache)
+       (make-directory sys-cache)
+       (let* ((c1 (expand-file-name "29.0.50-cur" usr-cache))
+              (c2 (expand-file-name "29.0.50-old" usr-cache))
+	      (s1 (expand-file-name "29.0.50-cur" sys-cache))
+	      (s2 (expand-file-name "preloaded" s1))
+              (native-comp-eln-load-path (list usr-cache sys-cache))
+              (comp-native-version-dir "29.0.50-cur"))
+	 (dolist (d (list c1 c2 s1 s2))
+           (make-directory d)
+           (with-temp-file (expand-file-name "some.eln" d) (insert "foo"))
+           (with-temp-file (expand-file-name "some.eln.tmp" d) (insert "foo")))
+	 ,@body))))
 
 (ert-deftest test-native-compile-prune-cache ()
@@ -48,7 +52,8 @@ test-native-compile-prune-cache
   (with-test-native-compile-prune-cache
     (native-compile-prune-cache)
-    (should (file-directory-p c1))
-    (should (file-regular-p (expand-file-name "some.eln" c1)))
-    (should (file-regular-p (expand-file-name "some.eln.tmp" c1)))
+    (dolist (d (list c1 s1 s2))
+      (should (file-directory-p d))
+      (should (file-regular-p (expand-file-name "some.eln" d)))
+      (should (file-regular-p (expand-file-name "some.eln.tmp" d))))
     (should-not (file-directory-p c2))
     (should-not (file-regular-p (expand-file-name "some.eln" c2)))
@@ -58,20 +63,22 @@ test-native-compile-prune-cache/delete-only-eln
   (skip-unless (featurep 'native-compile))
   (with-test-native-compile-prune-cache
-    (with-temp-file (expand-file-name "keep1.txt" c1) (insert "foo"))
-    (with-temp-file (expand-file-name "keep2.txt" c2) (insert "foo"))
+    (dolist (d (list c1 c2 s1 s2))
+      (with-temp-file (expand-file-name "keep.txt" d) (insert "foo")))
     (native-compile-prune-cache)
-    (should (file-regular-p (expand-file-name "keep1.txt" c1)))
-    (should (file-regular-p (expand-file-name "keep2.txt" c2)))))
+    (dolist (d (list c1 c2 s1 s2))
+      (should (file-regular-p (expand-file-name "keep.txt" d))))))
 
 (ert-deftest test-native-compile-prune-cache/dont-delete-in-parent-of-cache ()
   (skip-unless (featurep 'native-compile))
   (with-test-native-compile-prune-cache
-    (let ((f1 (expand-file-name "../some.eln" testdir))
-          (f2 (expand-file-name "some.eln" testdir)))
-      (with-temp-file f1 (insert "foo"))
-      (with-temp-file f2 (insert "foo"))
+    (let ((f1 (expand-file-name "../some.eln" usr-cache))
+          (f2 (expand-file-name "some.eln" usr-cache))
+	  (f3 (expand-file-name "../some.eln" sys-cache))
+	  (f4 (expand-file-name "some.eln" sys-cache)))
+      (dolist (f (list f1 f2 f3 f4))
+	(with-temp-file f (insert "foo")))
       (native-compile-prune-cache)
-      (should (file-regular-p f1))
-      (should (file-regular-p f2)))))
+      (dolist (f (list f1 f2 f3 f4))
+	(should (file-regular-p f))))))
 
 ;;; comp-tests.el ends here

Re: master 2772ebe366: Do not prune native-compiled system directories (bug#59658)

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 33 bytes --]

I've committed the test changes.

[-- Attachment #2: Type: text/html, Size: 130 bytes --]