Re: feature/package+vc 04c4c578c7 3/4: Allow for packages to be installed directly from VCS

[Philip Kaludercic]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> @@ -560,7 +571,9 @@ This is, approximately, the inverse of 
>>>> `version-to-list'.
>>>>  This is the name of the package with its version appended."
>>>>    (format "%s-%s"
>>>>            (package-desc-name pkg-desc)
>>>> -          (package-version-join (package-desc-version pkg-desc))))
>>>> +          (if (eq (package-desc-kind pkg-desc) 'source)
>>>> +              "devel"
>>>> +            (package-version-join (package-desc-version pkg-desc)))))
>>>
>>> Currently the way `package.el` handles the use of Git-installed packages
>>> is that it allows a package's directory to be just `<pkg>` instead of
>>> `<pkg>-<vers>`.
>>>
>>> So maybe we could do the same here.
>>
>> I guess the question here is if you want to make it explicit that
>> e.g. when removing a package with package-delete, you will be removing a
>> local checkout that might have modifications.  If not, it would probably
>> make sense to add warnings where necessary.
>
> I don't understand why using `<pkg>-devel` instead of `<pkg>` makes it
> easier to know that "you will be removing a local checkout that might
> have modifications"

The -devel doesn't necessarily indicate that directly, I just meant it
was worth somehow explicitly indicating that this is a VCS-package.

>>>> +(declare-function vc-working-revision "vc" (file &optional backend))
>>>> +(defun package-devel-commit (pkg)
>>>> +  "Extract the commit of a development package PKG."
>>>> +  (cl-assert (eq (package-desc-kind pkg) 'source))
>>>> +  (require 'vc)
>>>> +  (cl-loop with dir = (package-desc-dir pkg)
>>>> +           for file in (directory-files dir t "\\.el\\'" t)
>>>> +           when (vc-working-revision file) return it
>>>> +           finally return "unknown"))
>>>
>>> Any chance we could use `vc-working-revision` on the package's root
>>> directory to avoid this loop?
>>
>> If you pass the backend directly to vc-working-revision, and set the
>> file to the expanded file name of the current directory, it seems to
>> work (at least for git and SVN).  Would it make sense to add that as a
>> first step, then fall back to the loop?
>
> Since you say it works, yes.  We could even drop the loop altogether
> (and either not accept working with those VC backends where it doesn't
> work, or improve those backends which we do want to work).

As I presume that in most cases Git is used, and this appears to be
working with vc-git, this should be ok.

>>>> @@ -681,6 +704,14 @@ return it."
>>>>                               (read (current-buffer)))
>>>>                              (error "Can't find define-package in %s" 
>>>> pkg-file))))
>>>>            (setf (package-desc-dir pkg-desc) pkg-dir)
>>>> +          (when (file-exists-p (expand-file-name
>>>> +                                (symbol-name (package-desc-name pkg-desc))
>>>> +                                package-devel-dir))
>>>> +            ;; XXX: This check seems dirty, there should be a better
>>>> +            ;; way to deduce if a package is in the devel directory.
>>>> +            (setf (package-desc-kind pkg-desc) 'source)
>>>> +            (push (cons :commit (package-devel-commit pkg-desc))
>>>> +                  (package-desc-extras pkg-desc)))
>>>>            (if (file-exists-p signed-file)
>>>>                (setf (package-desc-signed pkg-desc) t))
>>>>            pkg-desc)))))
>>>
>>> Hmm... why do we need to know if a package is in the devel directory?
>>
>> Currently the :kind is not stored, so we have to infer it somehow.  I am
>> certain this could also be solve some other way, but I would also like
>> to allow for simply placing a directory into elpa/devel to be handled as
>> a package, as before with my site-lisp.el proposal.
>
> IIUC for those VCS-installed packages we generate the <pkg>-pkg.el file
> ourselves, so it should be fairly easy to make sure that file provides
> the needed info so we don't need to guess.

Yes, but again, if I just clone a project into elpa/devel, this won't
work.  It could be argued that this shouldn't be supported because it is
a different issue, and the elpa/... directories maintained by
package.el, but in that case I would like some other way to have
package.el manage non-versioned local source.

>>>> +         ;; In case the package was installed directly from source, the
>>>> +         ;; dependency list wasn't know beforehand, and they might have
>>>> +         ;; to be installed explicitly.
>>>> +         (let (deps)
>>>> +           (dolist (file (directory-files pkg-dir t "\\.el\\'" t))
>>>> +             (with-temp-buffer
>>>> +               (insert-file-contents file)
>>>> +               (when-let* ((require-lines (lm-header-multiline 
>>>> "package-requires")))
>>>> +                 (thread-last
>>>> +                   (mapconcat #'identity require-lines " ")
>>>> +                   package-read-from-string
>>>> +                   package--prepare-dependencies
>>>> +                   (nconc deps)
>>>> +                   (setq deps)))))
>>>
>>> Hmm... I expected here we'd open the `<pkg>.el` file and call
>>> `package-buffer-info`.
>>
>> That was my first thought too, but are we always certain that <pkg>.el
>> is the main file?
>
> For the rare cases where this is not the case, the `:main-file` property
> of the package spec should tell us which file it is.  I'd rather not try
> and guess.

Ok, I didn't know that :main-file was propagated into the package
specification.  In that case there should be no issue.

>> That would also be possible, I believe I initially assumed that :vc
>> would contain the contents of the header, and forgot to revise that
>> assumption later on.
>
> I think making it hold the package spec (as given in `elpa-packages`)
> would be a natural starting point.

Ok.

-- 
        Philip Kaludercic