GNU bug report logs - #7756
24.0.50; enhancements to package.el

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 7756 in the body.
You can then email your comments to 7756 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: emacs18 <at> gmail.com
To: bug-gnu-emacs <at> gnu.org
Subject: 24.0.50; enhancements to package.el
Date: Wed, 29 Dec 2010 10:40:56 -0800

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)

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: emacs18 <at> gmail.com
Cc: 7756 <at> debbugs.gnu.org
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Thu, 30 Dec 2010 10:32:27 -0500

> @@ -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

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

From: Richard Kim <emacs18 <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7756 <at> debbugs.gnu.org
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Thu, 30 Dec 2010 09:27:24 -0800

Stefan Monnier <monnier <at> 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. 

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: emacs18 <at> gmail.com
Cc: 7756 <at> debbugs.gnu.org
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Fri, 31 Dec 2010 11:46:11 -0500

>>> @@ -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

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7756 <at> debbugs.gnu.org, Richard Kim <emacs18 <at> gmail.com>
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Sun, 15 Sep 2019 01:56:08 +0200

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

Stefan Monnier <monnier <at> 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

[0001-Prompt-to-delete-duplicate-package-directory-on-inst.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 7756 <at> debbugs.gnu.org, Richard Kim <emacs18 <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Sat, 23 Nov 2019 14:35:43 +0100

Stefan Kangas <stefan <at> 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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: emacs18 <at> gmail.com, 7756 <at> debbugs.gnu.org, stefan <at> marxist.se,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Sat, 23 Nov 2019 15:45:38 +0200

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Sat, 23 Nov 2019 14:35:43 +0100
> Cc: 7756 <at> debbugs.gnu.org, Richard Kim <emacs18 <at> gmail.com>,
>  Stefan Monnier <monnier <at> 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.

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 7756 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 Richard Kim <emacs18 <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Tue, 26 Nov 2019 16:01:04 +0100

Eli Zaretskii <eliz <at> 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

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 7756-done <at> debbugs.gnu.org,
 Richard Kim <emacs18 <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#7756: 24.0.50; enhancements to package.el
Date: Fri, 17 Jan 2020 00:49:36 +0100

Stefan Kangas <stefan <at> marxist.se> writes:

> Eli Zaretskii <eliz <at> 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.

Now done on the master branch in commit f18c78e611.  I see nothing
more to do here, so I'm also closing this bug.

Best regards,
Stefan Kangas

Previous Next

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