[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case)
From: |
Philip Kaludercic |
Subject: |
bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case) |
Date: |
Thu, 16 Nov 2023 07:25:54 +0000 |
Sylvain Bougerel <sylvain.bougerel.devel@gmail.com> writes:
>> Here the issue shows itself, `seq` is now requested for installation,
>> which is incorrect, since Emacs already have the latest version.
>
> TL;DR I believe I have found the bug in `package-compute-transaction'.
> It misses the
> assignment to the newer version of the package that was already in the list of
> packages to be installed.
>
>
> Some details. The following 1-line change fixes it for me (added context
> around
> for readability), in `package-compute-transaction':
>
> (when already
> (if (version-list-<= next-version (package-desc-version already))
> ;; `next-pkg' is already in `packages', but its position there
> ;; means it might be installed too late: remove it from there, so
> ;; we re-add it (along with its dependencies) at an earlier place
> ;; below (bug#16994).
> (if (memq already seen) ;Avoid inf-loop on dependency cycles.
> (message "Dependency cycle going through %S"
> (package-desc-full-name already))
> (setq packages (delq already packages))
> + (setq next-version (package-desc-version already))
> (setq already nil))
> (error "Need package `%s-%s', but only %s is being installed"
> next-pkg (package-version-join next-version)
> (package-version-join (package-desc-version already)))))
>
>
> The issue occurs in the following scenario:
> - Package A (version a), noted A(a), depends on packages B(z), then C(y)
> - Package C(y) depends on package B(x), which is an _older_
> version than B(z)
> - Neither A(a), B(z) nor C(y) are installed (the state of B(x) is
> irrelevant)
>
> `package-compute-transaction' will consider first the requirement B(z) and
> push
> it to PACKAGES; then the requirement C(y) is pushed. It will then consider
> C(y)'s requirement B(x). Since B is ALREADY seen, it first deletes B(z) from
> PACKAGES, expecting to push in front of PACKAGES later, in `cond''s body (so
> that B is installed earlier than both A and C). **But it fails to set
> NEXT-VERSION to the newer B(z) and retains B(x)'s older version instead.**
> That's the edge-case of the bug.
>
> The fix is to set NEXT-VERSION to the version of the equal or newer package,
> which is always the one already present in PACKAGE, given the preceding `if'
> condition.
This certainly makes sense, one just has to make sure that there aren't
any other edge-cases being broken by this change :/ I'll apply the
patch locally and test it over the next few days.
> In my case, this result in the strange disappearance of `seq' from PACKAGES
> when
> `compat' is not installed, because the older version of `seq' is already
> installed and it's enough to meet the requirement of one of the transitive
> dependencies of `git-commit', which is `compat'. On the other end, if `compat'
> is installed, it's requirements are not checked and thus `seq' remains in the
> list.
>
> I provided the format-patch for it in the attachment, but it's the
> same single line
> of code change as shown above. I have no knowledge on how to submit it other
> than by this email, any guidance or direction is appreciated.
A short commit like this can be manually applied, and attributed to you,
otherwise you should consult the CONTRIBUTING file in emacs.git for
notes on how to format a patch and the commit message.
> Though interestingly, that was actually not the root cause of my personal
> problem. I didn't expect `seq' to be installed in the first place, it is
> built-in and `package-install-upgrade-built-in' is `nil' on my setup. (I was
> expecting to find an issue revolving around this, and stumbled on this one :)
> )
> Do you think I have misunderstood the purpose of
> `package-install-upgrade-built-in'? Should the `package-compute-transaction`
> be
> fixed to take it into account? It is not too difficult to do, however I think
> this would be a rather impacting change, as users may have gotten used to
> their
> built-in packages dependencies being upgraded without friction, even when they
> are built-in.
That user option shouldn't affect this issue, it was added so that
package-install could install a newer version of a built-in package from
ELPA, but that doesn't affect dependencies IIRC.
> Looking for another opinion, I lack context on the purpose of
> `package-install-upgrade-built-in`.
If you are really interested, you can take a look at bug#62720.
> - Sylvain
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: 66985@debbugs.gnu.org
>> From: Sylvain Bougerel <sylvain.bougerel.devel@gmail.com>
>> Date: Sat, 11 Nov 2023 01:05:45 +0800
>>
>> > Here the issue shows itself, `seq` is now requested for installation,
>> > which is incorrect, since Emacs already have the latest version.
>>
>> TL;DR I believe I have found the bug in `package-compute-transaction'.
>> It misses the
>> assignment to the newer version of the package that was already in the list
>> of
>> packages to be installed.
>
> Philip, any comments or suggestions?
Nothing special, other than that I had been planning to re-write the
function anyway, to make it simpler and re-usable by package-vc.
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Sylvain Bougerel, 2023/11/07
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Sylvain Bougerel, 2023/11/07
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Philip Kaludercic, 2023/11/08
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Sylvain Bougerel, 2023/11/08
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Sylvain Bougerel, 2023/11/10
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Sylvain Bougerel, 2023/11/15
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Eli Zaretskii, 2023/11/15
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case),
Philip Kaludercic <=
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Sylvain Bougerel, 2023/11/17
- bug#66985: 29.1; Issue with `package.el` upgrading builtin (edge-case), Philip Kaludercic, 2023/11/27