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 15:56:13 +0000 Message-ID: <87mt8qnbaa.fsf@posteo.net> References: <164484721900.31751.1453162457552427931@vcs2.savannah.gnu.org> <87leoond7l.fsf@posteo.net> <83r0yfzz01.fsf@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> 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="23233"; 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 16:56:49 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 1ovKmO-0005nP-7P for ged-emacs-devel@m.gmane-mx.org; Wed, 16 Nov 2022 16:56:48 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ovKm0-0008H2-Rx; Wed, 16 Nov 2022 10:56:24 -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 1ovKlz-0008EZ-D4 for emacs-devel@gnu.org; Wed, 16 Nov 2022 10:56:23 -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 1ovKlv-0004ZL-MB for emacs-devel@gnu.org; Wed, 16 Nov 2022 10:56:23 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id B144524002B for ; Wed, 16 Nov 2022 16:56:13 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1668614173; bh=8rXsUxkTEv75NDyIDrEiJJuaMG2HSCqKMI3LsgaX7fc=; h=From:To:Cc:Subject:Date:From; b=DaimHmtzqCCNobHR+4mROEbFB/HXVwolx7XoOEBCUytrZBw7WvwQSFng4uuxgIffu hGmx8CaFEnl4mdhnAUsHhNWMpau0dHM8v8spzekm/95EnM9Zt1FUAZGRbtLx3B/pvo O62rIUilp4f0EWgsM5Ip5ASPq4IrEU/N6S7caoZTVpxpLxqqWfg3HpH7IewM31/dMJ QQvwLEHXw7K6R6Pa8Iv1pvSfd6fJF7yfURt+zJVJ3p5v/6CV9ssgyKiuClHFRMcAIA bGWL82tfBMZjRU1jIhq/V4KpYO+/gr/9fW75zCffLkpLQ0giAHcQhCVsPB7ZG83dNK Dy2ULrx2gVwyg== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NC7244zwpz6tmB; Wed, 16 Nov 2022 16:56:12 +0100 (CET) In-Reply-To: (Stefan Monnier's message of "Wed, 16 Nov 2022 10:23:55 -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:299944 Archived-At: Stefan Monnier writes: >> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el >> index a7bcdd214c..bf6849af65 100644 >> --- a/lisp/emacs-lisp/package.el >> +++ b/lisp/emacs-lisp/package.el >> @@ -462,6 +462,15 @@ package-vc-p >> (inline-letevals (pkg-desc) >> (inline-quote (eq (package-desc-kind ,pkg-desc) 'vc)))) >>=20=20 >> +(define-inline package-lisp-dir (pkg-desc) >> + "Return the directory with Lisp files for PKG-DESC." >> + (inline-letevals (pkg-desc) >> + (inline-quote >> + (let* ((extras (package-desc-extras ,pkg-desc)) >> + (lisp-dir (alist-get :lisp-dir extras)) >> + (dir (package-desc-dir ,pkg-desc))) >> + (file-name-directory (file-name-concat dir lisp-dir)))))) > > Any reason why this needs to be inlined? I'd expect the inlining to > have a negligible effect since the body contains its fair share of > further function calls, none of which are conditional and AFAICT all the > calls to this pass a variable as argument, so the inlining will not > expose further optimization opportunities. You are probably right, it would be better to convert this into a regular function. >> @@ -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. >> @@ -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 may= be 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. >> @@ -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. If you think it is better, I could rename the function and add a compatibility alias. >> @@ -1118,7 +1140,7 @@ package--compile >> (let ((byte-compile-ignore-files (package--parse-elpaignore pkg-desc)) >> (warning-minimum-level :error) >> (load-path load-path)) >> - (byte-recompile-directory (package-desc-dir pkg-desc) 0 t))) >> + (byte-recompile-directory (package-lisp-dir pkg-desc) 0 t))) > > 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. >> (defun package--native-compile-async (pkg-desc) >> "Native compile installed package PKG-DESC asynchronously. >> @@ -1126,7 +1148,7 @@ package--native-compile-async >> `package-activate-1'." >> (when (native-comp-available-p) >> (let ((warning-minimum-level :error)) >> - (native-compile-async (package-desc-dir pkg-desc) t)))) >> + (native-compile-async (package-lisp-dir pkg-desc) t)))) > > Same here. Same here. >> @@ -2419,7 +2441,7 @@ package--newest-p >>=20=20 >> (declare-function comp-el-to-eln-filename "comp.c") >> (defvar package-vc-repository-store) >> -(defun package--delete-directory (dir pkg-desc) >> +(defun package--delete-directory (dir) >> "Delete PKG-DESC directory DIR recursively. >> Clean-up the corresponding .eln files if Emacs is native >> compiled." > > 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: - (if (and (package-vc-p pkg-desc) - (require 'package-vc) ;load `package-vc-repository-store' - (file-in-directory-p dir package-vc-repository-store)) - (progn - (delete-directory - (expand-file-name - (car (file-name-split - (file-relative-name dir package-vc-repository-store))) - package-vc-repository-store) - t) - (delete-file (directory-file-name dir))) - (delete-directory dir t))) + (delete-directory dir t)) Now that this isn't being done anymore, I could revert the change and simplify the code back to the original state (actually it has become slightly more complicated since, because I noticed that `package-vc-install-from-checkout' requires some special handling, but that doesn't require a package description to be detected). > Packages installed with `package-install` can be deleted without too > much fuss because they usually don't contain any important info, but for > `package-vc` this is completely different and we should either never > delete them or at least not without a minimum of sanity checks > (uncommited changes, stashes, additional branches) and very > explicit prompt. 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. >> @@ -2549,7 +2560,7 @@ package-recompile >> ;; load them (in case they contain byte code/macros that are now >> ;; invalid). >> (dolist (elc (directory-files-recursively >> - (package-desc-dir pkg-desc) "\\.elc\\'")) >> + (package-lisp-dir pkg-desc) "\\.elc\\'")) >> (delete-file elc)) >> (package--compile pkg-desc))) > > Same as above. The explanation remains the same. > > Stefan