Re: feature/package-vc has been merged

[Philip Kaludercic]

Stefan Monnier <monnier@iro.umontreal.ca> 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≥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.

OK, I'll revert that change.

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

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.