Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 210025
Summary: | Review Request: openpbx - The truly open source PBX | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | David Woodhouse <dwmw2> |
Component: | Package Review | Assignee: | Jeffrey C. Ollie <jeff> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | dennis, tjb |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-10-17 08:19:57 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
David Woodhouse
2006-10-09 15:52:23 UTC
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... 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? (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. (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. (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). (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. 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) 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 I've created a userland/library only zaptel SRPM - see bug #177584 Until you switch to building from a released tarball you should probably BR libtool, automake, and autoconf. (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 (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... 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 (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. 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 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 * 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. 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. Please keep the blocker on FE-ACCEPT OK, sorry. Until when? The FE_ACCEPT summary says 'pending implementation' but this package is implemented now. (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... mISDN review requested: bug 211088. Once imported, we should build chan_misdn for Asterisk as well as OpenPBX. |