[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot
From: |
Philip Kaludercic |
Subject: |
bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot |
Date: |
Wed, 12 Apr 2023 20:10:50 +0000 |
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: joaotavora@gmail.com, monnier@iro.umontreal.ca, 62720@debbugs.gnu.org,
>> larsi@gnus.org
>> Date: Wed, 12 Apr 2023 13:42:56 +0000
>>
>> >> After thinking about this for a bit, I think that the right approach is
>> >> to use package-install instead of writing a separate command. After
>> >> all, this will make the behaviour of package-install consistent with
>> >> that of the package menu.
>> >
>> > Is this for master or for the release branch?
>>
>> Personally I am indifferent, it should be compatible with both
>
> If we want to install this on the release branch, the changes must be
> very safe, which in this case basically means "do not touch any code
> used when updating non-core packages".
>
> If the patch you presented is all there is to it, then I'm afraid it
> doesn't satisfy the above condition, because it does affect the use
> case of updating an ELPA package that is not in core. So I cannot
> agree to installing this on emacs-29 in the form you posted, sorry.
I understand your concern,
>> > And I thought we all agreed built-in packages need special treatment
>> > anyway, didn't we? Then why having a separate command is not a
>> > natural next step?
>>
>> I don't necessarily agree that "special treatment" requires a separate
>> command.
>
> Even if you don't agree with that in general, having a separate
> command would allow us to install that command on emacs-29 without any
> fears. So that alone is a significant advantage, even if the rest are
> not yet agreed upon.
Would this be a permanent thing, or would the command be deprecated by
emacs-30? I get the need under the circumstances, but it doesn't seem
like the best solution if we were to set aside release-concerns.
>> I think it is wrong the assume that an built-in package should
>> automatically be updated to a ELPA package whenever possible.
>
> This seems to be an argument in favor of a separate command? Or what
> did you mean by that, and how is it related to the issue at hand?
I don't think package-update should switch from a built-in package to a
package that was installed from ELPA. The user should at least once
commit to the switch (be it with a separate command or package-install).
>> >> It might work but it should be tested somewhat thoroughly before the
>> >> patch is applied. In the meantime, I just finished a similar approach
>> >> that does not modify `package-installed-p', but just adds another
>> >> utility function:
>> >
>> > A new utility function is fine by me, even if this is e branch. But I
>> > don't quite understand how this is supposed to work in package-install
>> > to allow updating built-in packages, and do that in a way that will
>> > not touch the existing code for non-built-in packages in significant
>> > ways (assuming you propose this from the release branch). Can you
>> > elaborate on that?
>>
>> The only reason we couldn't install built-in packages is that when
>> planning to install packages `package-compute-transaction' believes that
>> if a built-in package is provided, then everything is fine and we don't
>> need to proceed with installing any packages. All I propose is to lift
>> this assumption, then this works fine.
>
> My problem is _how_ to lift this assumption. The way you propose
> doing that affects updating non-core packages in ways that we will not
> have enough time to test well enough, not compared to the year that
> these commands exist in package.el and were used by many people in
> many cases. So we have the following alternatives for the way
> forward:
>
> . install your changes on master only, and leave the problem of
> updating a core package unsolved in Emacs 29 (with the workaround
> mentioned in the beginning of this bug's discussion available to
> alleviate the problem to some extent)
> . come up with safer changes for package-install that could be
> installed on emacs-29
> . add a new command for updating a core package, which can then be
> safely installed on emacs-29
>
> The last 2 alternatives can be for emacs-29 only, whereas on master we
> install your proposed change (or something else).
I would like to investigate option 2, but if nothing is found we can
fall back to 3. But even if there are issues in this case, I don't
consider the matter in general to be that drastic. If all Joao wants is
to avoid confusion, we can also improve the error message that
`package-install' generates when it says that a package ins already
installed.
> For the 2nd alternative above to be acceptable, the added/modified
> code must be completely separate from the code we have now, so that
> any possibility of its destabilizing the current code could be
> eliminated. It could be a separate code, triggered by the prefix
> argument, for example.
OK.
>> One point that might be deliberated is that this means all built-in
>> dependencies are also installed, even if these are not strictly
>> necessary. It shouldn't matter that much, since most users would
>> upgrade them eventually, but worth mentioning I guess?
>
> That just confirms my fears that we are opening a Pandora's box. We
> have no idea what this will do, and no time to test the results.
> Unintended consequences are abundant. We must draw any such
> consequences to the absolute minimum, at least the way the commands
> work by default. Even if the result is less than elegant.
Intuitively I would want to argue that this change has an upper-bound to
how much harm it could do, but as I cannot prove it in any way I'll
rather not assert that the point.
>> >> +(defun package-core-p (package)
>> >> + "Return non-nil the built-in version of PACKAGE is loaded."
>> >
>> > Didn't you say the "core" terminology was confusing people?
>>
>> TBH I am not really satisfied with the name (so any other suggestion is
>> just as fine for me), and as Joao said it would be better to make the
>> predicate as internal so that users are not expected to deal with it.
>
> The name should still be self-explanatory enough, because we the Emacs
> maintainers will read this code and should be able to understand what
> the function does without reading is source every time.
OK.
>>
>> >> + (let ((package (if (package-desc-p package)
>> >> + (package-desc-name package)
>> >> + package)))
>> >> + (and (assq package (package--alist))
>> >> + (package-built-in-p package))))
>> >
>> > It sounds like this doesn't check whether a package is "core", it
>> > checks whether it's built-in and can be updated? So maybe the name
>> > should be changed to reflect that? And the doc string as well (what
>> > it means by "is loaded")?
>>
>> Right the "loaded" doesn't make sense. How about this:
>>
>> +(defun package--upgradable-built-in-p (package)
>> + "Check if a built-in PACKAGE can be upgraded.
>> +This command differs from `package-built-in-p' in that it only
>> +returns a non-nil value if the user has not installed a more
>> +recent version of the package from a package archive."
>
> Note that what the doc string says is not what the name tells us.
> "Upgradeable" and "user has not installed a more recent version" are
> not the same. What the doc string says calls for a name like
> package--built-in-and-up-to-date or something.
>
>> + (and (not (assq (cond
>> + ((package-desc-p package)
>> + (package-desc-name package))
>> + ((stringp package) (intern package))
>> + ((symbolp package) package)
>> + ((error "Unknown package format: %S" package)))
>> + (package--alist)))
>> + (package-built-in-p package)))
>
> Why do we need all these conditions, where we didn't need them in the
> current code?
Practically speaking these conditions are not necessary, I just added it
in case there was the need to use the function elsewhere at some point.
> Also, package-alist is documented as "alist of all packages available
> for activation", so it is not clear how the fact that assq returns nil
> is evidence that "the user has not installed a more recent version".
> Looking at what package--alist and package-load-all-descriptors do, it
> looks like they just collect packages that were downloaded into the
> relevant directories? Is that enough to consider any package not in
> the list to be "not installed"?
The point is that package.el will add all packages it manages (ie. are
"available to activation") to package-alist. Built-in/core packages are
not managed by package.el, but just "acknowledged" via
`package--builtins'. So this looks like a safe assumption to me.
> And what about the "more recent
> version" condition -- this doesn't seem to be tested anywhere in
> package--alist?
You are right, but this is a misphrasing.
> The above questions and undocumented subtleties is what scares me in
> installing such changes at this late stage. I'm not sure everyone
> involved, yourself included, have a clear understanding of what the
> modified code will do in each possible use case. That is why I very
> much prefer separate code, which will then free us from the need of
> considering all these subtleties, as the last year of user's
> experience with this code can vouch that it does its job correctly, by
> and large.
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, (continued)
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Philip Kaludercic, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Philip Kaludercic, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Philip Kaludercic, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/15
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Philip Kaludercic, 2023/04/16
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, João Távora, 2023/04/16
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Kévin Le Gouguec, 2023/04/16
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot,
Philip Kaludercic <=
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/13
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Dmitry Gutov, 2023/04/12
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/12
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, João Távora, 2023/04/12
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Dmitry Gutov, 2023/04/12
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Dmitry Gutov, 2023/04/12
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/11
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, João Távora, 2023/04/11
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, Eli Zaretskii, 2023/04/12
- bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot, João Távora, 2023/04/12