[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: feature/package-vc has been merged
From: |
Philip Kaludercic |
Subject: |
Re: feature/package-vc has been merged |
Date: |
Wed, 16 Nov 2022 15:56:13 +0000 |
Stefan Monnier <monnier@iro.umontreal.ca> 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))))
>>
>> +(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ā„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).
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)
>>
>> -(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
>>
>> (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
- Re: feature/package-vc has been merged, (continued)
- 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, 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/16
- Re: feature/package-vc has been merged,
Philip Kaludercic <=
- 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
- Re: Updating the "ELPA Protocol", Stefan Kangas, 2022/11/15