bug#65666: Lockfiles break package-vc-install-from-checkout

bug#65666: Lockfiles break package-vc-install-from-checkout

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

To reproduce: clone a repo, ensure that create-lockfiles is non-nil,
edit a source file in the repo but do not save it, run
package-vc-install-from-checkout on the repo.

Backtrace:

Debugger entered--Lisp error: (file-missing "/home/joseph/.emacs.d/elpa/hyperdrive/.#hyperdrive.el")
  comp-el-to-eln-filename("/home/joseph/.emacs.d/elpa/hyperdrive/.#hyperdrive.el")
  (comp-clean-up-stale-eln (comp-el-to-eln-filename file))
  (while (consp --cl-var--) (setq file (car --cl-var--)) (comp-clean-up-stale-eln (comp-el-to-eln-filename file)) (setq --cl-var-- (cdr --cl-var--)))
  (let* ((--cl-var-- (directory-files-recursively dir "\\.el\\'")) (file nil)) (while (consp --cl-var--) (setq file (car --cl-var--)) (comp-clean-up-stale-eln (comp-el-to-eln-filename file)) (setq --cl-var-- (cdr --cl-var--))) nil)
  (progn (let* ((--cl-var-- (directory-files-recursively dir "\\.el\\'")) (file nil)) (while (consp --cl-var--) (setq file (car --cl-var--)) (comp-clean-up-stale-eln (comp-el-to-eln-filename file)) (setq --cl-var-- (cdr --cl-var--))) nil))
  (if (featurep 'native-compile) (progn (let* ((--cl-var-- (directory-files-recursively dir "\\.el\\'")) (file nil)) (while (consp --cl-var--) (setq file (car --cl-var--)) (comp-clean-up-stale-eln (comp-el-to-eln-filename file)) (setq --cl-var-- (cdr --cl-var--))) nil)))
  package--delete-directory("/home/joseph/.emacs.d/elpa/hyperdrive")
  package-vc-install-from-checkout("~/.local/src/hyperdrive.el/" "hyperdrive")
  funcall-interactively(package-vc-install-from-checkout "~/.local/src/hyperdrive.el/" "hyperdrive")
  command-execute(package-vc-install-from-checkout record)
  execute-extended-command(nil "package-vc-install-from-checkout" nil)
  funcall-interactively(execute-extended-command nil "package-vc-install-from-checkout" nil)
  command-execute(execute-extended-command)

bug#65666: Lockfiles break package-vc-install-from-checkout

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>, Adam Porter <adam.porter@47ap.net>
> Date: Thu, 31 Aug 2023 14:47:48 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> To reproduce: clone a repo, ensure that create-lockfiles is non-nil,
> edit a source file in the repo but do not save it, run
> package-vc-install-from-checkout on the repo.
> 
> Backtrace:
> 
> Debugger entered--Lisp error: (file-missing "/home/joseph/.emacs.d/elpa/hyperdrive/.#hyperdrive.el")
>   comp-el-to-eln-filename("/home/joseph/.emacs.d/elpa/hyperdrive/.#hyperdrive.el")

We should not try to native-compile lock files, obviously.  Some code
naïvely uses "*.el" to find all the Lisp files; it should filter out
lock files.

P.S. Please always say in what version of Emacs you see the problem
you report.  Bonus points for including all of the information
collected by report-emacs-bug about your system and Emacs
configurations.

bug#65666: Lockfiles break package-vc-install-from-checkout

[joseph--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:
> We should not try to native-compile lock files, obviously.  Some code
> naïvely uses "*.el" to find all the Lisp files; it should filter out
> lock files.

Please see attached patch.

> P.S. Please always say in what version of Emacs you see the problem
> you report.  Bonus points for including all of the information
> collected by report-emacs-bug about your system and Emacs
> configurations.

Thanks - I'm on 29.0.92. I'll include that information next time.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-native-compile-lock-files.patch --]
[-- Type: text/x-diff; name="0001-Don-t-native-compile-lock-files.patch", Size: 912 bytes --]

From 7b38b0bfd8c9da08daf734f6d0062d31dd54b947 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Thu, 31 Aug 2023 23:11:53 -0700
Subject: [PATCH] Don't native compile lock files

* lisp/emacs-lisp/package.el (package--delete-directory):
Check that each file exists before compiling.
---
 lisp/emacs-lisp/package.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index e1172d69bf0..52a538e0627 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2485,6 +2485,7 @@ compiled."
   (when (featurep 'native-compile)
     (cl-loop
      for file in (directory-files-recursively dir "\\.el\\'")
+     when (file-exists-p file)
      do (comp-clean-up-stale-eln (comp-el-to-eln-filename file))))
   (if (file-symlink-p (directory-file-name dir))
       (delete-file (directory-file-name dir))
-- 
2.41.0

bug#65666: Lockfiles break package-vc-install-from-checkout

[Philip Kaludercic]

joseph@breatheoutbreathe.in writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>> We should not try to native-compile lock files, obviously.  Some code
>> naïvely uses "*.el" to find all the Lisp files; it should filter out
>> lock files.
>
> Please see attached patch.
>
>> P.S. Please always say in what version of Emacs you see the problem
>> you report.  Bonus points for including all of the information
>> collected by report-emacs-bug about your system and Emacs
>> configurations.
>
> Thanks - I'm on 29.0.92. I'll include that information next time.
>
> From 7b38b0bfd8c9da08daf734f6d0062d31dd54b947 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Thu, 31 Aug 2023 23:11:53 -0700
> Subject: [PATCH] Don't native compile lock files
>
> * lisp/emacs-lisp/package.el (package--delete-directory):
> Check that each file exists before compiling.
> ---
>  lisp/emacs-lisp/package.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index e1172d69bf0..52a538e0627 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2485,6 +2485,7 @@ compiled."
>    (when (featurep 'native-compile)
>      (cl-loop
>       for file in (directory-files-recursively dir "\\.el\\'")
> +     when (file-exists-p file)
>       do (comp-clean-up-stale-eln (comp-el-to-eln-filename file))))
>    (if (file-symlink-p (directory-file-name dir))
>        (delete-file (directory-file-name dir))

LGTM, but I wonder if there is a better way to detect lockfiles
specifically?  If not, I can imagine that just using `file-exists-p'
would a too broad check, in the sense that it could make it difficult to
find other issues?

bug#65666: Lockfiles break package-vc-install-from-checkout

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> LGTM, but I wonder if there is a better way to detect lockfiles
> specifically?  If not, I can imagine that just using `file-exists-p'
> would a too broad check, in the sense that it could make it difficult to
> find other issues?

We could use a regex like

(unless (string-match-p (rx string-start ".#") file))

bug#65666: Lockfiles break package-vc-install-from-checkout

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: Eli Zaretskii <eliz@gnu.org>, 65666@debbugs.gnu.org, adam.porter@47ap.net
> Date: Fri, 01 Sep 2023 16:43:27 -0700
> 
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
> > LGTM, but I wonder if there is a better way to detect lockfiles
> > specifically?  If not, I can imagine that just using `file-exists-p'
> > would a too broad check, in the sense that it could make it difficult to
> > find other issues?
> 
> We could use a regex like
> 
> (unless (string-match-p (rx string-start ".#") file))

Or make the REGEXP argument to directory-files-recursively more
specific, to reject lock files.

But yes, the use of file-exists-p is not TRT, IMO.

bug#65666: Lockfiles break package-vc-install-from-checkout

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Eli Zaretskii <eliz@gnu.org> writes:
> Or make the REGEXP argument to directory-files-recursively more
> specific, to reject lock files.
>
> But yes, the use of file-exists-p is not TRT, IMO.

See patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-native-compile-lock-files.patch --]
[-- Type: text/x-diff, Size: 1169 bytes --]

From 31ea77e547e333c8e216d759c1c26ad1fe6c4df0 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 10:14:22 -0700
Subject: [PATCH] Don't native compile lock files

* lisp/emacs-lisp/package.el (package--delete-directory):
Exclude lock files in regex.
---
 lisp/emacs-lisp/package.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index e1172d69bf0..8bc7936a5bf 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2484,7 +2484,9 @@ Clean-up the corresponding .eln files if Emacs is native
 compiled."
   (when (featurep 'native-compile)
     (cl-loop
-     for file in (directory-files-recursively dir "\\.el\\'")
+     for file in (directory-files-recursively dir
+                                              ;; Exclude lockfiles
+                                              (rx bos (or (and "." (not "#")) (not ".")) (* nonl) ".el" eos))
      do (comp-clean-up-stale-eln (comp-el-to-eln-filename file))))
   (if (file-symlink-p (directory-file-name dir))
       (delete-file (directory-file-name dir))
-- 
2.41.0

bug#65666: Lockfiles break package-vc-install-from-checkout

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: philipk@posteo.net, 65666@debbugs.gnu.org, adam.porter@47ap.net
> Date: Sat, 02 Sep 2023 10:15:48 -0700
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > Or make the REGEXP argument to directory-files-recursively more
> > specific, to reject lock files.
> >
> > But yes, the use of file-exists-p is not TRT, IMO.
> 
> See patch.

Thanks, this LGTM.

bug#65666: Lockfiles break package-vc-install-from-checkout

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: philipk@posteo.net, 65666@debbugs.gnu.org, adam.porter@47ap.net
>> Date: Sat, 02 Sep 2023 10:15:48 -0700
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> > Or make the REGEXP argument to directory-files-recursively more
>> > specific, to reject lock files.
>> >
>> > But yes, the use of file-exists-p is not TRT, IMO.
>> 
>> See patch.
>
> Thanks, this LGTM.

Agree, I have pushed the change to master.