Bug 210025 - Review Request: openpbx - The truly open source PBX
Review Request: openpbx - The truly open source PBX
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jeffrey C. Ollie
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-10-09 11:52 EDT by David Woodhouse
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-17 04:19:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description David Woodhouse 2006-10-09 11:52:23 EDT
Spec URL: http://david.woodhou.se/opbx/openpbx.spec
SRPM URL: http://david.woodhou.se/opbx/openpbx-0-1.svn1912.src.rpm
Description: 
OpenPBX is an Open Source PBX and telephony development platform that
can both replace a conventional PBX and act as a platform for developing
custom telephony applications for delivering dynamic content over a
telephone similarly to how one can deliver dynamic content through a
web browser using CGI and a web server.

OpenPBX talks to a variety of telephony hardware including BRI, PRI,
POTS, Bluetooth headsets and IP telephony clients using SIP and IAX
protocols protocol (e.g. ekiga or kphone).  For more information and a
current list of supported hardware, see www.openpbx.com.


rpmlint complains a lot about non-standard-[ug]id -- obviously I need to add the openpbx user to the UserRegistry at http://fedoraproject.org/wiki/PackageUserRegistry

It also complains of lack of documentation for the subpackages, since the documentation is in the _main_ package. And it complains about lack of release version in the changelog entry -- obviously that'll be fixed when it's committed. I'm tracking Subversion in the expectation that there'll be a release any day now, and that's what I'd actually build.

Finally, rpmlint complains of dangling relative symlinks and 'only-non-binary-in-usr-lib' because of the foo.so symlinks in the -devel package. I'm not entirely sure what its problem is there, but I don't see anything wrong. Please advise.

It also complaints of mixed-use-of-spaces-and-tabs. I use tabs. Except where I need to indent the second and subsequent lines of %config to a column which isn't a multiple of 8, where I use some spaces. That's fine.

Oh, and log-files-without-logrotate in amongst all the noise. Will fix...
Comment 1 Jeffrey C. Ollie 2006-10-10 13:44:30 EDT
Some initial comments... I'll do some more review later...

(In reply to comment #0)
> Spec URL: http://david.woodhou.se/opbx/openpbx.spec
> SRPM URL: http://david.woodhou.se/opbx/openpbx-0-1.svn1912.src.rpm

Should the name of the RPM be "openpbx.org" since that's the formal name? 
Unwieldy, yes, but ISTR that "openpbx" was already taken.

> OpenPBX talks to a variety of telephony hardware including BRI, PRI,
> POTS, Bluetooth headsets and IP telephony clients using SIP and IAX
> protocols protocol (e.g. ekiga or kphone).  For more information and a
> current list of supported hardware, see www.openpbx.com.

Shouldn't the URL be openpbx.org?

> rpmlint complains a lot about non-standard-[ug]id -- obviously I need to add
> the openpbx user to the UserRegistry at
> http://fedoraproject.org/wiki/PackageUserRegistry

Yes, annoying.

> It also complains of lack of documentation for the subpackages, since the
> documentation is in the _main_ package.

I generally add the COPYING to the subpackages unless there are documentation
files that are specific to the subpackage.
 
> It also complaints of mixed-use-of-spaces-and-tabs. I use tabs. Except where
> I need to indent the second and subsequent lines of %config to a column which
> isn't a multiple of 8, where I use some spaces. That's fine.

A completely trivial and pointless check in rpmlint as far as I'm concerned.

> Oh, and log-files-without-logrotate in amongst all the noise. Will fix...

Comment 2 Dennis Gilmore 2006-10-10 14:56:56 EDT
quick look,
it may not be needed as you dont intend to ship a svn snapshot  but a script  
or comment listing exactly how you got your tarball is needed for svn 
snapshots  so that the tarball can be reproduced 

%package javascript
Group:          Applications/Internet
Summary:        Jabber support for OpenPBX

cut and paste typo?
Comment 3 David Woodhouse 2006-10-11 12:43:41 EDT
(In reply to comment #2)
> quick look,
> it may not be needed as you dont intend to ship a svn snapshot  but a script  
> or comment listing exactly how you got your tarball is needed for svn 
> snapshots  so that the tarball can be reproduced 

Such a comment is immediately above the Source0: line.

> %package javascript
> Group:          Applications/Internet
> Summary:        Jabber support for OpenPBX
> 
> cut and paste typo?

Yeah, fixed. Thanks.
Comment 4 David Woodhouse 2006-10-11 12:49:35 EDT
(In reply to comment #1)
> Should the name of the RPM be "openpbx.org" since that's the formal name? 
> Unwieldy, yes, but ISTR that "openpbx" was already taken.

Maybe. In practice the '.org' is fairly much irrelevant. Although the owners of
the original 'OpenPBX' said they were happy with the use of the name, I still
think it's a bad choice and I'm almost hoping for it to be renamed before the
first proper release. We'll see.

> > OpenPBX talks to a variety of telephony hardware including BRI, PRI,
> > POTS, Bluetooth headsets and IP telephony clients using SIP and IAX
> > protocols protocol (e.g. ekiga or kphone).  For more information and a
> > current list of supported hardware, see www.openpbx.com.
> 
> Shouldn't the URL be openpbx.org?

Yeah, fixed. Thanks.
 
> I generally add the COPYING to the subpackages unless there are documentation
> files that are specific to the subpackage.

It's bad enough having seventeen thousand redundant copies of the GPL in
/usr/share/doc already, without putting them in subpackages too just to keep
rpmlint happy :)

> > Oh, and log-files-without-logrotate in amongst all the noise. Will fix...

That's fixed, along with the fact that it was missing some BuildRequires.

http://david.woodhou.se/opbx/openpbx.spec
http://david.woodhou.se/opbx/openpbx-0-1.svn1946.src.rpm

I need to sort through the extra documentation and see what else I should be
putting in %doc, and also probably add support for building with zaptel --
there's no real need to let the kernel problems hold up the userspace library,
and OpenPBX can use it as well as Asterisk. Better, in fact, since OpenPBX
doesn't _need_ it for timing -- it uses POSIX timers as I always wanted Asterisk
to do.
Comment 5 Jeffrey C. Ollie 2006-10-11 16:46:33 EDT
(In reply to comment #4)
> http://david.woodhou.se/opbx/openpbx.spec
> http://david.woodhou.se/opbx/openpbx-0-1.svn1946.src.rpm

This version has a dependency on fedora-usermgmt-devel which is in FE-devel
only.  It also has a dependency on mISDN which isn't in FE at all (and I didn't
see any reviews pending either).
Comment 6 David Woodhouse 2006-10-11 16:53:48 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > http://david.woodhou.se/opbx/openpbx.spec
> > http://david.woodhou.se/opbx/openpbx-0-1.svn1946.src.rpm
> 
> This version has a dependency on fedora-usermgmt-devel which is in FE-devel
> only.  

Yeah, I was following the instructions on the wiki, and I was targetting FC6.
I'm testing on FC6/PowerPC. If I need to make changes to backport to FC5 then
I'll do that after we get it approved and built for -devel.

> It also has a dependency on mISDN which isn't in FE at all (and I didn't
> see any reviews pending either).

mISDN kernel code is in the same situation as Zaptel -- it isn't yet upstream
and despite the fact that my production boxes run on it, I don't want to build
kmod packages for it -- it's got to go upstream first. I'm testing with it since
the lines I have to test with here are BRI ISDN, but I'd meant to %define
build_misdn 0 for the submission.
Comment 7 Enrico Scholz 2006-10-11 18:56:09 EDT
fwiw, the misdn conditional can be expressed as

| %bcond_with misdn
| ...
| %{?with_misdn:BuildRequires: mISDN}
| ...
| configure ... %{?with_misdn:--with-chan_misdn}
| ...
| %if 0%{?with_misdn:1}
| ...

It allows short (and IMO more elegant) expressions, and allows the user to build
with '--with misdn' (or %_with_misdn in ~/.rpmmacros)
Comment 8 David Woodhouse 2006-10-11 19:25:46 EDT
OK, thanks. I've updated it to use that method for the mISDN conditional and
also the newly-added zaptel conditional.

http://david.woodhou.se/opbx/openpbx.spec
http://david.woodhou.se/opbx/openpbx-0-1.svn1951.src.rpm
Comment 9 Jeffrey C. Ollie 2006-10-12 16:14:26 EDT
I've created a userland/library only zaptel SRPM - see bug #177584
Comment 10 Jeffrey C. Ollie 2006-10-12 16:42:51 EDT
Until you switch to building from a released tarball you should probably BR
libtool, automake, and autoconf.
Comment 11 Jeffrey C. Ollie 2006-10-13 07:58:58 EDT
(In reply to comment #0)
>
> Finally, rpmlint complains of dangling relative symlinks and
> 'only-non-binary-in-usr-lib' because of the foo.so symlinks in the
> -devel package. I'm not entirely sure what its problem is there, but
> I don't see anything wrong. Please advise.

I think that the dangling relative symlinks are because "libfoo.so" is
a symlink to "libfoo.so.0.0.0" but the soname embedded in the library
is "libfoo.so.0".  RPM detects the soname and adds a "Requires:
libfoo.so.0" to the -devel package, so without the manual "Requires:
openpbx = %{version}-%{release}" installing the -devel package
wouldn't pull in the main package.  The proper fix would be to figure
out why libtool is installing the libs in this way.  The quick fix
would be to delete and recreate the "libfoo.so" symlink after "make
install" does it's thing. I think that something like the following
should fix it:

for l in libopenpbx.so libedit.so libopbxilbc.so libopbxjb.so
do
  rm -f %{buildroot}%{_libdir}/openpbx.org/$l
  ln -s $l.0 %{buildroot}%{_libdir}/openpbx.org/$l
done
Comment 12 Jeffrey C. Ollie 2006-10-13 09:31:52 EDT
(In reply to comment #11)
> (In reply to comment #0)
> >
> > Finally, rpmlint complains of dangling relative symlinks and
> > 'only-non-binary-in-usr-lib' because of the foo.so symlinks in the
> > -devel package. I'm not entirely sure what its problem is there, but
> > I don't see anything wrong. Please advise.
> 
> I think that the dangling relative symlinks are because "libfoo.so" is
> a symlink to "libfoo.so.0.0.0" but the soname embedded in the library
> is "libfoo.so.0".  RPM detects the soname and adds a "Requires:
> libfoo.so.0" to the -devel package, so without the manual "Requires:
> openpbx = %{version}-%{release}" installing the -devel package
> wouldn't pull in the main package.  The proper fix would be to figure
> out why libtool is installing the libs in this way.  The quick fix
> would be to delete and recreate the "libfoo.so" symlink after "make
> install" does it's thing. I think that something like the following
> should fix it:
> 
> for l in libopenpbx.so libedit.so libopbxilbc.so libopbxjb.so
> do
>   rm -f %{buildroot}%{_libdir}/openpbx.org/$l
>   ln -s $l.0 %{buildroot}%{_libdir}/openpbx.org/$l
> done

And of course, I should have waited for my build to finish before posting.  The
above fix didn't work...
Comment 13 David Woodhouse 2006-10-13 11:12:24 EDT
I do already BR the autoflagellation packages. Updated package now builds with
the new zaptel and drops the non-GPL-compatible ilbc library from the build. We
should make sure we drop that from the Asterisk package too.

http://david.woodhou.se/opbx/openpbx.spec
http://david.woodhou.se/opbx/openpbx-0-1.svn1958.src.rpm
Comment 14 Jeffrey C. Ollie 2006-10-13 12:38:28 EDT
(In reply to comment #13)
> I do already BR the autoflagellation packages.

Yeah, I saw that after I posted the note but then forgot to update BZ.

> Updated package now builds with
> the new zaptel and drops the non-GPL-compatible ilbc library from the build. We
> should make sure we drop that from the Asterisk package too.

Yeah, I've been working on the review of OpenPBX and updating the zaptel library
based upon your notes...  Hope to have something more soon...  Once I get those
taken care of I'll produce new libpri and asterisk packages based upon the 1.4
betas.

Comment 15 David Woodhouse 2006-10-14 07:55:06 EDT
openpbx builds against your new zaptel and libpri packages, and the
openpbx-zaptel subpackage will add the openpbx user to the zaptel group as follows:

%pre zaptel
%{_sbindir}/usermod -G `id -G openpbx | tr " " , `,zaptel openpbx

http://david.woodhou.se/opbx/openpbx.spec
http://david.woodhou.se/opbx/openpbx-0-1.svn1965.src.rpm
Comment 16 David Woodhouse 2006-10-16 08:32:27 EDT
Updated to post-1.2rc1 release and version number, and zaptel compiled by
default since it's now in Extras. I think we should be ready to go now:

http://david.woodhou.se/opbx/openpbx.spec
http://david.woodhou.se/opbx/openpbx-1.2-1.rc1.svn1976.fc6.src.rpm
Comment 17 Jeffrey C. Ollie 2006-10-16 20:53:54 EDT
* source files match upstream (can't compare MD5 since this package
  currently uses a SVN snapshot).  I'd reccomend using "svn export"
  rather than "svn checkout" to generate the tarball.  I'd also
  recommend a more specific comment on how to generate the tarball.
  Something like:

  svn export -r %{snap} svn://svn.openpbx.org/openpbx/trunk openpbx; tar czf
openpbx-r%{snap}.tar.gz
  
  Not a blocker though...

* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* rpmlint has only acceptable complaints.
* %check is not present; no test suite upstream.  Package works with a basic config.
* shared libraries are present; ldconfig is called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel subpackage.
* unversioned .so file is in the -devel subpackage.
* no pkgconfig files.
* no libtool .la droppings.

Approved.
Comment 18 David Woodhouse 2006-10-17 04:19:57 EDT
I'd prefer to keep 'svn co' instead of 'svn export', since that makes it easier
to make patches after editing files in place. But I've improved the comments --
even though if the reader has the wit to replace %{snap} for themselves they
really ought to have been able to manage 'svn' 'co' and 'tar' too :)

Built for devel and branch requested for FC5. Will need to do the new user the
old way for the FC5 package.
Comment 19 Christian Iseli 2006-10-17 04:32:22 EDT
Please keep the blocker on FE-ACCEPT
Comment 20 David Woodhouse 2006-10-17 04:40:11 EDT
OK, sorry. Until when? The FE_ACCEPT summary says 'pending implementation' but
this package is implemented now.
Comment 21 Christian Iseli 2006-10-17 05:37:44 EDT
(In reply to comment #20)
> OK, sorry. Until when?

"forever" ATM...  This might change when the package database is ready, but
we'll see

> The FE_ACCEPT summary says 'pending implementation' but
> this package is implemented now.

Fixed...
Comment 22 David Woodhouse 2006-10-17 05:59:27 EDT
mISDN review requested: bug 211088. Once imported, we should build chan_misdn
for Asterisk as well as OpenPBX.

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