[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: feature/package-vc has been merged
From: |
Stefan Monnier |
Subject: |
Re: feature/package-vc has been merged |
Date: |
Wed, 16 Nov 2022 10:23:55 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) |
> 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))))
>
> +(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.
> @@ -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?
> @@ -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ā„26 (or maybe 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).
> @@ -1080,9 +1089,10 @@ package-autoload-ensure-default-file
> (defvar autoload-timestamps)
> (defvar version-control)
>
> -(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.
> @@ -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?
> (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.
> @@ -2419,7 +2441,7 @@ package--newest-p
>
> (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%:
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.
> @@ -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.
Stefan
- Re: feature/package-vc has been merged, (continued)
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/09
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/09
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/09
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/09
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/09
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/09
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/09
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/09
- Re: feature/package-vc has been merged,
Stefan Monnier <=
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/16
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/16
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/16
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/16
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/16
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/16
- Re: feature/package-vc has been merged, Philip Kaludercic, 2022/11/17
- Re: feature/package-vc has been merged, Stefan Monnier, 2022/11/16
- Updating the "ELPA Protocol", Philip Kaludercic, 2022/11/09
- Re: Updating the "ELPA Protocol", Philip Kaludercic, 2022/11/15