bug#15108: 24.3.50; Package dependency documentation

bug#15108: 24.3.50; Package dependency documentation

[Tom Willemse]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

I noticed that the "Packaging Basics" section in the Emacs Lisp manual
suggests that version numbers in `Package-Requires' headers are
optional, however the explanation of `Package-Requires' makes it clear
that the version specifier is necessary and testing confirms this.

The values `foo', `(foo)' and `((foo))' all cause an error to be
signaled by `package-buffer-info'. This seems to me that there is no
"possibly" about it.

It's only a tiny change and I hope it's OK. I have tried to follow the
guidelines found in the Emacs manual about sending patches, I didn't
find any reference about diffs of multiple files, so I hope it's OK to
send the diffs separately.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ChangeLog diff --]
[-- Type: text/x-diff, Size: 435 bytes --]

*** ChangeLog	2013-08-16 00:30:03.814759000 +0200
--- ChangeLog.new	2013-08-16 00:29:31.431138190 +0200
***************
*** 1,3 ****
--- 1,8 ----
+ 2013-08-15  Tom Willemse  <tom@ryuslash.org>
+ 
+ 	* package.texi (Packaging Basics): The minimal acceptable version
+ 	numbers specified in a package's dependencies are not optional.
+ 
  2013-08-15  Xue Fuqiao  <xfq.free@gmail.com>
  
  	* markers.texi (The Region): Add/move indexes.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: package.texi diff --]
[-- Type: text/x-diff, Size: 1025 bytes --]

*** package.texi	2013-08-16 00:30:09.127486000 +0200
--- package.texi.new	2013-08-16 00:29:44.384571450 +0200
***************
*** 68,78 ****
  once it is installed.
  
  @item Dependencies
! A list of other packages (possibly including minimal acceptable
! version numbers) on which this package depends.  The list may be
! empty, meaning this package has no dependencies.  Otherwise,
! installing this package also automatically installs its dependencies;
! if any dependency cannot be found, the package cannot be installed.
  @end table
  
  @cindex content directory, package
--- 68,78 ----
  once it is installed.
  
  @item Dependencies
! A list of other packages (including minimal acceptable version
! numbers) on which this package depends.  The list may be empty,
! meaning this package has no dependencies.  Otherwise, installing this
! package also automatically installs its dependencies; if any
! dependency cannot be found, the package cannot be installed.
  @end table
  
  @cindex content directory, package

bug#15108: 24.3.50; Package dependency documentation

[Stefan Monnier]

> The values `foo', `(foo)' and `((foo))' all cause an error to be
> signaled by `package-buffer-info'. This seems to me that there is no
> "possibly" about it.

Indeed a lot of the code assumes that there's a version number in there,
but some of the code does try to handle a nil value as well.
I think it would be desirable to make it possible to leave the version
number unspecified.


        Stefan

bug#15108: 24.3.50; Package dependency documentation

[Drew Adams]

> > The values `foo', `(foo)' and `((foo))' all cause an error to be
> > signaled by `package-buffer-info'. This seems to me that there is no
> > "possibly" about it.
> 
> Indeed a lot of the code assumes that there's a version number in there,
> but some of the code does try to handle a nil value as well.
> I think it would be desirable to make it possible to leave the version
> number unspecified.

+1; a BIG +1.

You can use `Package Requires ((foo "0"))' as a workaround, but it's
silly that you have to do that.  If a library is not versioned, or if
for some reason any version of it will do for the package that requires
it, it should be possible to use just `Package Requires ((foo))'.

Furthermore, if no library is required, it should be possible to omit
a `Package-Requires' altogether - that should be equivalent to
`Package-Requires ()'.

bug#15108: 24.3.50; Package dependency documentation

[Tom Willemse]

Drew Adams <drew.adams@oracle.com> writes:

>> > The values `foo', `(foo)' and `((foo))' all cause an error to be
>> > signaled by `package-buffer-info'. This seems to me that there is no
>> > "possibly" about it.
>> 
>> Indeed a lot of the code assumes that there's a version number in there,
>> but some of the code does try to handle a nil value as well.
>> I think it would be desirable to make it possible to leave the version
>> number unspecified.
>
> +1; a BIG +1.

I could look into that if you guys like. Any pointers on where to look
apart from `package-buffer-info'?

> You can use `Package Requires ((foo "0"))' as a workaround, but it's
> silly that you have to do that.  If a library is not versioned, or if
> for some reason any version of it will do for the package that requires
> it, it should be possible to use just `Package Requires ((foo))'.

Would values of either `foo' for just a single package dependency, `(foo
bar)' for packages without version dependencies and `(foo (baz "1.1.0")
bar)' for mixed values be OK? That would be my preference, but perhaps
this is to complex.

> Furthermore, if no library is required, it should be possible to omit
> a `Package-Requires' altogether - that should be equivalent to
> `Package-Requires ()'.

This is already possible isn't it? Most of my packages don't require
anything and as such don't have a `Package-Requires' header.

bug#15108: 24.3.50; Package dependency documentation

[Drew Adams]

> > You can use `Package Requires ((foo "0"))' as a workaround, but it's
> > silly that you have to do that.  If a library is not versioned, or if
> > for some reason any version of it will do for the package that requires
> > it, it should be possible to use just `Package Requires ((foo))'.
> 
> Would values of either `foo' for just a single package dependency, `(foo
> bar)' for packages without version dependencies and `(foo (baz "1.1.0")
> bar)' for mixed values be OK? That would be my preference, but perhaps
> this is to complex.

Are you asking to be able to use (foo (baz "1.1.0") bar) as an
alternative to ((foo) (baz "1.1.0") (bar))?

IOW, `foo' instead of `(foo)' or (foo "0")?  Sounds OK to me.  Maybe
someone else knows a reason why that would not work or be a good idea.

> > Furthermore, if no library is required, it should be possible to omit
> > a `Package-Requires' altogether - that should be equivalent to
> > `Package-Requires ()'.
> 
> This is already possible isn't it? Most of my packages don't require
> anything and as such don't have a `Package-Requires' header.

Dunno.  I ended up adding `Package-Requires' everywhere, because
it wasn't clear to me that its absence means `Package-Requires ()', in
practice.

That said, an explicit `Package-Requires ()' makes it clear that there
is no dependency, whereas if it is absent that could just mean that
no one looked at the question.

Nevertheless, optional is better, IMO.  Leave it up to the programmer
to decide whether to be crystal clear by adding an explicit empty spec.

bug#15108: 24.3.50; Package dependency documentation

[Tom Willemse]

Drew Adams <drew.adams@oracle.com> writes:

>> > You can use `Package Requires ((foo "0"))' as a workaround, but it's
>> > silly that you have to do that.  If a library is not versioned, or if
>> > for some reason any version of it will do for the package that requires
>> > it, it should be possible to use just `Package Requires ((foo))'.
>> 
>> Would values of either `foo' for just a single package dependency, `(foo
>> bar)' for packages without version dependencies and `(foo (baz "1.1.0")
>> bar)' for mixed values be OK? That would be my preference, but perhaps
>> this is to complex.
>
> Are you asking to be able to use (foo (baz "1.1.0") bar) as an
> alternative to ((foo) (baz "1.1.0") (bar))?
>
> IOW, `foo' instead of `(foo)' or (foo "0")?  Sounds OK to me.  Maybe
> someone else knows a reason why that would not work or be a good idea.

It's not that I desperately want to use that, it just seems cleaner to
me to be able to omit parentheses when they're not needed. But if
someone else knows a reason not to do it, or if everyone else just
thinks it's messy or complicated, that would be reasons not to look into
that direction. I'm asking for opinions on the idea that I had.

And yes, you seem to understand my meaning correctly. In case anyone
else didn't understand I'll try to illustrate:

    ;; Package-Requires: foo

Would just be a dependency on any version of `foo',

    ;; Package-Requires: (foo bar)

Would be a dependency on any version of both `foo' and `bar',

   ;; Package-Requires: (foo (baz "1.1.0") bar)

Would be a dependency on any version of both `foo' and `bar' and version
1.1.0 of `baz'.

A single package with a version could also be an option, like so:

    ;; Package-Requires: (baz "1.1.0")

bug#15108: More flexible package dependency specification

[Tom Willemse]

[-- Attachment #1: Type: text/plain, Size: 165 bytes --]

What would you think of the attached patch? It works for all the
examples I sent earlier. Any comments, criticisms and considerations
would be greatly appreciated.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ChangeLog diff --]
[-- Type: text/x-diff, Size: 592 bytes --]

*** lisp/ChangeLog	2013-08-16 22:53:18.590328000 +0200
--- lisp/ChangeLog.new	2013-08-16 22:45:04.548991805 +0200
***************
*** 1,3 ****
--- 1,11 ----
+ 2013-08-16  Tom Willemse  <tom@ryuslash.org>
+ 
+ 	* emacs-lisp/package.el (package--prepare-dependencies): New
+ 	function.
+ 	(package-buffer-info): Call `package--prepare-dependencies' on the
+ 	list contained in the package-requires header to make dependency
+ 	specification a little more flexible.
+ 
  2013-08-16  Lars Magne Ingebrigtsen  <larsi@gnus.org>
  
  	* net/shr.el (shr-rescale-image): Use ImageMagick even for GIFs

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: package.el diff --]
[-- Type: text/x-diff, Size: 1487 bytes --]

*** lisp/emacs-lisp/package.el	2013-08-16 22:53:18.590328000 +0200
--- lisp/emacs-lisp/package.el.new	2013-08-16 22:52:51.479137603 +0200
***************
*** 973,978 ****
--- 973,989 ----
  	    str)
        (error nil))))
  
+ (defun package--prepare-dependencies (deps)
+   "Turn DEPS into an acceptable list of dependencies.
+ 
+ Any parts missing a version string get a default version string
+ of \"0\" (meaning any version) and an appropriate level of lists
+ is wrapped around any parts requiring it."
+   (cond
+    ((symbolp deps) `((,deps "0")))
+    ((stringp (cadr deps)) `(,deps))
+    (t (mapcar (lambda (dep) (if (symbolp dep) `(,dep "0") dep)) deps))))
+ 
  (defun package-buffer-info ()
    "Return a `package-desc' describing the package in the current buffer.
  
***************
*** 1003,1009 ****
  	 "Package lacks a \"Version\" or \"Package-Version\" header"))
        (package-desc-from-define
         file-name pkg-version desc
!        (if requires-str (package-read-from-string requires-str))
         :kind 'single))))
  
  (declare-function tar-get-file-descriptor "tar-mode" (file))
--- 1014,1022 ----
  	 "Package lacks a \"Version\" or \"Package-Version\" header"))
        (package-desc-from-define
         file-name pkg-version desc
!        (if requires-str
!            (package--prepare-dependencies
!             (package-read-from-string requires-str)))
         :kind 'single))))
  
  (declare-function tar-get-file-descriptor "tar-mode" (file))

bug#15108: [Patch] Updated package dependencies

[Tom Willemse]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

Hey,

I've been meaning to send this for a couple of weeks, but I was unable
to. I apologize.

A while ago the patch I sent last time broke, I fixed it and now here
I'm sending it again, hoping there is still some interest in this or
need for it. I've removed the changes to the ChangeLog (or at least, I'm
not sending them again), since I've lost confidence in it being my place
to do so.

I've tested it most recently bzr trunk revision 115470. All the ways to
specify dependencies work as described before:

;; Package-Requires: foo
;; Package-Requires: (foo bar)
;; Package-Requires: (foo (baz "1.1.0") bar)
;; Package-Requires: (baz "1.1.0")

These would all be valid after this patch.

As before, any comments, criticisms and considerations would be greatly
appreciated.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Package dependency patch --]
[-- Type: text/x-diff, Size: 1149 bytes --]

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el	2013-08-03 02:34:22 +0000
+++ lisp/emacs-lisp/package.el	2013-08-16 20:40:09 +0000
@@ -973,6 +973,17 @@
 	    str)
       (error nil))))

+(defun package--prepare-dependencies (deps)
+  "Turn DEPS into an acceptable list of dependencies.
+
+Any parts missing a version string get a default version string
+of \"0\" (meaning any version) and an appropriate level of lists
+is wrapped around any parts requiring it."
+  (cond
+   ((symbolp deps) `((,deps "0")))
+   ((stringp (cadr deps)) `(,deps))
+   (t (mapcar (lambda (dep) (if (symbolp dep) `(,dep "0") dep)) deps))))
+
 (defun package-buffer-info ()
   "Return a `package-desc' describing the package in the current buffer.

@@ -1003,7 +1014,9 @@
 	 "Package lacks a \"Version\" or \"Package-Version\" header"))
       (package-desc-from-define
        file-name pkg-version desc
-       (if requires-str (package-read-from-string requires-str))
+       (if requires-str
+           (package--prepare-dependencies
+            (package-read-from-string requires-str)))
        :kind 'single
        :url homepage))))

bug#15108: [Patch] Updated package dependencies

[Drew Adams]

> I've tested it most recently bzr trunk revision 115470. All the ways to
> specify dependencies work as described before:
> 
> ;; Package-Requires: foo
> ;; Package-Requires: (foo bar)
> ;; Package-Requires: (foo (baz "1.1.0") bar)
> ;; Package-Requires: (baz "1.1.0")

Those are not all of the ways.  Most of my libraries use this way:

;; Package-Requires: ()

And most of the others (of mine) use this way (perhaps covered by
your 3rd, perhaps not, depending on your spec):

;; Package-Requires: ((fit-frame "0"))

See also bug #14941 (still unanswered, so far).

bug#15108: Updated package dependencies

[Tom Willemse]

Hey Drew,

Drew Adams <drew.adams@oracle.com> writes:

>> I've tested it most recently bzr trunk revision 115470. All the ways to
>> specify dependencies work as described before:
>> 
>> ;; Package-Requires: foo
>> ;; Package-Requires: (foo bar)
>> ;; Package-Requires: (foo (baz "1.1.0") bar)
>> ;; Package-Requires: (baz "1.1.0")
>
> Those are not all of the ways.  Most of my libraries use this way:

Sorry, bad choice of words, never meant to say all the ways work, I
meant all the ways given in a previous mail.

> ;; Package-Requires: ()

This will cause a dependency on `((nil (0)))' right now, I will fix
that. May I ask why you wouldn't just leave the header out of the file
or leave it empty? Just out of curiosity, this should work the same way
as Emacs 24.3 I'm sure.

> And most of the others (of mine) use this way (perhaps covered by
> your 3rd, perhaps not, depending on your spec):
>
> ;; Package-Requires: ((fit-frame "0"))

This produces a dependency on `((fit-frame (0)))' with my patch
installed, according to `package-buffer-info'.

> See also bug #14941 (still unanswered, so far).

I will look at it, if you don't mind.

bug#15108: Updated package dependencies

[Drew Adams]

> > ;; Package-Requires: ()
> 
> This will cause a dependency on `((nil (0)))' right now, I will fix
> that. May I ask why you wouldn't just leave the header out of the file
> or leave it empty? Just out of curiosity, this should work the same way
> as Emacs 24.3 I'm sure.

From correspondence with people at MELPA, `Package-Requires' seems to be
required.  And being present with an empty list tells human readers that
the package has no dependencies.  (If absent, you don't get that explicit
info, and you might be wrong in assuming it.)

> > And most of the others (of mine) use this way (perhaps covered by
> > your 3rd, perhaps not, depending on your spec):
> >
> > ;; Package-Requires: ((fit-frame "0"))
> 
> This produces a dependency on `((fit-frame (0)))' with my patch
> installed, according to `package-buffer-info'.

It should mean that the library in which it is found requires library
`fit-frame', and any version of `fit-frame' is sufficient.  (Any real
version # is >= 0.)

Dunno what that syntax with 0 instead of "0" is.  Is is supported?
Does it mean something different from ((fit-frame "0"))?

> > See also bug #14941 (still unanswered, so far).
> 
> I will look at it, if you don't mind.

Thx.

bug#15108: [Patch] Updated package dependencies, again

[Tom Willemse]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

Hey,

Another update of my patch. Fixes two cases the last one missed or
messed up:

;; Package-Requires: ()
;; Package-Requires: ((foo))

The first is the same as not specifying any `Package-Requires' or
leaving it empty. The second is the equivalent of any one of:

;; Package-Requires: foo
;; Package-Requires: (foo)
;; Package-Requires: ((foo "0"))

Any other situations I might have to take into consideration?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Updated Package dependency patch --]
[-- Type: text/x-diff, Size: 1590 bytes --]

=== modified file 'lisp/emacs-lisp/package.el'
*** lisp/emacs-lisp/package.el	2013-12-11 21:15:12 +0000
--- lisp/emacs-lisp/package.el	2013-12-11 22:29:35 +0000
***************
*** 1104,1109 ****
--- 1104,1124 ----
  
  (declare-function lm-homepage "lisp-mnt" (&optional file))
  
+ (defun package--prepare-dependencies (deps)
+   "Turn DEPS into an acceptable list of dependencies.
+ 
+ Any parts missing a version string get a default version string
+ of \"0\" (meaning any version) and an appropriate level of lists
+ is wrapped around any parts requiring it."
+   (cond
+    ((null deps) nil)
+    ((symbolp deps) `((,deps "0")))
+    ((stringp (cadr deps)) `(,deps))
+    ((and (listp deps) (listp (car deps))
+          (null (cdar deps)))
+     `((,(caar deps) "0")))
+    (t (mapcar (lambda (dep) (if (symbolp dep) `(,dep "0") dep)) deps))))
+ 
  (defun package-buffer-info ()
    "Return a `package-desc' describing the package in the current buffer.
  
***************
*** 1135,1141 ****
  	 "Package lacks a \"Version\" or \"Package-Version\" header"))
        (package-desc-from-define
         file-name pkg-version desc
!        (if requires-str (package-read-from-string requires-str))
         :kind 'single
         :url homepage))))
  
--- 1150,1158 ----
  	 "Package lacks a \"Version\" or \"Package-Version\" header"))
        (package-desc-from-define
         file-name pkg-version desc
!        (if requires-str
!            (package--prepare-dependencies
!             (package-read-from-string requires-str)))
         :kind 'single
         :url homepage))))
  

bug#15108: [Patch] Updated package dependencies

[Stefan Monnier]

> I've tested it most recently bzr trunk revision 115470. All the ways to
> specify dependencies work as described before:

Looks good, just one nitpick:

> ;; Package-Requires: foo
> ;; Package-Requires: (foo bar)
> ;; Package-Requires: (foo (baz "1.1.0") bar)
> ;; Package-Requires: (baz "1.1.0")

Please don't support the first form nor the last form, since it
otherwise gives the impression that

   ;; Package-Requires: foo bar baz
or
   ;; Package-Requires: (foo "0.1") (bar "2.0")

will also work and we don't want to go that way.


        Stefan

bug#15108: [Patch] Updated package dependencies

[Tom Willemse]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> I've tested it most recently bzr trunk revision 115470. All the ways to
>> specify dependencies work as described before:
>
> Looks good, just one nitpick:
>
>> ;; Package-Requires: foo
>> ;; Package-Requires: (foo bar)
>> ;; Package-Requires: (foo (baz "1.1.0") bar)
>> ;; Package-Requires: (baz "1.1.0")
>
> Please don't support the first form nor the last form, since it
> otherwise gives the impression that
>
>    ;; Package-Requires: foo bar baz
> or
>    ;; Package-Requires: (foo "0.1") (bar "2.0")
>
> will also work and we don't want to go that way.

Taken care of in attached patch. I was unsure of how to deal with these
situations, so I explicitly signal a (hopefully) clear error. Just
letting it crash on trying to get the cdr of a symbol seemed messy.
Please let me know what you think.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Another patch --]
[-- Type: text/x-diff, Size: 1723 bytes --]

=== modified file 'lisp/emacs-lisp/package.el'
*** lisp/emacs-lisp/package.el	2013-12-11 21:15:12 +0000
--- lisp/emacs-lisp/package.el	2013-12-12 19:13:00 +0000
***************
*** 1104,1109 ****
--- 1104,1128 ----

  (declare-function lm-homepage "lisp-mnt" (&optional file))

+ (defun package--prepare-dependencies (deps)
+   "Turn DEPS into an acceptable list of dependencies.
+
+ Any parts missing a version string get a default version string
+ of \"0\" (meaning any version) and an appropriate level of lists
+ is wrapped around any parts requiring it."
+   (cond
+    ((null deps) nil)
+    ((or (symbolp deps)
+         (and (listp deps)
+              (symbolp (car deps))
+              (stringp (cadr deps))))
+     (error "Invalid requirement specifier: %s" deps))
+    ((stringp (cadr deps)) `(,deps))
+    ((and (listp deps) (listp (car deps))
+          (null (cdar deps)))
+     `((,(caar deps) "0")))
+    (t (mapcar (lambda (dep) (if (symbolp dep) `(,dep "0") dep)) deps))))
+
  (defun package-buffer-info ()
    "Return a `package-desc' describing the package in the current buffer.

***************
*** 1135,1141 ****
  	 "Package lacks a \"Version\" or \"Package-Version\" header"))
        (package-desc-from-define
         file-name pkg-version desc
!        (if requires-str (package-read-from-string requires-str))
         :kind 'single
         :url homepage))))

--- 1154,1162 ----
  	 "Package lacks a \"Version\" or \"Package-Version\" header"))
        (package-desc-from-define
         file-name pkg-version desc
!        (if requires-str
!            (package--prepare-dependencies
!             (package-read-from-string requires-str)))
         :kind 'single
         :url homepage))))

bug#15108: [Patch] Updated package dependencies

[Stefan Monnier]

> Taken care of in attached patch. I was unsure of how to deal with these
> situations, so I explicitly signal a (hopefully) clear error. Just
> letting it crash on trying to get the cdr of a symbol seemed messy.
> Please let me know what you think.

Thanks, installed in trunk with a few changes,


        Stefan