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.devel Subject: Re: feature/package-vc has been merged Date: Wed, 16 Nov 2022 17:57:14 +0000 Message-ID: <87o7t6lr45.fsf@posteo.net> References: <164484721900.31751.1453162457552427931@vcs2.savannah.gnu.org> <87bkpjyx3p.fsf@posteo.net> <83bkpjynmj.fsf@gnu.org> <87iljqya44.fsf@posteo.net> <8335auzo9s.fsf@gnu.org> <87zgd2ws8z.fsf@posteo.net> <831qqezkxj.fsf@gnu.org> <87y1slgq3m.fsf@posteo.net> <87bkpgfsqv.fsf@posteo.net> <87educ9fei.fsf@posteo.net> <8735as9cfb.fsf@posteo.net> <87pmdv98du.fsf@posteo.net> <87zgcz7qyy.fsf@posteo.net> <87mt8qnbaa.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="33393"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Eli Zaretskii , rms@gnu.org, emacs-devel@gnu.org, Lars Ingebrigtsen To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Nov 16 18:58:19 2022 Return-path: Envelope-to: ged-emacs-devel@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 1ovMfy-0008MG-Vp for ged-emacs-devel@m.gmane-mx.org; Wed, 16 Nov 2022 18:58:19 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ovMfQ-0004ok-O9; Wed, 16 Nov 2022 12:57:44 -0500 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 1ovMfO-0004oX-5l for emacs-devel@gnu.org; Wed, 16 Nov 2022 12:57:42 -0500 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ovMfL-0004pZ-Ts for emacs-devel@gnu.org; Wed, 16 Nov 2022 12:57:41 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id E2ED9240027 for ; Wed, 16 Nov 2022 18:57:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1668621442; bh=ztWyx01SMWZBZM4J/GIPyerO7AgtOYthtoDZeWgxSus=; h=From:To:Cc:Subject:Date:From; b=S7lFPKWlqz2iTZ0bd15rlKxgOC7r40xYsYI5Pu7nFapmK4Bq3dC9KkCsI5fPfM9pu 2IH5PbblOGE2IvudKPDts+6USFp9i1/Ei8FGetgs3lI4fs1/2tPfMsDilw9QUAIOpX EDTQKx5mQ48xAaYdqoe82GNpNInNlh4pfYytfiRFbd7f2I/p/8jUNLY8k/TX9akU5V s7FXvM0y5YTIGxKrCo9sn2TTp7XbXhc+CxRBQFv30UlXoga75+AXodpZZKYtseX76D QFItLnm2QBvEHs3hQYrNuSwVrIOQxdsMM5kNeiry9X5mOJ4F9x0tp2Q9A/h4+wRjKI DKmJSCs4klxcg== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NC9jp1Swpz9rxG; Wed, 16 Nov 2022 18:57:18 +0100 (CET) In-Reply-To: (Stefan Monnier's message of "Wed, 16 Nov 2022 12:29:01 -0500") Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:299965 Archived-At: Stefan Monnier writes: >>>> @@ -827,7 +836,7 @@ package--reload-previously-loaded >>>> byte-compilation of the new package to fail." >>>> (with-demoted-errors "Error in package--load-files-for-activation: = %s" >>>> (let* (result >>>> - (dir (package-desc-dir pkg-desc)) >>>> + (dir (package-lisp-dir pkg-desc)) >>>> ;; A previous implementation would skip `dir' itself. >>>> ;; However, in normal use reloading from the same directory >>>> ;; never happens anyway, while in certain cases external to >>> >>> Why do we need this change? >> >> Because we are only interested in the sub-directory containing the lisp >> files. > > But is there any harm in considering the whole parent directory instead? As mentioned below, I think the harm is that unintended error could appear. But I get your argument too, that mistakes should be fixed in general and having these pop up during byte compilation is a good way to make these more noticeable... >>>> @@ -891,7 +900,7 @@ package-activate-1 >>>> (package--reload-previously-loaded pkg-desc)) >>>> (with-demoted-errors "Error loading autoloads: %s" >>>> (load (package--autoloads-file-name pkg-desc) nil t)) >>>> - (add-to-list 'load-path (directory-file-name pkg-dir))) >>>> + (add-to-list 'load-path (package-lisp-dir pkg-desc))) >>>> ;; Add info node. >>>> (when (file-exists-p (expand-file-name "dir" pkg-dir)) >>>> ;; FIXME: not the friendliest, but simple. >>> >>> This should really not be needed (actually, this `add-to-list` is not >>> needed at all for any package (re)installed with Emacs=E2=89=A526 (or m= aybe even >>> 25, can't remember)). The "normal" behavior is that it's the autoloads >>> file which adds to `load-path` (which makes it possible for that >>> autoloads file to add the `:lisp-dir` instead of the root dir, indeed). >> >> I see what you mean. But this would be change that is unrelated to >> package-vc, so it could just be removed directly on master. > > Removing it would is indeed a decision unrelated to `package-vc`. > But in the mean time you can simply not change that code. OK, I'll revert that change. >>>> @@ -1080,9 +1089,10 @@ package-autoload-ensure-default-file >>>> (defvar autoload-timestamps) >>>> (defvar version-control) >>>>=20=20 >>>> -(defun package-generate-autoloads (name pkg-dir) >>>> - "Generate autoloads in PKG-DIR for package named NAME." >>>> - (let* ((auto-name (format "%s-autoloads.el" name)) >>>> +(defun package-generate-autoloads (pkg-desc pkg-dir) >>>> + "Generate autoloads for PKG-DESC in PKG-DIR." >>>> + (let* ((name (package-desc-name pkg-desc)) >>>> + (auto-name (format "%s-autoloads.el" name)) >>>> ;;(ignore-name (concat name "-pkg.el")) >>>> (output-file (expand-file-name auto-name pkg-dir)) >>>> ;; We don't need 'em, and this makes the output reproducible. >>> >>> I thought an alternative was for `package-vc.el` to call this function >>> with the `:lisp-dir` as `pkg-dir`, so we don't need to change this part >>> of the code. >> >> I might be missing something, but the previous signature was missing a >> package description object that the change required. > > No, I mean that the change should not be needed (and hence the change > in signature shouldn't be needed either). If there is any place where :lisp-dir this is needed, then here, because this is the place where the auto-load is generated containing the `load-path' modification. If I don't have the package description, then I cannot infer the right sub-directory. >>> Why do we need this? AFAIK this recurses into subdirectories so using >>> `package-desc-dir` still compiles all the files just fine, no? >> Same as above, if we know all the lisp code is located in one >> sub-directory, there is no need to compile everything in the >> directory -- which might contain test files or other scripts that were >> not meant to be compiled. > > In elpa-admin.el I compile all those ancillary files as well. > I'm not sure we should refrain from compiling them, actually. > [ But yes: their compilation will fail more often. I consider it > a problem in the packages. ] > >>> IIUC this part of the change is because `package-delete` does not delete >>> package-vc packages, right? If so, I support that 100%: >> >> This change reverts a previous feature, back when package-vc didn't load >> sub-directories, but cloned the repository into some XDG directory and >> created a symbolic link from ~/.emacs.d/elpa to the right sub-directory. >> To do that, knowledge of what package was been deleted was required: > > Good, thanks. > >> This is currently not done, but could be added. I am imagining checking >> of the package directory is the root directory of a repository (does >> that work for all VCS?), and if so double-prompting the user. But I >> would be opposed to preventing users from deleting packages installed >> using `package-vc', if only it would go against my workflow of fetching >> the sources for a package, preparing and sending a patch, and then >> reverting to the release package. > > I'm not saying we should prevent it, but just that we should be careful > to only delete with the full, express, and informed consent of the users. I agree.