From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.bugs 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 Message-ID: <87pm88kgj9.fsf@posteo.net> References: <87a5zj2vfo.fsf@gmail.com> <87ile2n0kn.fsf@gmail.com> <83v8i2abqi.fsf@gnu.org> <87wn2ilgx7.fsf@gmail.com> <83a5ze9uc1.fsf@gnu.org> <831qkq9rpy.fsf@gnu.org> <83pm898xb9.fsf@gnu.org> <87h6tlleg0.fsf@gmail.com> <8335558qc7.fsf@gnu.org> <83sfd5761f.fsf@gnu.org> <87zg7djrgr.fsf@gmail.com> <83o7nt73za.fsf@gnu.org> <83mt3d73c2.fsf@gnu.org> <87r0sptinq.fsf@posteo.net> <83jzyh706c.fsf@gnu.org> <875ya1tdwf.fsf@posteo.net> <83edop6sdy.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1967"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, 62720@debbugs.gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Apr 12 22:11:25 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pmgoP-0000Iy-3I for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 12 Apr 2023 22:11:25 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pmgo5-0004qE-Pk; Wed, 12 Apr 2023 16:11:05 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pmgo2-0004pV-P2 for bug-gnu-emacs@gnu.org; Wed, 12 Apr 2023 16:11:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pmgo2-0003ve-FU for bug-gnu-emacs@gnu.org; Wed, 12 Apr 2023 16:11:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pmgo1-0007bY-NH for bug-gnu-emacs@gnu.org; Wed, 12 Apr 2023 16:11:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Philip Kaludercic Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 12 Apr 2023 20:11:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 62720 X-GNU-PR-Package: emacs Original-Received: via spool by 62720-submit@debbugs.gnu.org id=B62720.168133023229192 (code B ref 62720); Wed, 12 Apr 2023 20:11:01 +0000 Original-Received: (at 62720) by debbugs.gnu.org; 12 Apr 2023 20:10:32 +0000 Original-Received: from localhost ([127.0.0.1]:41866 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pmgnX-0007al-4s for submit@debbugs.gnu.org; Wed, 12 Apr 2023 16:10:31 -0400 Original-Received: from mout01.posteo.de ([185.67.36.65]:35111) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pmgnT-0007aV-5Z for 62720@debbugs.gnu.org; Wed, 12 Apr 2023 16:10:29 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 4F9572402BB for <62720@debbugs.gnu.org>; Wed, 12 Apr 2023 22:10:21 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1681330221; bh=1nGiK8sILhLyJCLIxfBhjHH7wxSyBRQd7FTibJKuyac=; h=From:To:Cc:Subject:Autocrypt:Date:From; b=XJPqwIhHP4VkTYTnq5Pfjoou2/xXuR5oN0FVfP7bF3Qjk8HcKZhElpXGpLn1NmCqH eXQyM/r3Ry3aFJfcTfXQx9zczou8IhYACtbsKk7kfXTzb9FQu62ePTI902NVaX9AVe PqOE1cNWcYQ30fZwXvpOLeeP8q9O6mPANq8R20FqJj88KubDfNn+AcSxBGYD3OHf3w BMKHcIgPBsoTvAHHFYuRnhizd6K+j7Eh9HA8d99zceA6ELAw5c268e/B0q1QwbM/YQ q7wkTOgc1hQNi8dUdmgk/87Y7nblV86nyw8N72bJbaGfFxy3hsBe6+khhOfY1hsBZ5 FVduhWb5knSww== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4PxYjS3WKvz6twd; Wed, 12 Apr 2023 22:10:20 +0200 (CEST) In-Reply-To: <83edop6sdy.fsf@gnu.org> (Eli Zaretskii's message of "Wed, 12 Apr 2023 18:18:33 +0300") Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:259810 Archived-At: Eli Zaretskii writes: >> From: Philip Kaludercic >> 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.