bug#27822: 26.0.50; package-autoremove and 'package-directory-list

bug#27822: 26.0.50; package-autoremove and 'package-directory-list

[Yuri D'Elia]

package-autoremove shouldn't attempt to remove external packages
(additional packages detected via in package-directory-list).

Debian now includes several copies of elpa packages in the repository.
The directory including those are added to 'package-directory-list.

I also manage some personal packages locally, and I also add the
directory into 'package'directory-list.

These packages are shown as "external" by "list-packages", which is
correct. They are obviously not directly installed via package-install,
and cannot be autoremoved either.

package-autoremove however will still attempt to remove them. My feeling
is that any package which is not directly in 'package-user-dir shouldn't
be autoremoved.

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Yuri D'Elia]

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

User-agent: mu4e 1.1.0; emacs 27.0.50
The package-autoremove machinery assumes all packages which are not
user-selected are removable (see https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27822)

However this is blatantly untrue. For example, in debian unstable, emacs
packages can be installed system-wide via apt as well (see all the elpa-*
packages).

Such packages _cannot_ and shouldn't be auto-removed.

The following patch checks whether a package has been _directly_ installed
into `package-user-dir' before marking it eligible for auto-removal.

[copyright assignment already done]


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-consider-external-packages-to-be-removable.patch --]
[-- Type: text/x-diff, Size: 1911 bytes --]

From 5a4234b3c1289960fd760256aa6399efbd04bfc3 Mon Sep 17 00:00:00 2001
From: Yuri D'Elia <wavexx@thregr.org>
Date: Tue, 17 Jul 2018 12:59:35 +0200
Subject: [PATCH] Do not consider external packages to be removable

Packages which are not directly user-installed shouldn't be autoremoved,
since they can be setup through a different path (via
`package-directory-list') where we have no authority over.
---
 lisp/emacs-lisp/package.el | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 36233522..534f155c 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1754,6 +1754,15 @@ if it is still empty."
       (indirect indirect-deps)
       (t        (delete-dups (append direct-deps indirect-deps))))))
 
+(defun package--user-installed-p (pkg)
+  "Check if the package is a user-installed package.
+PKG is a package name.
+Checks whether the package was installed into `package-user-dir' where
+we assume to have control over."
+  (let* ((pkg-desc (cadr (assq pkg package-alist)))
+         (dir (package-desc-dir pkg-desc)))
+    (file-in-directory-p dir package-user-dir)))
+
 (defun package--removable-packages ()
   "Return a list of names of packages no longer needed.
 These are packages which are neither contained in
@@ -1763,7 +1772,9 @@ These are packages which are neither contained in
                          ;; `p' and its dependencies are needed.
                          append (cons p (package--get-deps p)))))
     (cl-loop for p in (mapcar #'car package-alist)
-             unless (memq p needed)
+             unless (or (memq p needed)
+                        ;; do not auto-remove external packages
+                        (not (package--user-installed-p p)))
              collect p)))
 
 (defun package--used-elsewhere-p (pkg-desc &optional pkg-list all)
-- 
2.18.0

[PATCH] do not auto-remove external packages (fixes #27822)

[Yuri D'Elia]

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

User-agent: mu4e 1.1.0; emacs 27.0.50
The package-autoremove machinery assumes all packages which are not
user-selected are removable (see https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27822)

However this is blatantly untrue. For example, in debian unstable, emacs
packages can be installed system-wide via apt as well (see all the elpa-*
packages).

Such packages _cannot_ and shouldn't be auto-removed.

The following patch checks whether a package has been _directly_ installed
into `package-user-dir' before marking it eligible for auto-removal.

[copyright assignment already done]


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-consider-external-packages-to-be-removable.patch --]
[-- Type: text/x-diff, Size: 1911 bytes --]

From 5a4234b3c1289960fd760256aa6399efbd04bfc3 Mon Sep 17 00:00:00 2001
From: Yuri D'Elia <wavexx@thregr.org>
Date: Tue, 17 Jul 2018 12:59:35 +0200
Subject: [PATCH] Do not consider external packages to be removable

Packages which are not directly user-installed shouldn't be autoremoved,
since they can be setup through a different path (via
`package-directory-list') where we have no authority over.
---
 lisp/emacs-lisp/package.el | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 36233522..534f155c 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1754,6 +1754,15 @@ if it is still empty."
       (indirect indirect-deps)
       (t        (delete-dups (append direct-deps indirect-deps))))))
 
+(defun package--user-installed-p (pkg)
+  "Check if the package is a user-installed package.
+PKG is a package name.
+Checks whether the package was installed into `package-user-dir' where
+we assume to have control over."
+  (let* ((pkg-desc (cadr (assq pkg package-alist)))
+         (dir (package-desc-dir pkg-desc)))
+    (file-in-directory-p dir package-user-dir)))
+
 (defun package--removable-packages ()
   "Return a list of names of packages no longer needed.
 These are packages which are neither contained in
@@ -1763,7 +1772,9 @@ These are packages which are neither contained in
                          ;; `p' and its dependencies are needed.
                          append (cons p (package--get-deps p)))))
     (cl-loop for p in (mapcar #'car package-alist)
-             unless (memq p needed)
+             unless (or (memq p needed)
+                        ;; do not auto-remove external packages
+                        (not (package--user-installed-p p)))
              collect p)))
 
 (defun package--used-elsewhere-p (pkg-desc &optional pkg-list all)
-- 
2.18.0

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Stefan Monnier]

[ Please don't crosspost between bugs and emacs-devel.  ]

> package-autoremove however will still attempt to remove them. My feeling
> is that any package which is not directly in 'package-user-dir shouldn't
> be autoremoved.

AFAIK packages installed via dpkg can't be removed (lack of access
rights), so attempting to remove them is not that big of a deal, is it?
Or does it signal an error, prevent local packages from being
auto-removed, or something (I mean, beside the philosophical argument,
does it result in an actual harmful behavior in practice)?

> The following patch checks whether a package has been _directly_ installed
> into `package-user-dir' before marking it eligible for auto-removal.

Looks good to me,


        Stefan

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Yuri D'Elia]

On Tue, Jul 17 2018, Stefan Monnier wrote:
> AFAIK packages installed via dpkg can't be removed (lack of access
> rights), so attempting to remove them is not that big of a deal, is it?

As described in the bug report, if I add a directory of with custom
packages, autoremove will happily zap all of them because they're not
marked as manually installed.

Additional directories shouldn't be managed by package.el.

> Or does it signal an error, prevent local packages from being
> auto-removed, or something (I mean, beside the philosophical argument,
> does it result in an actual harmful behavior in practice)?

Both. Harmful behavior (deletion) in case there's an additional
directory of user-handled packages. Removal error due to missing
permissions also prevents dependent packages to be auto-removed.

package.el fix (was Re: [PATCH] do not auto-remove external packages (fixes #27822))

[Yuri D'Elia]

Can anybody have a look at the proposed fix in bug #27822?

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27822

That's a trivial fix in package.el to improve autoremoval. I've been
using it ever since and I'd like to get it mainlined.

Thanks.

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Noam Postavsky]

Yuri D'Elia <wavexx@thregr.org> writes:
>
>>From 5a4234b3c1289960fd760256aa6399efbd04bfc3 Mon Sep 17 00:00:00 2001
> From: Yuri D'Elia <wavexx@thregr.org>
> Date: Tue, 17 Jul 2018 12:59:35 +0200
> Subject: [PATCH] Do not consider external packages to be removable
>
> Packages which are not directly user-installed shouldn't be autoremoved,
> since they can be setup through a different path (via
> `package-directory-list') where we have no authority over.

The patch looks okay for emacs-26, I'll push in a few days assuming no
objections.

A couple of nitpicks below:

> +(defun package--user-installed-p (pkg)
> +  "Check if the package is a user-installed package.
> +PKG is a package name.

I think this is better shortened to

    Return non-nil if the package named PKG is user-installed.

> +Checks whether the package was installed into `package-user-dir' where
> +we assume to have control over."
> +  (let* ((pkg-desc (cadr (assq pkg package-alist)))
> +         (dir (package-desc-dir pkg-desc)))
> +    (file-in-directory-p dir package-user-dir)))
> +
>  (defun package--removable-packages ()
>    "Return a list of names of packages no longer needed.
>  These are packages which are neither contained in
> @@ -1763,7 +1772,9 @@ These are packages which are neither contained in

By the way, if you run './autogen --git' you should get nicer headers
for lisp diffs.

>                           ;; `p' and its dependencies are needed.
>                           append (cons p (package--get-deps p)))))
>      (cl-loop for p in (mapcar #'car package-alist)
> -             unless (memq p needed)
> +             unless (or (memq p needed)
> +                        ;; do not auto-remove external packages

Comments should be capitalized and end with a period.

> +                        (not (package--user-installed-p p)))
>               collect p)))

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Yuri D'Elia]

On Thu, Aug 09 2018, Noam Postavsky wrote:
>> @@ -1763,7 +1772,9 @@ These are packages which are neither contained in
>
> By the way, if you run './autogen --git' you should get nicer headers
> for lisp diffs.

Thanks for pointing this out!

As for the rest, I have no objections.
I assume I don't have to tweak the patch myself.

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Wed, 08 Aug 2018 21:19:02 -0400
> Cc: 27822@debbugs.gnu.org
> 
> > +(defun package--user-installed-p (pkg)
> > +  "Check if the package is a user-installed package.
> > +PKG is a package name.
> 
> I think this is better shortened to
> 
>     Return non-nil if the package named PKG is user-installed.

Or maybe

  (defun package--user-installed-p (package)
    "Return non-nil if PACKAGE is a user-installed package.
  PACKAGE is the package name, a string."

Thanks.

bug#27822: [PATCH] do not auto-remove external packages (fixes #27822)

[Noam Postavsky]

tags 27822 fixed
close 27822 26.2
quit

Eli Zaretskii <eliz@gnu.org> writes:

> Or maybe
>
>   (defun package--user-installed-p (package)
>     "Return non-nil if PACKAGE is a user-installed package.
>   PACKAGE is the package name, a string."

I went with this phrasing, except PACKAGE is a symbol, not a string.

[1: d2ad4ba4f3]: 2018-08-11 21:06:26 -0400
  Do not consider external packages to be removable (Bug#27822)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d2ad4ba4f3c5db6f6be7d73c17332e9bc4570e29