bug#7756: 24.0.50; enhancements to package.el

bug#7756: 24.0.50; enhancements to package.el

[emacs18]

I would like to suggest the following two sets of changes.
The reason for the changes are explained in the diffs.

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el	2010-11-10 21:35:06 +0000
+++ lisp/emacs-lisp/package.el	2010-12-29 18:38:08 +0000
@@ -338,9 +338,14 @@
 	 (pkg-file (expand-file-name
 		    (concat (package-strip-version package) "-pkg")
 		    pkg-dir)))
-    (when (and (file-directory-p pkg-dir)
-	       (file-exists-p (concat pkg-file ".el")))
-      (load pkg-file nil t))))
+    ;; When one is creating a package and testing it out, it is easy
+    ;; to forget to add the -pkg.el file.  When that happens, it would
+    ;; be useful to provide feedback to the user rather than silently
+    ;; failing.  That is what (error ...) below is for.
+    (when (file-directory-p pkg-dir)
+      (if (file-exists-p (concat pkg-file ".el"))
+          (load pkg-file nil t)
+        (error "'%s' file is missing!")))))
 
 (defun package-load-all-descriptors ()
   "Load descriptors for installed Emacs Lisp packages.
@@ -569,8 +574,16 @@
 (defun package-unpack (name version)
   (let ((pkg-dir (expand-file-name (concat (symbol-name name) "-" version)
 				   package-user-dir)))
+    ;; Delete the package directory if it exists already.  This may
+    ;; not be useful to the end users.  However this is extremely
+    ;; important for package developers as they experiment with which
+    ;; files to include in a package.  If a file is removed from one
+    ;; iteration to the next, then the presence of the unwanted elisp
+    ;; file could cause problems by polluting the generated autoload
+    ;; file.
+    (if (file-directory-p pkg-dir)
+        (delete-directory pkg-dir 'recursive))
     (make-directory package-user-dir t)
-    ;; FIXME: should we delete PKG-DIR if it exists?
     (let* ((default-directory (file-name-as-directory package-user-dir)))
       (package-untar-buffer)
       (package-generate-autoloads (symbol-name name) pkg-dir)

bug#7756: 24.0.50; enhancements to package.el

[Stefan Monnier]

> @@ -338,9 +338,14 @@
>  	 (pkg-file (expand-file-name
>  		    (concat (package-strip-version package) "-pkg")
>  		    pkg-dir)))
> -    (when (and (file-directory-p pkg-dir)
> -	       (file-exists-p (concat pkg-file ".el")))
> -      (load pkg-file nil t))))
> +    ;; When one is creating a package and testing it out, it is easy
> +    ;; to forget to add the -pkg.el file.  When that happens, it would
> +    ;; be useful to provide feedback to the user rather than silently
> +    ;; failing.  That is what (error ...) below is for.
> +    (when (file-directory-p pkg-dir)
> +      (if (file-exists-p (concat pkg-file ".el"))
> +          (load pkg-file nil t)
> +        (error "'%s' file is missing!")))))

I agree with the intention, but I wonder: why do we pass the `noerror'
parameter to `load' in the first place?

> @@ -569,8 +574,16 @@
>  (defun package-unpack (name version)
>    (let ((pkg-dir (expand-file-name (concat (symbol-name name) "-" version)
>  				   package-user-dir)))
> +    ;; Delete the package directory if it exists already.  This may
> +    ;; not be useful to the end users.  However this is extremely
> +    ;; important for package developers as they experiment with which
> +    ;; files to include in a package.  If a file is removed from one
> +    ;; iteration to the next, then the presence of the unwanted elisp
> +    ;; file could cause problems by polluting the generated autoload
> +    ;; file.
> +    (if (file-directory-p pkg-dir)
> +        (delete-directory pkg-dir 'recursive))

Here, I also agree with the intention, but I'm a little uneasy blindly
removing a whole subdirectory without warning: I'd add a confirmation prompt.


        Stefan

bug#7756: 24.0.50; enhancements to package.el

[Richard Kim]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> @@ -338,9 +338,14 @@
>>  	 (pkg-file (expand-file-name
>>  		    (concat (package-strip-version package) "-pkg")
>>  		    pkg-dir)))
>> -    (when (and (file-directory-p pkg-dir)
>> -	       (file-exists-p (concat pkg-file ".el")))
>> -      (load pkg-file nil t))))
>> +    ;; When one is creating a package and testing it out, it is easy
>> +    ;; to forget to add the -pkg.el file.  When that happens, it would
>> +    ;; be useful to provide feedback to the user rather than silently
>> +    ;; failing.  That is what (error ...) below is for.
>> +    (when (file-directory-p pkg-dir)
>> +      (if (file-exists-p (concat pkg-file ".el"))
>> +          (load pkg-file nil t)
>> +        (error "'%s' file is missing!")))))
>
> I agree with the intention, but I wonder: why do we pass the `noerror'
> parameter to `load' in the first place?

Hi Stefan,

Thanks for looking into this.
I was focused in adding the error message and neglected to re-examine
the arguments to `load', i.e., I just kept the existing code.  I suppose
it was used before my proposed change to prevent signalling error if the
file was not found.  With the proposed change, since we check for the
existence of the file prior to calling `load', I suppose it is
appropriate to no longer pass `noerror'.  Good catch.  Also I'm sure you
have noticed that I forgot to add `pkg-file' argument to (error ...)
expression above.

>> @@ -569,8 +574,16 @@
>>  (defun package-unpack (name version)
>>    (let ((pkg-dir (expand-file-name (concat (symbol-name name) "-" version)
>>  				   package-user-dir)))
>> +    ;; Delete the package directory if it exists already.  This may
>> +    ;; not be useful to the end users.  However this is extremely
>> +    ;; important for package developers as they experiment with which
>> +    ;; files to include in a package.  If a file is removed from one
>> +    ;; iteration to the next, then the presence of the unwanted elisp
>> +    ;; file could cause problems by polluting the generated autoload
>> +    ;; file.
>> +    (if (file-directory-p pkg-dir)
>> +        (delete-directory pkg-dir 'recursive))
>
> Here, I also agree with the intention, but I'm a little uneasy blindly
> removing a whole subdirectory without warning: I'd add a confirmation prompt.

I agree with your concern.  A confirmation prompt seems to be a good
idea. 

bug#7756: 24.0.50; enhancements to package.el

[Stefan Monnier]

>>> @@ -338,9 +338,14 @@
>>> (pkg-file (expand-file-name
>>> (concat (package-strip-version package) "-pkg")
>>> pkg-dir)))
>>> -    (when (and (file-directory-p pkg-dir)
>>> -	       (file-exists-p (concat pkg-file ".el")))
>>> -      (load pkg-file nil t))))
>>> +    ;; When one is creating a package and testing it out, it is easy
>>> +    ;; to forget to add the -pkg.el file.  When that happens, it would
>>> +    ;; be useful to provide feedback to the user rather than silently
>>> +    ;; failing.  That is what (error ...) below is for.
>>> +    (when (file-directory-p pkg-dir)
>>> +      (if (file-exists-p (concat pkg-file ".el"))
>>> +          (load pkg-file nil t)
>>> +        (error "'%s' file is missing!")))))
>> 
>> I agree with the intention, but I wonder: why do we pass the `noerror'
>> parameter to `load' in the first place?

> Thanks for looking into this.
> I was focused in adding the error message and neglected to re-examine
> the arguments to `load', i.e., I just kept the existing code.  I suppose
> it was used before my proposed change to prevent signalling error if the
> file was not found.  With the proposed change, since we check for the
> existence of the file prior to calling `load', I suppose it is
> appropriate to no longer pass `noerror'.  Good catch.  Also I'm sure you
> have noticed that I forgot to add `pkg-file' argument to (error ...)
> expression above.

Right, so a simpler way to make the change is to not add an explicit
check but instead just remove the `noerror' argument.  So your patch is
apparently changing a consciously made decision to ignore errors.
I tend to agree with the change, but I'd first understand why someone
decided to ignore those error.


        Stefan

bug#7756: 24.0.50; enhancements to package.el

[Stefan Kangas]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> @@ -338,9 +338,14 @@
>>       (pkg-file (expand-file-name
>>              (concat (package-strip-version package) "-pkg")
>>              pkg-dir)))
>> -    (when (and (file-directory-p pkg-dir)
>> -           (file-exists-p (concat pkg-file ".el")))
>> -      (load pkg-file nil t))))
>> +    ;; When one is creating a package and testing it out, it is easy
>> +    ;; to forget to add the -pkg.el file.  When that happens, it would
>> +    ;; be useful to provide feedback to the user rather than silently
>> +    ;; failing.  That is what (error ...) below is for.
>> +    (when (file-directory-p pkg-dir)
>> +      (if (file-exists-p (concat pkg-file ".el"))
>> +          (load pkg-file nil t)
>> +        (error "'%s' file is missing!")))))
>
> I agree with the intention, but I wonder: why do we pass the `noerror'
> parameter to `load' in the first place?

This code seems to have changed substantially since this patch was
proposed in 2010.  Having looked at it now, I think naively raising an
error here would risk breaking things.  I therefore suggest to do
nothing here for now.

>> @@ -569,8 +574,16 @@
>>  (defun package-unpack (name version)
>>    (let ((pkg-dir (expand-file-name (concat (symbol-name name) "-" version)
>>                     package-user-dir)))
>> +    ;; Delete the package directory if it exists already.  This may
>> +    ;; not be useful to the end users.  However this is extremely
>> +    ;; important for package developers as they experiment with which
>> +    ;; files to include in a package.  If a file is removed from one
>> +    ;; iteration to the next, then the presence of the unwanted elisp
>> +    ;; file could cause problems by polluting the generated autoload
>> +    ;; file.
>> +    (if (file-directory-p pkg-dir)
>> +        (delete-directory pkg-dir 'recursive))
>
> Here, I also agree with the intention, but I'm a little uneasy blindly
> removing a whole subdirectory without warning: I'd add a confirmation prompt.

I have attached a patch which does the same but adds a confirmation
prompt.  I've also added a test case for this particular case.  WDYT?

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Prompt-to-delete-duplicate-package-directory-on-inst.patch --]
[-- Type: text/x-patch, Size: 3400 bytes --]

From b1962898dac4c89c58db066f598b656ba42f8807 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 15 Sep 2019 01:49:37 +0200
Subject: [PATCH] Prompt to delete duplicate package directory on install

* lisp/emacs-lisp/package.el (package-unpack): Prompt to delete
package directory if it already exists.  (Bug#7756)

* test/lisp/emacs-lisp/package-tests.el
(package-test-install-multi-file/pkg-dir-already-exists): New test
case for installation when package directory already exists.
(with-package-test): Support above test case.
---
 lisp/emacs-lisp/package.el            |  6 +++++-
 test/lisp/emacs-lisp/package-tests.el | 12 +++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ef0c5171de..2ac07bc055 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -888,7 +888,11 @@ package-unpack
                (if (> (length file-list) 1) 'tar 'single))))
       ('tar
        (make-directory package-user-dir t)
-       ;; FIXME: should we delete PKG-DIR if it exists?
+       (when (and (file-directory-p pkg-dir)
+                  (yes-or-no-p
+                   (format "Package directory already exists: `%s'.  Delete it?"
+                           (directory-file-name pkg-dir))))
+         (delete-directory pkg-dir 'recursive))
        (let* ((default-directory (file-name-as-directory package-user-dir)))
          (package-untar-buffer dirname)))
       ('single
diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
index c757bccf67..ffe04e8fe7 100644
--- a/test/lisp/emacs-lisp/package-tests.el
+++ b/test/lisp/emacs-lisp/package-tests.el
@@ -109,7 +109,8 @@ with-package-test
                                            install
                                            location
                                            update-news
-                                           upload-base)
+                                           upload-base
+                                           create-directory)
                                 &rest body)
   "Set up temporary locations and variables for testing."
   (declare (indent 1) (debug (([&rest form]) body)))
@@ -138,6 +139,9 @@ with-package-test
            ,(if basedir `(cd ,basedir))
            (unless (file-directory-p package-user-dir)
              (mkdir package-user-dir))
+           ,(if create-directory
+                `(make-directory (expand-file-name ,create-directory
+                                                   package-test-user-dir) t))
            (cl-letf (((symbol-function 'yes-or-no-p) (lambda (&rest r) t))
                      ((symbol-function 'y-or-n-p)    (lambda (&rest r) t)))
              ,@(when install
@@ -344,6 +348,12 @@ package-test-install-multifile
           (goto-char (point-min))
           (should (re-search-forward re nil t)))))))
 
+(ert-deftest package-test-install-multi-file/pkg-dir-already-exists ()
+  "Install multi-file package over already existing directory."
+  (with-package-test (:basedir "package-resources" :install '(multi-file)
+                      :create-directory "multi-file-0.2.3")
+    (should (package-installed-p 'multi-file))))
+
 (ert-deftest package-test-update-listing ()
   "Ensure installed package status is updated."
   (with-package-test ()
-- 
2.20.1

bug#7756: 24.0.50; enhancements to package.el

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> -       ;; FIXME: should we delete PKG-DIR if it exists?
> +       (when (and (file-directory-p pkg-dir)
> +                  (yes-or-no-p
> +                   (format "Package directory already exists: `%s'.  Delete it?"
> +                           (directory-file-name pkg-dir))))
> +         (delete-directory pkg-dir 'recursive))

I'm a bit uneasy about doing a recursive deletion of the directory, even
after prompting (the developer may have put, well, anything in that
directory), so I wonder whether the right fix here is to just remove the
FIXME.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#7756: 24.0.50; enhancements to package.el

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 23 Nov 2019 14:35:43 +0100
> Cc: 7756@debbugs.gnu.org, Richard Kim <emacs18@gmail.com>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> 
> I'm a bit uneasy about doing a recursive deletion of the directory, even
> after prompting (the developer may have put, well, anything in that
> directory), so I wonder whether the right fix here is to just remove the
> FIXME.

FWIW, I'm fine with deleting the FIXME.  But I'm not a package
developer, so perhaps my opinion isn't worth much.

bug#7756: 24.0.50; enhancements to package.el

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> > I'm a bit uneasy about doing a recursive deletion of the directory, even
> > after prompting (the developer may have put, well, anything in that
> > directory), so I wonder whether the right fix here is to just remove the
> > FIXME.
>
> FWIW, I'm fine with deleting the FIXME.  But I'm not a package
> developer, so perhaps my opinion isn't worth much.

Sounds good to me.  I'll go ahead and do that in a couple of days if I
see no objections.

Best regards,
Stefan Kangas