Bug 226800 - Review Request: emacs-bbdb - email database for Emacs
Review Request: emacs-bbdb - email database for Emacs
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
: 227230 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-01 12:38 EST by Tom Tromey
Modified: 2014-08-11 01:46 EDT (History)
4 users (show)

See Also:
Fixed In Version: 2.35-8.fc8
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-12-19 18:47:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Build log from mock (3.62 KB, text/plain)
2007-02-02 10:04 EST, Anthony Green
no flags Details

  None (edit)
Description Tom Tromey 2007-02-01 12:38:26 EST
Spec URL: http://people.redhat.com/~tromey/bbdb-srpm/emacs-bbdb.spec
SRPM URL: http://people.redhat.com/~tromey/bbdb-srpm/emacs-bbdb-2.35-1.src.rpm
Description:
The Insidious Big Brother Database (BBDB) is a contact management
utility.  It is tightly integrated with several mail and news readers,
allowing it to create database entries directly from mail and news
messages.
Comment 1 Anthony Green 2007-02-02 10:04:15 EST
Created attachment 147231 [details]
Build log from mock

Hi Tom.  I can't build this package from mock (or outside of it even).	I've
attached the build log.
Comment 2 Tom Tromey 2007-02-03 14:59:35 EST
Sorry about that.  It worked fine on my FC5 box, but I see now that
it fails on FC6.  I get a different error fwiw.
I will fix this soon.
Comment 3 Tom Tromey 2007-02-03 19:06:46 EST
Could you try this?

http://people.redhat.com/~tromey/bbdb-srpm/emacs-bbdb-2.35-2.src.rpm

This includes a makefile patch that I needed on FC6.
Comment 4 Anthony Green 2007-02-05 19:41:54 EST
Thanks Tom.  This one builds for FC6 in mock.

rpmlint has a single complaint about this package...

# rpmlint emacs-bbdb-2.35-2.fc6.noarch.rpm
W: emacs-bbdb file-not-utf8 /usr/share/info/bbdb.info.gz

You can fix this with the following spec file patch...

--- emacs-bbdb.spec~    2007-02-03 12:06:44.000000000 -0800
+++ emacs-bbdb.spec     2007-02-05 16:35:51.000000000 -0800
@@ -34,6 +34,10 @@
 %prep
 %setup -q -n %{pkg}-%{version}
 %patch0 -p1
+pushd texinfo
+iconv --from=ISO-8859-1 --to=UTF-8 bbdb.texinfo > bbdb.texinfo.new
+mv bbdb.texinfo.new bbdb.texinfo
+popd

 %build



Actually, rpmlint has a "no documentation" complaint about the -el
package as well, but we can ignore it.

Comment 5 Tom Tromey 2007-02-08 21:37:40 EST
Thanks once again.  I applied this patch.
I saw this rpmlint error but somehow managed to ignore it :-/
I also saw the no documentation complaint but, I agree,
we can ignore it.  There's simply no docs to put in there.

I've uploaded the latest.

http://people.redhat.com/~tromey/bbdb-srpm/emacs-bbdb-2.35-3.src.rpm
Comment 6 Anthony Green 2007-02-20 09:13:05 EST
One new little annoyance...

# rpmlint /usr/src/redhat/RPMS/noarch/emacs-bbdb-2.35-3.noarch.rpm 
W: emacs-bbdb no-version-in-last-changelog

I'll go through the full review list on your next update, but I don't believe I
can approve this until you've been sponsored for Fedora Extras.   I've blocked
this bugzilla on FE-NEEDSPONSOR for now.
Comment 7 Anthony Green 2007-02-20 09:16:14 EST
*** Bug 227230 has been marked as a duplicate of this bug. ***
Comment 8 Jonathan Underwood 2007-02-20 18:35:31 EST
Hi Tom,

Interestingly I submitted this around the same time as you, so please look at
the spec file in bug 227230, as that contains a number of things I think you
should consider, including:

0) You're building and packaging *only* the bbdb bindings for VM, whereas bbdb
comes with bindings for Gnus and MH-E, both included with Emacs - you need to do
make all to build those as well.

1) BBDB requires TeX for the bbdb-print functionality to work, as bbdb-print
creates a TeX file, and so I included a Requires: tetex. I'm on the fence with
this one, I have to say.

2) Your package description doesn't mention that the BBDB is an add on for Emacs
- it should.

3) You should use $RPM_BUILD_ROOT *or* %{buildroot} in your spec consistently,
not a mixture of both.

4) You need to remove [ "$RPM_BUILD_ROOT" != "/" ] && from the %clean section

5) There are a number of shell scripts and extra bits which are very useful to
bbdb users - I packaged these as documentation, along with the ChangeLog with 
    %doc bits misc utils ChangeLog
in the %files section - I strongly urge you to do the same.

6) Assuming you do (5) above, you'll need to make the *.pl files non executeable
with a 
    find -name '*.pl' -exec chmod -x {} \;
in the %install section, and you'll also need to remove some irrelevant files:
    rm -f bits/make.bat
    rm -f utils/Makefile*

7) What was your reason for putting bbdb-autoloads in site-start.d ? If I recall
correctly, this is unecessary, all of the files can live happily in
site-lisp/bbdb (which is on the load-path by default). Perhaps I missed
something here though.

8) The make install-pkg target in the bbdb makefile is totally useless for our
purposes - for readability I'd really recommend not using that, but rather doing
a few simple copies into the buildroot. Example from my spec:
# There is no usable install make rule - install by hand.
%define lispdir %{_datadir}/emacs/site-lisp/bbdb
mkdir -p $RPM_BUILD_ROOT%{lispdir}
cp lisp/*.{elc,el} $RPM_BUILD_ROOT%{lispdir}

%define texdir %{_datadir}/texmf/tex/generic/bbdb
mkdir -p $RPM_BUILD_ROOT%{texdir}
cp tex/*.tex $RPM_BUILD_ROOT%{texdir}

mkdir -p $RPM_BUILD_ROOT%{_infodir}
cp texinfo/bbdb.info $RPM_BUILD_ROOT%{_infodir}

9) I think your BuildRequires for emacs-vm-el is unecessary, as although
warnings will pop up, I don't think they matter. I *may* be wrong here though. I
sort of hope so, since I recently added a patch to the emacs-vm package (the
vmrf patch) which ultimately will lead to a BuildRequires: emacs-bbdb, at which
point we'll have a circular dependency. 

I need to think more about point 9.

Also, if you want a co-maintainer, I'm happy to do that.

Great to see someone else interested in bbdb!
Comment 9 Jonathan Underwood 2007-03-04 14:58:39 EST
Hi Tom - you've been quiet a while on this, any progress?
Comment 10 Tom Tromey 2007-03-04 15:11:08 EST
Sorry, I've been on the road and I've just been catching up.
Why wouldn't we start with your spec file instead of mine?
You've clearly thought through the issues more completely.
Is there any particular thing mine did better?

About the buildreq issue -- I did not look to see whether VM
defines any macros that BBDB must use.  I think that would
be the only potential problem.

In re #7, I just copied some other package.  I don't think there
is any deeper thing than that.
Comment 11 Jonathan Underwood 2007-03-04 19:49:51 EST
Hi Tom, OK, when I get a chance, I'll confirm (or otherwise) on #7 and the
buildreq issue. One thing you spotted that I hadn't was the need to convert the
info file to UTF-8. Doesn't matter to me which spec we use as the starting point
though, but it's great to have a couple of us working on it!
Comment 12 Jonathan Underwood 2007-05-06 19:05:49 EDT
OK, have put a "best of breeds" (mongrel?) spec file and src.rpm here:

SPEC: http://jgu.nonlogic.org/emacs-bbdb.spec
SRPM: http://jgu.nonlogic.org/emacs-bbdb-2.35-3.src.rpm

See what you think.
Comment 13 Jonathan Underwood 2007-05-23 17:13:26 EDT
OK, I have been conversing with the upstream maintainer about bbdb and VM - he
is upstream maintainer of both. He has this to say regarding building of bbdb
and VM:

"At least BBDB requires VM at built time as it uses VM
macros.  If you built without VM sources then the ELCs
will contain functions calls where macros should be used
and thus it will fail, i.e. they are broken!

IMHO for VM it is the same just the other way around.

This is no problem if your distro is using binary packages
with precompiled ELCs.  Then the package maintainer needs to
care for correct compilation, but the user is not forces to
install both packages."

And so that means that there's a chicken and egg issue with building these
packages because bbdb needs a BuildRequires: emacs-vm-el and vm needs a
BuildRequires: emacs-bbdb-el.

I am unsure how to deal with that, I shall ask on the maintainers list.
Comment 14 Jonathan Underwood 2007-05-23 17:50:16 EDT
I talked to Bill Nottingham on IRC about this issue. He thinks it is fine to
have both emacs-vm and emacs-bbdb built in the buildsystem without the circular
BuildRequires, and then once packages exist, add in the BuildRequires and
rebuild both packages. So, issue resolved.
Comment 15 Ville Skyttä 2007-05-23 18:03:52 EDT
People who bootstrap these packages on new archs or derivative distros would
probably appreciate a comment in the specfile(s) explaining the scenario.
Comment 16 Jonathan Underwood 2007-05-29 18:41:19 EDT
SPEC: http://jgu.nonlogic.org/emacs-bbdb.spec
SRPM: http://jgu.nonlogic.org/emacs-bbdb-2.35-4.src.rpm

* Tue May 29 2007 Jonathan G. Underwood <jonathan.underwood@gmail.com> - 2.35-4
- Add BuildRequires: emacs-vm-el
- Add macro to determine emacsversion at package build time
- Add Requires emacs-common >= emacsversion
- Add notes about bootstrapping with VM to top of spec file
Comment 17 Jason Tibbitts 2007-09-05 19:24:39 EDT
Hey, we have approved emacs guidelines now.  Finally.  It doesn't look like this
package quite follows the guidelines, but Jonathan wrote them so I suppose he'd
know better than I.  Any chance it could be fixed up?

Who's actually submitting this package, anyway?
Comment 18 Jonathan Underwood 2007-09-05 19:38:57 EDT
I am happy to fix the package up in the next day or two (need sleep right now).
(In reply to comment #17)
> Hey, we have approved emacs guidelines now.  Finally.  It doesn't look like this
> package quite follows the guidelines, but Jonathan wrote them so I suppose he'd
> know better than I.  Any chance it could be fixed up?
> 

Happy to - will get to it tomorrow (need bed right now).

> Who's actually submitting this package, anyway?

Well, if Tom is happy to have me as a co-maintainer, I'd suggest we both submit
it - it requires some coordination with VM, as there is an element of
bootstrapping between the two packages required (I'm the emacs-vm packager, but
again, would welcome co-maintainers!).
Comment 19 Jonathan Underwood 2007-09-06 19:03:14 EDT
Updated spec file:

http://jgu.fedorapeople.org/emacs-bbdb.spec

Hven't had chance to test it yet.
Comment 21 Tom Tromey 2007-09-09 01:21:32 EDT
Hi.  Sorry I've been ignoring this once again :(
I think it would be fine for Jonathan to be the primary packager.

I looked at the newest spec file and I read through the Emacs
packaging draft.  Looking good!

Going back to the bbdb-autoloads.el question -- I think the reason that
this is in site-start.d is so that the autoloads will be evaluated at
Emacs startup (see site-lisp/site-start.el).  This means that the user
doesn't have to add an explicit require or anything to their .emacs to
start using BBDB.  I was wrong about this back in comment #10.  (FWIW
this is mentioned in the packaging guidelines.)

In my copy of the latest RPM, bbdb-autoloads.el ends up in site-lisp/bbdb/.
So, I think it should be moved again.

BTW ... BBDB and VM requiring each other seems pretty ugly!  Perhaps one or
the other could be fixed upstream.
Comment 22 Jonathan Underwood 2007-09-09 08:42:38 EDT
(In reply to comment #21)
> Hi.  Sorry I've been ignoring this once again :(
> I think it would be fine for Jonathan to be the primary packager.
> 

Fine with me, but would very much welcome a co-maintaniner, hint hint :).

> I looked at the newest spec file and I read through the Emacs
> packaging draft.  Looking good!
> 

Thanks.

> Going back to the bbdb-autoloads.el question -- I think the reason that
> this is in site-start.d is so that the autoloads will be evaluated at
> Emacs startup (see site-lisp/site-start.el).  This means that the user
> doesn't have to add an explicit require or anything to their .emacs to
> start using BBDB.  I was wrong about this back in comment #10.  (FWIW
> this is mentioned in the packaging guidelines.)

Yes - actually the best way of dealing with this IMO is to add an init file to
site-lisp/site-start.d which does a (requires 'bbdb-autoloads) - that's what
most packages do, and is the spirit of the guidelines - have updated the spec
file to create such a file. Is this ok with you?

> 
> In my copy of the latest RPM, bbdb-autoloads.el ends up in site-lisp/bbdb/.
> So, I think it should be moved again.
> 

As I say above - I would prefer to leave bbdb-autoloads in the site-lisp/bbdb
directory, and add a file to site-lisp/site-start.d which requires bbdb-autolods

> BTW ... BBDB and VM requiring each other seems pretty ugly!  Perhaps one or
> the other could be fixed upstream.

Yeah, it's very horrible. Fortunately recently it has transpired that the same
person has taken over upstream leadership of both packages, so I will begin a
campaign with him to change this situation :) He is away until the end of
October though.

Updated
SPEC: http://jgu.fedorapeople.org/emacs-bbdb.spec
SRPM: http://jgu.fedorapeople.org/emacs-bbdb-2.35-7.fc7.src.rpm

Comment 23 Tom Tromey 2007-09-09 12:46:40 EDT
> Fine with me, but would very much welcome a co-maintaniner, hint hint :).

:-).  Sounds good.

[ init file that requires bbdb-autoloads ]
> Is this ok with you?

Yes, that's great.  Thanks.

Comment 24 Jason Tibbitts 2007-09-27 15:57:14 EDT
I have a little time; let's start with the rpmlint output:
Comment 25 Jason Tibbitts 2007-09-27 16:10:47 EDT
Oops.  Let's try that again:

emacs-bbdb.src: W: spelling-error-in-description recieved received
  Funny rpmlint, but correct.

emacs-bbdb.src: W: invalid-license GPL
  The license needs some adjusting to meet the current guidelines.  I believe 
  the proper tag for this package is "GPLv2+" according to the comment block at 
  the head of bbdb.el

emacs-bbdb.noarch: W: file-not-utf8 /usr/share/info/bbdb.info.gz
emacs-bbdb.noarch: W: file-not-utf8 /usr/share/doc/emacs-bbdb-2.35/ChangeLog
  A quick call to iconv should fix these up.

emacs-bbdb.noarch: E: wrong-script-interpreter
/usr/share/doc/emacs-bbdb-2.35/utils/bbdb-cid.pl "/usr/local/bin/perl5"
emacs-bbdb.noarch: E: wrong-script-interpreter
/usr/share/doc/emacs-bbdb-2.35/utils/bbdb-unlazy-lock.pl "/usr/local/bin/perl"
emacs-bbdb.noarch: E: wrong-script-interpreter
/usr/share/doc/emacs-bbdb-2.35/utils/bbdb-srv.pl "/usr/local/bin/perl"
  Even though these are packaged as documentation, it would still be nice if 
  these were fixed up with some quick calls to sed.

emacs-bbdb-el.noarch: W: no-documentation
  This is OK.
Comment 26 Jonathan Underwood 2007-09-27 19:57:56 EDT
Updated to fix the rpmlint warnings:

SPEC: http://jgu.fedorapeople.org/emacs-bbdb.spec
SRPM: http://jgu.fedorapeople.org/emacs-bbdb-2.35-8.fc7.src.rpm
Comment 27 Jason Tibbitts 2007-10-11 01:01:07 EDT
I finally found time to get back to this.  rpmlint is indeed down to:
   emacs-bbdb-el.noarch: W: no-documentation
which is fine.

Looking through the source, a good portion of this is actually GPL+, but I'm no
licensing expert so I can't really say if you need to mention that in your
License: tag.  I'm going to make the assumption that the package as a whole is
GPLv2+.

There are a few bits in here that aren't in the emacs guidelines, such as the
handling of emacs packages without pkgconfig support.  Should those make it into
the guidelines?  It looks like the emacs in F7 doesn't have pkgconfig support,
so the guidelines seem to be missing support for, basically, all of the releases.

Honestly, though, I see nothing wrong with this package.

* source files match upstream:
   3fb1316e2ed74d47ca61187fada550e58797467bd9e8ad67343ed16da769f916  
   bbdb-2.35.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  emacs-bbdb-2.35-8.fc8.noarch.rpm
   emacs-bbdb = 2.35-8.fc8
  =
   /bin/sh
   emacs(bin) >= 22.1

  emacs-bbdb-el-2.35-8.fc8.noarch.rpm
   emacs-bbdb-el = 2.35-8.fc8
  =
   emacs-bbdb = 2.35-8.fc8

* %check is not present; no test suite upstream.  I can't remember enough about 
   emacs to be able to test this package.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK (info file registration)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

APPROVED

Since Jonathan's submitting this, I've removed FE-NEEDSPONSOR and I suppose it's
ready to go.  Do let me know if there's still a sponsorship issue.
Comment 28 Jonathan Underwood 2007-10-11 05:42:41 EDT
(In reply to comment #27)
> Looking through the source, a good portion of this is actually GPL+, but I'm no
> licensing expert so I can't really say if you need to mention that in your
> License: tag.  I'm going to make the assumption that the package as a whole is
> GPLv2+.
> 

I will check this once more and confirm the licensing.

[snip]

> Since Jonathan's submitting this, I've removed FE-NEEDSPONSOR and I suppose it's
> ready to go.  Do let me know if there's still a sponsorship issue.

This really depends on Tom (Tromey). Tom has previously said he's happy for me
to be the package owner, and that is fine with me. On the other hand if Tom
wanted to use this package in order to become sponsored (though with such an
"old hand" I wonder if sponsorship is necessary), that is fine as well, and I'm
happy to be a co-maintainer. Unless Tom replies to the contrary, I'll import the
package into CVS at the weekend.
Comment 29 Tom Tromey 2007-10-11 10:03:27 EDT
Yes, please go for it.
Comment 30 Jason Tibbitts 2007-10-11 12:30:50 EDT
If Tom needs sponsorship, he need only apply and I'll be happy to click the
appropriate button.
Comment 31 Jonathan Underwood 2007-10-12 08:33:11 EDT
New Package CVS Request
=======================
Package Name: emacs-bbdb
Short Description: A contact management utility for use with Emacs
Owners: jgu
Branches: F-7 
InitialCC: tromey@redhat.com)
Cvsextras Commits: Yes
Comment 32 Jonathan Underwood 2007-10-12 08:34:23 EDT
Tom - I've taken the liberty of adding you to the inital-cc list. Once you're
sponsored and have a FAS username, we can add that to package owners.
Comment 33 Kevin Fenzi 2007-10-12 13:39:54 EDT
We can't add arbitrary email addresses to InitialCC. It must be a Fedora Account
system account name. ;( 

cvs done. (except for the cc)
Comment 34 Jason Tibbitts 2007-12-19 13:30:23 EST
Any reason this ticket can't be closed now?
Comment 35 Jonathan Underwood 2007-12-19 18:47:44 EST
oops, sorry, this skipped my mind. Closing.

Note You need to log in before you can comment on or make changes to this bug.