From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii 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 18:18:33 +0300 Message-ID: <83edop6sdy.fsf@gnu.org> References: <87a5zj2vfo.fsf@gmail.com> <87y1mz38rl.fsf@posteo.net> <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> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1695"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, 62720@debbugs.gnu.org, joaotavora@gmail.com, monnier@iro.umontreal.ca To: Philip Kaludercic Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Apr 12 17:18:35 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 1pmcF1-00006t-AU for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 12 Apr 2023 17:18:35 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pmcEW-0000OG-Qe; Wed, 12 Apr 2023 11:18:04 -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 1pmcEV-0000Nl-EC for bug-gnu-emacs@gnu.org; Wed, 12 Apr 2023 11:18: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 1pmcEV-0001xA-6k for bug-gnu-emacs@gnu.org; Wed, 12 Apr 2023 11:18:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pmcEV-0004QV-2a for bug-gnu-emacs@gnu.org; Wed, 12 Apr 2023 11:18:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 12 Apr 2023 15:18:03 +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.168131268116994 (code B ref 62720); Wed, 12 Apr 2023 15:18:03 +0000 Original-Received: (at 62720) by debbugs.gnu.org; 12 Apr 2023 15:18:01 +0000 Original-Received: from localhost ([127.0.0.1]:40627 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pmcES-0004Q1-R2 for submit@debbugs.gnu.org; Wed, 12 Apr 2023 11:18:01 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:55078) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pmcEP-0004Pg-V4 for 62720@debbugs.gnu.org; Wed, 12 Apr 2023 11:17:59 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pmcEI-0001vS-Rq; Wed, 12 Apr 2023 11:17:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=ciJStzjh4t4XUgv+jshcr74BNpflzzqWyEcLB9sqwd8=; b=jY7RVhgU+gtZ 2WlZIFMOOGqusSKJdEEiLRgHSyZr7RrYzay0V7W9DuJBRIF7RAjuAV+4FGyL3ObqhPR37sHAbR62j khHWCp5GGElegfrZes6/FGA9HaIwbYI1kJ+m8eTz/z5d1ihXa3B0hFvnf+lRviyaXiGmchjckETRR MncegFB3XDjI/wEf4XNvyUKTLB1DC59c5957vbVaP5YXlW3VwywmlrxhOEN6zBGyaEgCYB6/DZp7W iw+8/So5v0W/u/hLiCAFRDO25X1fZz8Jx2inZuArJfD/8GWkGAPDRXPqmc6s2tca7ELaVHrNgVCUc KYZHvv3ptyKI1lYowhwnVA==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pmcEI-0001cE-0b; Wed, 12 Apr 2023 11:17:50 -0400 In-Reply-To: <875ya1tdwf.fsf@posteo.net> (message from Philip Kaludercic on Wed, 12 Apr 2023 13:42:56 +0000) 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:259757 Archived-At: > 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. > > 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. > 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? > >> 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). 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. > 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. > >> +(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. > > >> + (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? 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"? And what about the "more recent version" condition -- this doesn't seem to be tested anywhere in package--alist? 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.