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.