Suggestion for package.el: atomic package-upgrade

Suggestion for package.el: atomic package-upgrade

[dqs7cp2e]

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

The current `package-upgrade' from package.el delete old package
before installing the new one. This can be problematic if the user
interrupt the process or if there is some network problems.

`package-install' allow the same package to be installed if the
argument is `package-desc' instead symbol representing package name.
This allow package to be upgraded atomically. Is this a bad idea?

[-- Attachment #2: package-upgrade--diff.txt --]
[-- Type: text/plain, Size: 1691 bytes --]

diff -u --label \#\<buffer\ package.el.gz\> --label \#\<buffer\ \*scratch\*\> /tmp/buffer-content-4azQGZ /tmp/buffer-content-x8FLpt
--- #<buffer package.el.gz>
+++ #<buffer *scratch*>
@@ -2275,16 +2275,20 @@
 package using this command, first upgrade the package to a
 newer version from ELPA by using `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
   (interactive
-   (list (completing-read
-          "Upgrade package: " (package--upgradeable-packages) nil t)))
-  (let* ((package (if (symbolp name)
-                      name
-                    (intern name)))
-         (pkg-desc (cadr (assq package package-alist))))
-    (if (package-vc-p pkg-desc)
-        (package-vc-upgrade pkg-desc)
-      (package-delete pkg-desc 'force 'dont-unselect)
-      (package-install package 'dont-select))))
+   (list (intern (completing-read
+                  "Upgrade package: " (package--upgradeable-packages) nil t))))
+  (let* ((name (if (symbolp name)
+                   name
+                 (intern name)))
+         (old-pkg-desc (cadr (assq name package-alist)))
+         (new-pkg-desc (cadr (assq name package-archive-contents))))
+    (if (package-vc-p old-pkg-desc)
+        (package-vc-upgrade old-pkg-desc)
+      (unwind-protect
+          (package-install new-pkg-desc 'dont-select)
+        (if (package-installed-p (package-desc-name new-pkg-desc)
+                                 (package-desc-version new-pkg-desc))
+            (package-delete old-pkg-desc 'force 'dont-unselect))))))
 
 (defun package--upgradeable-packages ()
   ;; Initialize the package system to get the list of package

Diff finished.  Mon Jul 31 08:22:46 2023

Re: Suggestion for package.el: atomic package-upgrade

[Philip Kaludercic]

dqs7cp2e <dqs7cp2e@gmail.com> writes:

> The current `package-upgrade' from package.el delete old package
> before installing the new one. This can be problematic if the user
> interrupt the process or if there is some network problems.
>
> `package-install' allow the same package to be installed if the
> argument is `package-desc' instead symbol representing package name.
> This allow package to be upgraded atomically. Is this a bad idea?

No, I think this is a good idea, but it would be best if you could write
a Git patch and send it to "bug-gnu-emacs@gnu.org" (you can use M-x
submit-emacs-patch).

> diff -u --label \#\<buffer\ package.el.gz\> --label \#\<buffer\ \*scratch\*\> /tmp/buffer-content-4azQGZ /tmp/buffer-content-x8FLpt
> --- #<buffer package.el.gz>
> +++ #<buffer *scratch*>
> @@ -2275,16 +2275,20 @@
>  package using this command, first upgrade the package to a
>  newer version from ELPA by using `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>    (interactive
> -   (list (completing-read
> -          "Upgrade package: " (package--upgradeable-packages) nil t)))
> -  (let* ((package (if (symbolp name)
> -                      name
> -                    (intern name)))
> -         (pkg-desc (cadr (assq package package-alist))))
> -    (if (package-vc-p pkg-desc)
> -        (package-vc-upgrade pkg-desc)
> -      (package-delete pkg-desc 'force 'dont-unselect)
> -      (package-install package 'dont-select))))
> +   (list (intern (completing-read
> +                  "Upgrade package: " (package--upgradeable-packages) nil t))))
> +  (let* ((name (if (symbolp name)
> +                   name
> +                 (intern name)))

If you always intern the package name, you don't need this check
anymore.  On the other hand, you don't need to intern the name, because
of this check, and I think it is better to keep it because that gives
the user more flexibility when invoking the function.

> +         (old-pkg-desc (cadr (assq name package-alist)))
> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
> +    (if (package-vc-p old-pkg-desc)
> +        (package-vc-upgrade old-pkg-desc)
> +      (unwind-protect

I am wondering if unwind-protect is the best approach here, or even
necessary.  Wouldn't something like

--8<---------------cut here---------------start------------->8---
(when (progn (package-install new-pkg-desc 'dont-select) t)
  (package-delete old-pkg-desc 'force 'dont-unselect))
--8<---------------cut here---------------end--------------->8---

have the same effect?  My reasoning is that we are not actually cleaning
anything up in the UNWIND-FORMS, but just checking if the
`package-install' was not interrupted, right?

> +          (package-install new-pkg-desc 'dont-select)
> +        (if (package-installed-p (package-desc-name new-pkg-desc)
> +                                 (package-desc-version new-pkg-desc))

If you check the definition of `package-installed-p', you will find it
does not use MIN-VERSION if the first argument satisfied
`package-desc-p', which makes sense because with a concrete descriptor,
we can unambiguously check if a specific package version has been
installed (the implementation just checks if the expected directory
exists).

Also, perhaps consider using `when' here.  I argue that it looks better
in imperative code where the return-value is not of interest.

> +            (package-delete old-pkg-desc 'force 'dont-unselect))))))
>  
>  (defun package--upgradeable-packages ()
>    ;; Initialize the package system to get the list of package
>
> Diff finished.  Mon Jul 31 08:22:46 2023

Re: Suggestion for package.el: atomic package-upgrade

[Tegar Syahputra]

>> The current `package-upgrade' from package.el delete old package
>> before installing the new one. This can be problematic if the user
>> interrupt the process or if there is some network problems.
>>
>> `package-install' allow the same package to be installed if the
>> argument is `package-desc' instead symbol representing package name.
>> This allow package to be upgraded atomically. Is this a bad idea?
> No, I think this is a good idea, but it would be best if you could write
> a Git patch and send it to "bug-gnu-emacs@gnu.org" (you can use M-x
> submit-emacs-patch).

I'm not part of FSF contributor, and wasn't really thinking of contributing
directly. Just giving a heads up that's all.

>> diff -u --label \#\<buffer\ package.el.gz\> --label \#\<buffer\ \*scratch\*\> /tmp/buffer-content-4azQGZ /tmp/buffer-content-x8FLpt
>> --- #<buffer package.el.gz>
>> +++ #<buffer *scratch*>
>> @@ -2275,16 +2275,20 @@
>>    package using this command, first upgrade the package to a
>>    newer version from ELPA by using `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>>      (interactive
>> -   (list (completing-read
>> -          "Upgrade package: " (package--upgradeable-packages) nil t)))
>> -  (let* ((package (if (symbolp name)
>> -                      name
>> -                    (intern name)))
>> -         (pkg-desc (cadr (assq package package-alist))))
>> -    (if (package-vc-p pkg-desc)
>> -        (package-vc-upgrade pkg-desc)
>> -      (package-delete pkg-desc 'force 'dont-unselect)
>> -      (package-install package 'dont-select))))
>> +   (list (intern (completing-read
>> +                  "Upgrade package: " (package--upgradeable-packages) nil t))))
>> +  (let* ((name (if (symbolp name)
>> +                   name
>> +                 (intern name)))
> If you always intern the package name, you don't need this check
> anymore.  On the other hand, you don't need to intern the name, because
> of this check, and I think it is better to keep it because that gives
> the user more flexibility when invoking the function.

I intern the interactive input because the actual type needed is symbol 
type.
The check was from original, it accept both string and symbol.

>> +         (old-pkg-desc (cadr (assq name package-alist)))
>> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
>> +    (if (package-vc-p old-pkg-desc)
>> +        (package-vc-upgrade old-pkg-desc)
>> +      (unwind-protect
> I am wondering if unwind-protect is the best approach here, or even
> necessary.  Wouldn't something like
>
> --8<---------------cut here---------------start------------->8---
> (when (progn (package-install new-pkg-desc 'dont-select) t)
>     (package-delete old-pkg-desc 'force 'dont-unselect))
> --8<---------------cut here---------------end--------------->8---

No, you must check if the package is installed. That will always delete
`old-pkg-desc' even if installation error occurs.

> have the same effect?  My reasoning is that we are not actually cleaning
> anything up in the UNWIND-FORMS, but just checking if the
> `package-install' was not interrupted, right?

Yes, it's to check if the installations was interrupted or if error occur.
I don't think `package-install' gives meaningful return that indicates
package was properly installed. The output it gives could be used but,
I just thought checking the output string may not be reliable or elegant.

>> +          (package-install new-pkg-desc 'dont-select)
>> +        (if (package-installed-p (package-desc-name new-pkg-desc)
>> +                                 (package-desc-version new-pkg-desc))
> If you check the definition of `package-installed-p', you will find it
> does not use MIN-VERSION if the first argument satisfied
> `package-desc-p', which makes sense because with a concrete descriptor,
> we can unambiguously check if a specific package version has been
> installed (the implementation just checks if the expected directory
> exists).

Yeah, and new-pkg-desc which is a package-desc from package-archive-contents
is implied to be remote thus doesn't include directory.


I'm OP, sorry I didn't configure my email name earlier.

Re: Suggestion for package.el: atomic package-upgrade

[Philip Kaludercic]

Tegar Syahputra <dqs7cp2e@gmail.com> writes:

>>> The current `package-upgrade' from package.el delete old package
>>> before installing the new one. This can be problematic if the user
>>> interrupt the process or if there is some network problems.
>>>
>>> `package-install' allow the same package to be installed if the
>>> argument is `package-desc' instead symbol representing package name.
>>> This allow package to be upgraded atomically. Is this a bad idea?
>> No, I think this is a good idea, but it would be best if you could write
>> a Git patch and send it to "bug-gnu-emacs@gnu.org" (you can use M-x
>> submit-emacs-patch).
>
> I'm not part of FSF contributor, and wasn't really thinking of contributing
> directly. Just giving a heads up that's all.

So you are not interested in contributing a patch?

>>> diff -u --label \#\<buffer\ package.el.gz\> --label \#\<buffer\ \*scratch\*\> /tmp/buffer-content-4azQGZ /tmp/buffer-content-x8FLpt
>>> --- #<buffer package.el.gz>
>>> +++ #<buffer *scratch*>
>>> @@ -2275,16 +2275,20 @@
>>>    package using this command, first upgrade the package to a
>>>    newer version from ELPA by using `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>>>      (interactive
>>> -   (list (completing-read
>>> -          "Upgrade package: " (package--upgradeable-packages) nil t)))
>>> -  (let* ((package (if (symbolp name)
>>> -                      name
>>> -                    (intern name)))
>>> -         (pkg-desc (cadr (assq package package-alist))))
>>> -    (if (package-vc-p pkg-desc)
>>> -        (package-vc-upgrade pkg-desc)
>>> -      (package-delete pkg-desc 'force 'dont-unselect)
>>> -      (package-install package 'dont-select))))
>>> +   (list (intern (completing-read
>>> +                  "Upgrade package: " (package--upgradeable-packages) nil t))))
>>> +  (let* ((name (if (symbolp name)
>>> +                   name
>>> +                 (intern name)))
>> If you always intern the package name, you don't need this check
>> anymore.  On the other hand, you don't need to intern the name, because
>> of this check, and I think it is better to keep it because that gives
>> the user more flexibility when invoking the function.
>
> I intern the interactive input because the actual type needed is
> symbol type.
> The check was from original, it accept both string and symbol.

Right, you kept the check (which I think is correct), which makes the
conversion unnecessary.

>>> +         (old-pkg-desc (cadr (assq name package-alist)))
>>> +         (new-pkg-desc (cadr (assq name package-archive-contents))))
>>> +    (if (package-vc-p old-pkg-desc)
>>> +        (package-vc-upgrade old-pkg-desc)
>>> +      (unwind-protect
>> I am wondering if unwind-protect is the best approach here, or even
>> necessary.  Wouldn't something like
>>
>> --8<---------------cut here---------------start------------->8---
>> (when (progn (package-install new-pkg-desc 'dont-select) t)
>>     (package-delete old-pkg-desc 'force 'dont-unselect))
>> --8<---------------cut here---------------end--------------->8---
>
> No, you must check if the package is installed. That will always delete
> `old-pkg-desc' even if installation error occurs.
>
>> have the same effect?  My reasoning is that we are not actually cleaning
>> anything up in the UNWIND-FORMS, but just checking if the
>> `package-install' was not interrupted, right?
>
> Yes, it's to check if the installations was interrupted or if error occur.
> I don't think `package-install' gives meaningful return that indicates
> package was properly installed. The output it gives could be used but,
> I just thought checking the output string may not be reliable or elegant.

Unless I am mistaken, package-install should raise a signal (ie. causing
a non-local exit).  The return value of `package-install' is not
reliable, hence the (progn ... t).

>>> +          (package-install new-pkg-desc 'dont-select)
>>> +        (if (package-installed-p (package-desc-name new-pkg-desc)
>>> +                                 (package-desc-version new-pkg-desc))
>> If you check the definition of `package-installed-p', you will find it
>> does not use MIN-VERSION if the first argument satisfied
>> `package-desc-p', which makes sense because with a concrete descriptor,
>> we can unambiguously check if a specific package version has been
>> installed (the implementation just checks if the expected directory
>> exists).
>
> Yeah, and new-pkg-desc which is a package-desc from package-archive-contents
> is implied to be remote thus doesn't include directory.

Right, my mistake.

> I'm OP, sorry I didn't configure my email name earlier.