Re: feature/package-vc has been merged

[Stefan Monnier]

>>> @@ -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?

>>> @@ -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.

Removing it would is indeed a decision unrelated to `package-vc`.
But in the mean time you can simply not change that code.

>>> @@ -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.

No, I mean that the change should not be needed (and hence the change
in signature shouldn't be needed either).

>> 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.


        Stefan