Bug 177584 - Review Request: zaptel
Review Request: zaptel
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Woodhouse
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 177603 178922
  Show dependency treegraph
 
Reported: 2006-01-11 17:08 EST by Jeffrey C. Ollie
Modified: 2007-11-30 17:11 EST (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-16 07:55:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeffrey C. Ollie 2006-01-11 17:08:40 EST
Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.1-1.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.1-1.src.rpm
Description:

Tools for configuring/monitoring Zaptel telephony interfaces.
Comment 1 Will Tatam 2006-01-12 06:34:12 EST
This spec is totally missing the kernel modules and also does not split the devel

I would recommend you bases it on
http://dag.wieers.com/packages/zaptel/zaptel.spec but make it compliant with any
fedora conventions.

Also as you will see from DAG's packaging, that the Makefile will work if you
supply the correct parameters to it
Comment 2 Tim Jackson 2006-01-12 07:06:44 EST
bug #177583 has the kernel modules
Comment 3 Jeffrey C. Ollie 2006-01-12 08:31:46 EST
1) As Tim Jackson says, the kernel modules are in bug # 177583.  See
http://www.fedoraproject.org/wiki/Extras/KernelModuleProposal and the
fedora-extras list archives for a discussion of the new kernel module packaging
proposal.

2) Yes, my spec does split off a -devel package.

3) The problem with just specifying CFLAGS before running make is that the
Makefile will append additional optimization parameters that I don't want.

4) The problem with using "make install" is that it would force a build of the
device drivers (which are built in a separate package), as well as running a
large number of other commands that are inappropriate to be run as part of the
packaging process.

I've used Dag's and other people's .spec files as inspiration - I've been
packaging up Zaptel for a while now... Now that Fedora Extras has a process for
packaging kernel modules it was time for me to share my packages with the rest
of the world.
Comment 4 Thorsten Leemhuis 2006-01-13 09:58:56 EST
(In reply to comment #3)
> 4) The problem with using "make install" is that it would force a build of the
> device drivers (which are built in a separate package), as well as running a
> large number of other commands that are inappropriate to be run as part of the
> packaging process.

You really should use "make install" and remove the part of the makefile that
builds the module; use a patch during %prep
Comment 5 Jeffrey C. Ollie 2006-01-13 10:42:49 EST
Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.1-2.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.1-2.src.rpm

The devel subpackage needs to require the main package.
Comment 6 Jeffrey C. Ollie 2006-01-13 10:45:49 EST
(In reply to comment #4)
> (In reply to comment #3)
> > 4) The problem with using "make install" is that it would force a build of the
> > device drivers (which are built in a separate package), as well as running a
> > large number of other commands that are inappropriate to be run as part of the
> > packaging process.
> 
> You really should use "make install" and remove the part of the makefile that
> builds the module; use a patch during %prep

Hmm... the install section of the Makefile would require *EXTREME* surgery. 
Unless it's going to block approval of the package I'd rather install the files
that I need manually in the spec until the upstream package gets fixed.
Comment 7 Thorsten Leemhuis 2006-01-13 11:37:48 EST
(In reply to comment #6)
> Hmm... the install section of the Makefile would require *EXTREME* surgery. 

Really? I took a quick look. From a first glance I would presume that removing
everything between
- if [ -f zaptel.ko ]; then \
and
- fi
should be enough.

> Unless it's going to block approval of the package 
I'm not the reviewer. But if was I would consider it a blocker.

> I'd rather install the files
> that I need manually in the spec until the upstream package gets fixed.
I did something like that in the past -- you have to recheck after each upstream
update that you still install everything exactly as the makefile would do it.
And that is a lot of work and often is forgotten (and that's the reason why I
think it's a blocker). 
Comment 8 Roy-Magne Mo 2006-01-18 08:53:12 EST
asterisk, zaptel and libpri 1.2.2 has been released :) 
Comment 9 Jeffrey C. Ollie 2006-01-18 17:42:03 EST
Update Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.1-1.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.1-1.src.rpm

* Wed Jan 18 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.2-2
- Bump release number.

* Wed Jan 18 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.2-1
- Update to 1.2.2.
Comment 10 Jeffrey C. Ollie 2006-01-25 09:47:12 EST
Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.2-3.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.2-3.src.rpm

* Mon Jan 23 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.2-3
- provide zaptel-kmod-common
Comment 11 Jeffrey C. Ollie 2006-01-31 01:24:43 EST
Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.3-1.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.3-1.src.rpm

%changelog
* Tue Jan 31 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.3-1
- Preserve timestamps when we install.
- Use custom init.d file that does all the fancy RH stuff.

* Mon Jan 30 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.3-1
- Update to 1.2.3.
Comment 12 Jeffrey C. Ollie 2006-02-16 20:52:06 EST
Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.4-1.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.4-1.src.rpm

%changelog
* Wed Feb 15 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.4-1
- Update to 1.2.4
Comment 13 Matthias Saou 2006-03-29 07:20:03 EST
I would have to agree with Thorsten here, as it would be much cleaner, and have
less chances to break in a future update, if "make install" was used, with the
kernel modules build and install parts ripped out.
Comment 14 Jeffrey C. Ollie 2006-04-04 07:45:35 EDT
Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.5-1.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.5-1.src.rpm

%changelog
* Mon Mar 27 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.5-1
- Update to 1.2.5
Comment 15 Andy Kwong 2006-04-24 11:45:49 EDT
There is an issue with the zaptel.rules file for udev. The KERNEL= on each line
has an additional = sign which causes udev not to create the /dev/zap entries.

On a related note, the udev rule calls for the owner to be "asterisk", but
neither this package or the asterisk package seems to be creating the user. In
practice however, udev just creates the dev node (after the above is fixed) as root.
Comment 16 Jeffrey C. Ollie 2006-04-27 12:29:16 EDT
Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/zaptel-1.2.5-2.spec
SRPM Name or Url: http://www.ocjtech.us/zaptel-1.2.5-2.src.rpm

%changelog
* Thu Apr 27 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.2.5-2
- Changed ownership of device nodes to "root" in udev rules file.
- Don't build sethdlc.
Comment 17 Andy Kwong 2006-05-18 07:34:56 EDT
The issue with the udev file is still there -

--- zaptel.rules        2006-05-18 11:28:15.000000000 +0100
+++ zaptel.rules.clean  2006-05-18 11:28:42.000000000 +0100
@@ -1,5 +1,5 @@
-KERNEL=="zapctl",     NAME="zap/ctl",     OWNER="root", GROUP="root", MODE="0660"
-KERNEL=="zaptimer",   NAME="zap/timer",   OWNER="root", GROUP="root", MODE="0660"
-KERNEL=="zapchannel", NAME="zap/channel", OWNER="root", GROUP="root", MODE="0660"
-KERNEL=="zappseudo",  NAME="zap/pseudo",  OWNER="root", GROUP="root", MODE="0660"
-KERNEL=="zap[0-9]*",  NAME="zap/%n",      OWNER="root", GROUP="root", MODE="0660"
+KERNEL="zapctl",     NAME="zap/ctl",     OWNER="root", GROUP="root", MODE="0660"
+KERNEL="zaptimer",   NAME="zap/timer",   OWNER="root", GROUP="root", MODE="0660"
+KERNEL="zapchannel", NAME="zap/channel", OWNER="root", GROUP="root", MODE="0660"
+KERNEL="zappseudo",  NAME="zap/pseudo",  OWNER="root", GROUP="root", MODE="0660"
+KERNEL="zap[0-9]*",  NAME="zap/%n",      OWNER="root", GROUP="root", MODE="0660"
Comment 18 Jeffrey C. Ollie 2006-05-18 09:37:02 EDT
(In reply to comment #17)
> The issue with the udev file is still there -

The "extra" equals sign is a udev-version specific thing.  I believe it was in
udev version 054 where the change was made.  The next version of zaptel (due
RSN) will conditially generate a udev rules file appropriate for the installed
version of udev.  As far as the ownership of the device files goes, it makes a
lot of sense to run asterisk as a non-root user.  How best to accomplish that in
RPM packages is something I'll have to look into.
Comment 19 Kenneth Porter 2006-05-18 11:25:40 EDT
If you generate different udev files dynamically based on the udev version, then
you should probably also include a versioned Requires on udev.
Comment 20 Jeffrey C. Ollie 2006-05-18 11:38:04 EDT
(In reply to comment #19)
> If you generate different udev files dynamically based on the udev version, then
> you should probably also include a versioned Requires on udev.

Yes, I will.  I should also be calling udevstart in %post/%postun too.
Comment 21 Kevin Fenzi 2006-06-16 21:26:34 EDT
Hey Jeff. I am looking at this next since I started reviewing zaptel-kmod... 

Do you have an updated srpm/spec to review? 

Any chance of a patch to allow you to use 'make install'?
Any udev changes to do diffrent things vs diffrent udev versions?
Comment 22 Gavin Henry 2006-08-23 18:09:00 EDT
Hi,

What is the status of this package?

Thanks,

Gavin.
Comment 23 Kevin Fenzi 2006-08-23 19:03:11 EDT
In response to comment #22: 

I think it's waiting to hear the fate of the zaptel-kmod submission. 
Once thats decided we can close or move this one forward. 
Comment 24 Jeffrey C. Ollie 2006-10-11 15:24:21 EDT
Since its looking like zaptel will get included in the kernel RPM before the
kmod gets approved I may try and convert this to a userland/library-only package.
Comment 25 David Woodhouse 2006-10-11 18:15:45 EDT
Yes, that's a good idea. 
Comment 26 Jeffrey C. Ollie 2006-10-12 16:09:21 EDT
Spec:
http://repo.ocjtech.us/asterisk-1.4/fedora/development/SRPMS/zaptel-1.4.0-0.fc6.beta1.spec
SRPM:
http://repo.ocjtech.us/asterisk-1.4/fedora/development/SRPMS/zaptel-1.4.0-0.fc6.beta1.src.rpm

OK, here's a SRPM updated to the latest 1.4 beta, without any dependencies on a
kmod package.

Comment 27 Jeffrey C. Ollie 2006-10-12 16:11:36 EDT
Dropping the dependency on bug #177583 since it's likely that the Zaptel kernel
modules will someday become a part of the kernel package.
Comment 28 David Woodhouse 2006-10-13 04:51:50 EDT
Probably wants to drop the Provides: zaptel-kmod-common

Probably also wants to drop the loading of modules in the initscript, since udev
should handle that kind of stuff by the time we've finished.

Why are we building with -fsigned-char on PPC? That's scary and doesn't match
the rest of the system. It's usually a sign of buggy code -- where do we assume
that 'char' is signed, and why? Let's just fix it instead.

Utils should probably be going into %{_sbindir} instead of /sbin.

What is the licence on the OCT6114-128D.ima firmware file?

ifup-hdlc attempts to use 'sethdlc', which isn't present or in Requires.

Consider being more biarch-friendly by putting libraries into their own package
(which can then be install for both 32-bit and 64-bit simultaneously), while the
other bits like configuration and initscript (if you still have one) move into
the zaptel-utils package.

udev rules give ownership of all zap devices to asterisk, but that user doesn't
exist. Perhaps we should have a 'zaptel' group, and add both Asterisk and
OpenPBX to that group? You'll need to create the zaptel group for yourself in
the zaptel package, and the asterisk-zaptel and openpbx-zaptel packages would
each need to add their PBX user to the zaptel group. Or can someone think of a
better solution?
Comment 29 Jeffrey C. Ollie 2006-10-13 15:21:04 EDT
Spec:
http://repo.ocjtech.us/asterisk-1.4/fedora/development/SRPMS/zaptel-1.4.0-1.fc6.beta1.spec
SRPM:
http://repo.ocjtech.us/asterisk-1.4/fedora/development/SRPMS/zaptel-1.4.0-1.fc6.beta1.src.rpm

* Fri Oct 13 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.4.0-1.beta1
- Remove "Provides: zaptel-kmod-common"
- Don't load modules in initscript (except for possibly ztdummy) - leave that up
to udev/hotplug.
- Drop ifup-hdlc - it requires sethdlc which isn't available.
- Move utilities to %%{_sbindir} rather than /sbin.
- Split libtonezone off into a separate lib package.
- Use 'zaptel' user/group to own device files.
- Add patch to fix minor makefile bug.
Comment 30 David Woodhouse 2006-10-14 06:01:04 EDT
OK, looks good. Only outstanding question is the licence of the OCT6114-128D.ima
file. Is that GPL'd? If so, where's the source. If not, it needs to go.
Comment 31 Jeffrey C. Ollie 2006-10-14 20:24:34 EDT
I believe it's part of the GPL'd Oactasic API but I'm not sure... I say get rid
of it until we can find out for sure the license on it.
Comment 32 Jeffrey C. Ollie 2006-10-15 20:38:34 EDT
Spec:
http://repo.ocjtech.us/asterisk-1.4/fedora/development/SRPMS/zaptel-1.4.0-1.fc6.beta1.spec
SRPM:
http://repo.ocjtech.us/asterisk-1.4/fedora/development/SRPMS/zaptel-1.4.0-1.fc6.beta1.src.rpm

%changelog
* Sun Oct 15 2006 Jeffrey C. Ollie <jeff@ocjtech.us> - 1.4.0-2.beta1
- Don't package firmware until license can be figured out.
Comment 33 David Woodhouse 2006-10-16 05:57:14 EDT
Looks good. Go ahead.
Comment 34 Jeffrey C. Ollie 2006-10-16 07:55:08 EDT
Imported and built for FE-devel.  Branch for FE-5 requested.
Comment 35 David Woodhouse 2006-10-16 08:38:07 EDT
zaptel user added to http://fedoraproject.org/wiki/PackageUserRegistry
Comment 36 Matthias Saou 2006-10-25 06:04:00 EDT
The package release is wrong (zaptel's too). It should follow the package naming
guidelines and be something like "0.1.beta1.fc6" :
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a

And why this update to a beta version? If it had been only in FC6/devel, why
not, but the plan seems to be to also release for FC5??
Comment 37 Jeffrey C. Ollie 2007-11-03 14:22:30 EDT
Package Change Request
======================
Package Name: zaptel
New Branches: EL-5
Updated EPEL Owners: jcollie

Needed to build Asterisk package on EL-5.
Comment 38 Jeffrey C. Ollie 2007-11-03 17:41:09 EDT
Oops, forgot to set the CVS flag.
Comment 39 Kevin Fenzi 2007-11-04 13:57:51 EST
cvs done.

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