Bug 250971

Summary: Review Request: ivtv-utils - userspace tools for iTVC15/16 and CX23415/16 driven devices
Product: [Fedora] Fedora Reporter: Axel Thimm <axel.thimm>
Component: Package ReviewAssignee: Jarod Wilson <jarod>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: high    
Version: rawhideCC: fedora-package-review, nicolas.mailhot, notting, tuju, zing
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: jarod: fedora-review+
huzaifas: fedora-cvs+
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-12-02 19:50:29 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:

Description Axel Thimm 2007-08-06 09:02:11 UTC
Spec URL: http://dl.atrpms.net/all/ivtv-fedora.spec
SRPM URL: http://dl.atrpms.net/all/ivtv-1.0.1-1.src.rpm
Description:

The primary goal of the IvyTV Project is to create a kernel driver for
the iTVC15 familiy of MPEG codecs. The iTVC15 family includes the
iTVC15 (CX24315) and iTVC16 (CX24316). These chips are commonly found
on Hauppauge's WinTV PVR-250 and PVR-350 TV capture cards.

This package contans the userland tools required.

Comment 1 Nicolas Chauvet (kwizart) 2007-08-24 18:58:42 UTC
starting this review ...

Comment 2 Nicolas Chauvet (kwizart) 2007-08-24 20:26:34 UTC
mock build works but rpmlint show errors:

1- Same treatment as the firmware for Requires:
There is no perl scripts at this time to be reviewed at this time.
They need to be reviewed first...

2- License is now GPLv2 (not GPLv2+ ) this probably concern the kernel module
part... I haven't checked if the tools are concerned also... (we only care of
the tools actually)

3- We shouldn't provided a ivtv-devel for this, as kernel-headers should provide
it - which do not seems the case on a recent Fedora 7 kernel...Unless this
headers is made for tools...(I don't think so...)

4- make must honnor $RPM_OPT_FLAGS and %{?_smp_mflags} - you have to state why
you don't use them if it's don't work...

5- rpmlint error: E: ivtv only-non-binary-in-usr-lib
This concerns:  "install -p utils/ivtvfwextract.pl %{buildroot}%{_libdir}/ivtv/
see if they can be installed into %{_datadir}/ivtv/ or %{_bindir}/ if needed...
And it will probably need some perl dependency (but they could also be bring by
Perl-Video-ivtv and perl-Video-Frequencies )

6- symlinks have to be relative - But maybe those who produced the warning
aren't usefull anymore...
install -p v4l-cx2341x-init.mpg %{buildroot}/lib/modules/v4l-cx2341x-init.mpg
See if we can only install it in /lib/firmware

7- %{_libdir}/ivtv Directory ownership - 
It has to be %{_libdir}/ivtv/ if ever still used...





Comment 3 Nicolas Mailhot 2007-08-26 19:11:43 UTC
Some other comments

1. upstream released a new version, please update

2. now perl is not in the buildroots, a sed buildrequires would be lighter for
munging

3. should probably require the firmware stuff

4. the mpg stuff should be dumped since it's part of firmware

5. kill the kernel header

6. personnaly, I prefer explicit defattr that set permissions and install -d
instead of mkdir

7. I'd classify ivtv in System Environment/Kernel as those are low-level tools
closely associated to the kernel

8. I'd also build and install the tools in test, in a separate package if necessary

Comment 4 Nicolas Chauvet (kwizart) 2007-10-07 16:06:24 UTC
Any news ? We are getting short to have it added in Fedora 8 !...

Comment 5 Axel Thimm 2007-10-24 12:05:55 UTC
(In reply to comment #2)
> 6- symlinks have to be relative

Actually not, unless the package knows that the crossing folders are not
symlinks themselves, which it cannot control.

Comment 6 Axel Thimm 2007-10-24 12:55:52 UTC
(In reply to comment #2)
> 2- License is now GPLv2 (not GPLv2+ ) this probably concern the kernel module
> part... I haven't checked if the tools are concerned also... (we only care of
> the tools actually)

In the utils folder there are 22 files mentioning GPL2 of which 20 allow a later
version, e.g. 2 files are GPLv2 only.

Probably not the authors' intention, still until this is fixed upstream by the
author that placed the copyright note we cannot label this 2+.

Comment 7 Nicolas Chauvet (kwizart) 2007-10-24 17:39:10 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > 6- symlinks have to be relative
> 
> Actually not, unless the package knows that the crossing folders are not
> symlinks themselves, which it cannot control.

I didn't understood what you meant by that... ^^
What I meant was to use (if ever this is still needed, and i think not - but not
sure).
ln -s ../modules/v4l-cx2341x-init.mpg %{buildroot}/lib/firmware/
(but for now they are bundled with the ivtv-firmware which seems better !?)

So, since you wasn't here i've made some improvements:
https://bugzilla.redhat.com/show_bug.cgi?id=348911

Somes (all?) perls scripts have to be dropped because some are requesting
perl-Video-{ivtv,Frequencies}. They are known unmaintained and not working from
http://ivtvdriver.org/ . So removing them will prevent to be requested by the
automatic perl dependencies extractor.
I wonder if we should keep this one : ivtvfwextract.pl




Comment 8 Axel Thimm 2007-10-24 18:15:09 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > 6- symlinks have to be relative
> > 
> > Actually not, unless the package knows that the crossing folders are not
> > symlinks themselves, which it cannot control.
> 
> I didn't understood what you meant by that... ^^

In short: Every relative symlink is broken as it assumes a static non-symlinked
layout. For example symlinks to ../lib from /usr will be broken if /usr has been
moved to /storage15/usr and /usr being a symlink to it.

But this is not specific to this package and you can read the archives on this
at fedora-packaging. Just don't trust rpmlint on everything it recommends.

Comment 9 Axel Thimm 2007-10-26 06:21:27 UTC
http://dl.atrpms.net/all/ivtv-1.0.3-2.src.rpm
http://dl.atrpms.net/all/ivtv-fedora.spec

* Wed Oct 24 2007 Axel Thimm <Axel.Thimm> - 1:1.0.3-2
- Update to 1.0.3.
- Remove pseudo-firmware file (mpeg initialisation sequence).
- Remove devel subpackage.
- Use %%{optflags}.
- Move parl scripts to %%{_datadir}.
- Add dependency to firmware package.

Comment 10 Nicolas Chauvet (kwizart) 2007-10-26 13:05:04 UTC
Ok, so i will close the other ivtv review - but there is still some problem with
this package, and actually i cannot approve it because:

NEED_WORK:
MUST - Need to remove The perl Requires.
MUST - In %prep Please uses sed instead as this will not work as perl have been
removed of the minimal BR. (the two firsts lines in %prep could be removed)
perl -pi -e's@CFLAGS = -D_GNU_SOURCE .*@CFLAGS = -D_GNU_SOURCE %{optflags}@'
utils/Makefile
 And I wonder if some
MUST - %build : add make -C utils %{?_smp_mflags}
MUST - Description need to be improved to state that this is not the driver
package but tools for the driver...

SHOULD - You may prevent timestamps changes while using install -pm instead of
mv -  (but this is highly recommended by the reviewer, as this will keep the
upstream date creation of files, instead of the build date in Fedora...)
SHOULD - add %doc README* COPYING ChangeLog need to be added (at least README is
useful to have info about kernel module info and how to build them...)
SHOULD - License is missing whereas the package bundle it... But the license
seems related to the kernel part (anyway it is the same, so i would say, pick it)
SHOULD - This question is mainly an upstream question : the package bundles
videodev2.h (which is the same as the one from /usr/include/linux/videodev2.h
and as such do not give expected problem). But, some files are referring from
the videodev2.h from the package whereas some others from kernel-headers...
Maybe upstream have to choose one method... This isn't a problem as files are
the same (but it may change...)

- One Hight Remark, i wonder why we should keep compatibility


Comment 11 Nicolas Chauvet (kwizart) 2007-10-26 13:06:09 UTC
^^ the last sentence was for the ivtv-firmware

Comment 12 Nicolas Chauvet (kwizart) 2007-11-04 12:21:03 UTC
Any new Axel ?
Requires: perl-Video-ivtv, perl-Video-Frequencies
^^ we cannot have this at least (and others dependencies extracted for perls) !

Comment 13 Nicolas Chauvet (kwizart) 2007-11-07 12:56:29 UTC
Axel! I really would like to solve the problem "with you" !

I've imported nothing into the repository for firmware or xorg-x11-drv-ivtv yet.
So i will give you the packages owership on your request as soon as the packages
are approved... (for the firmware)

I've recently contributed a spec file for package where the maintainer would
like to stay compat with rh9 or older...(not within Fedora anyway). My point of
view is if it will work with our current releases (I mean FC-6 -> F8 and epel4
epel5 ) Then I will agree to have compat field for older release (but I view
this as unnecessary since we use branch for each releases).

If really needed, I think any can have a split with ivtv-firmware-compat that
would provides the necessary symlinks and perls requirement for older version of
the driver ! (but this seems unuseful within Fedora...)
Or we could have it replaced by a higher evr from your repository...



Comment 14 Nicolas Chauvet (kwizart) 2007-11-16 15:56:40 UTC
Theses tools seems now deprecated... Please re-open the review if you think
They are still usefull for the ivtv driver within 2.6.22 kernels and laters...
http://lists-archives.org/video4linux/19986-cx88-ivtv-emulation-hvr-1300.html

Comment 15 Axel Thimm 2007-11-29 16:52:40 UTC
(In reply to comment #14)
> Theses tools seems now deprecated... Please re-open the review if you think
> They are still usefull for the ivtv driver within 2.6.22 kernels and laters...
> http://lists-archives.org/video4linux/19986-cx88-ivtv-emulation-hvr-1300.html

The blackbird design is not relevant to 99% of ivtv hardware implementations.
ivtv itself is far from being deprecated.


Comment 16 Nicolas Chauvet (kwizart) 2007-11-29 20:21:18 UTC
Then please fix the each item explained in #10
Specially, would you agree to remove this line: 
Requires: perl-Video-ivtv, perl-Video-Frequencies
(those are deprecated from what's said on ivtvdriver.org, 
that's said here: http://ivtvdriver.org/index.php/Download )

Comment 17 Axel Thimm 2007-11-29 21:12:07 UTC
Yes, the perl-Video-* packages need to be removed. Will do so in the next
package update.

Comment 18 Nicolas Chauvet (kwizart) 2007-12-12 12:29:46 UTC
ping ?

Comment 19 Axel Thimm 2007-12-30 09:41:27 UTC
http://dl.atrpms.net/all/ivtv-1.0.3-3.src.rpm
http://dl.atrpms.net/all/ivtv-fedora.spec

* Wed Dec 26 2007 Axel Thimm <Axel.Thimm> - 1:1.0.3-3
- Remove the dependencies on old perl helper modules.
- Changed summary and description to reflect removal of driver code.


Comment 20 Juha Tuomala 2008-01-01 13:57:26 UTC
Is there already pkgs available in updates-testing/koji? I've a system waiting 
for this and could do some testing.

Comment 21 Axel Thimm 2008-01-02 08:17:59 UTC
> Is there already pkgs available in updates-testing/koji?

After a review is approved it is first imported into CVS and then packages are
built for rawhide and then releases. You can pick the src.rpm for now and build
it yourself which will give testing feedback to the review itself. If you just
want a working solution for now you can get binary builds from the above
mentioned src.rpm at

http://atrpms.net/dist/f8/ivtv-fedora/

Comment 22 Nicolas Chauvet (kwizart) 2008-01-02 16:36:49 UTC
From the the build.log
* Use of $RPM_OPT_FLAGS
cc -I/builddir/build/BUILD/ivtv-1.0.3/utils/../driver -D_GNU_SOURCE -O2 -Wall -g
   ivtv-radio.c   -o ivtv-radio
This mean that our $RPM_OPT_FLAGS aren't took, there is a problem with the perl
patch in %prep.

Others dynamic patches in %prep that are related to the driver part could be
dropped...

* Missing %{?_smp_mflags} when doing make at %build step
(if it do not work, you have to tell it within the spec).

* Checks if Requirements are sane:
Requires: /usr/bin/perl ivtv-firmware libc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit) libc.so.6(GLIBC_2.3)(64bit) libgcc_s.so.1()(64bit)
libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libm.so.6(GLIBC_2.2.5)(64bit)
libpthread.so.0()(64bit) libpthread.so.0(GLIBC_2.2.5)(64bit)
libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit)
libstdc++.so.6(GLIBCXX_3.4)(64bit) perl(Getopt::Long) rtld(GNU_HASH)
The above line show the requirement for the ivtv package, it has /usr/bin/perl
and perl(Getopt::Long) that are bring by the /usr/share/ivtv/ivtvfwextract.pl

* The README.utils state that:
----------------------------------------------------
v4l2-ctl
--------
This is a copy of the same utility that is available from the linux
v4l-dvb repository (www.linuxtv.org). It can be used to set, list and
get almost everything that the v4l2 API offers.

ivtvctl
-------
Similar to v4l2-ctl, but only contains ivtv-specific features. Eventually
all ivtv functionality should be moved into v4l2-ctl, and then this
utility will be removed.
----------------------------------------------------
So I wonder if v4l2-ctl will conflict the v4l-dvb tools. For now I think
providing v4l2-ctl with ivtv is better, but we might monitor if this package
will be replaced by something from http://www.video4linux.org/browser

So to sum-up: there is a need to check for $RPM_OPT_FLAGS and add  %{?_smp_mflags}.

@Juha Tuomala - I would say if you can test it would be fine, but I have no
doubt that the package works... What is checked here is about fine tuning the
package against the Fedora builsystem and guidelines.
Which card do you own for information ?




Comment 23 Juha Tuomala 2008-01-02 17:00:19 UTC
> Which card do you own for information ?

I've one Hauppage 500 dual card on CentOS5, but I'm not sure did it break once 
my environment temperatures got high (or did something change on centos4 back 
then). Thus I got another Hauppage 150 (wintv) card on f8 box which i can use 
for whatever testing. Box is old&slow and thus i can't compile on it but it 
should work as tuner just fine.

Comment 24 Juha Tuomala 2008-01-02 17:13:30 UTC
Single card: WinTV-Express, Multi-PAL, 44809 LF, REV E1A5.
 Combo card:       PAL-B/G-I-,D/K-SECAM 23559 LF REV E491.

Comment 25 Nicolas Chauvet (kwizart) 2008-02-20 12:48:38 UTC
I can't review this package anymore as the submitter and I are in conflict.
Reassigned to noby  - Feel free to take it


Comment 26 Jarod Wilson 2008-02-21 22:20:06 UTC
Taking over review...

Comment 27 Jarod Wilson 2008-02-21 22:23:19 UTC
Changing summary slightly. This isn't the driver, that's in the kernel... :)

Comment 28 Juha Tuomala 2008-02-27 17:35:04 UTC
Axel, any news on this? :)

Comment 29 Axel Thimm 2008-03-04 23:28:49 UTC
(In reply to comment #28)
> Axel, any news on this? :)

This package is waiting for the ivtv-firmware package to pass review and the new
review to happen. But you can still test it and provide feedback.


Comment 30 Axel Thimm 2008-03-16 09:51:48 UTC
Any chance we could squeeze it into F9 still?

Comment 31 Jarod Wilson 2008-03-17 00:01:37 UTC
Putting it on my todo list for tomorrow. How are we doing with respect to using
the Fedora $RPM_OPT_FLAGS and _smp_mflags, per comment #22?

Comment 32 Jason Tibbitts 2008-06-26 00:31:22 UTC
So did anything ever happen here?

Comment 33 Jarod Wilson 2008-06-26 04:36:28 UTC
Oh, crap. Seems I've completely dropped the ball on this one. The firmware is
in, but I think the last time I looked at this, I was halfway waiting on a reply
to comment #31 (and by extension, #22), and then forgot about it. My sincere
apologies.

I see there's a relatively new ivtv 1.2.0 out now, which ATrpms packaged up a
few days ago. Axel, where should I pick up the review at? (i.e., with what spec?)

Comment 34 Axel Thimm 2008-06-26 04:51:56 UTC
(In reply to comment #33)
> I see there's a relatively new ivtv 1.2.0 out now, which ATrpms packaged up a
> few days ago. Axel, where should I pick up the review at? (i.e., with what spec?)

I'll package up a separate one for Fedora inclusion, as 1.2.x still has kos inside.

On a different note: Conexant has been granting permissions on using the
firmware at the kernel source level (and other firmwares as well), so probably
some future kernel will need to Obsolete ivtv-firmware. But let's get there
first. :)


Comment 35 Jarod Wilson 2008-07-02 16:27:44 UTC
(In reply to comment #34)
> I'll package up a separate one for Fedora inclusion, as 1.2.x still has kos
inside.

Give me a swift kick whenever its ready and I swear I'll get right on it... :)

> On a different note: Conexant has been granting permissions on using the
> firmware at the kernel source level (and other firmwares as well), so probably
> some future kernel will need to Obsolete ivtv-firmware. But let's get there
> first. :)

Cool, good to know.


Comment 37 Jarod Wilson 2008-08-11 03:35:53 UTC
Hm... CFLAGS look good now, but still no smp_mflags, and -ENOBUILD on rawhide...

[...]
g++ -D_GNU_SOURCE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic   -c -o v4l2-driverids.o v4l2-driverids.cpp
g++ -D_GNU_SOURCE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic   -c -o v4l2-chipids.o v4l2-chipids.cpp
g++ -lm -o v4l2-dbg v4l2-dbg.o v4l2-driverids.o v4l2-chipids.o
cc -D_GNU_SOURCE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic   -c -o ivtvctl.o ivtvctl.c
In file included from ivtvctl.c:66:
linux/ivtv.h:48: error: expected ':', ',', ';', '}' or '__attribute__' before '*' token
make[1]: *** [ivtvctl.o] Error 1

Comment 38 Axel Thimm 2008-08-14 07:30:28 UTC
http://dl.atrpms.net/all/ivtv-fedora.spec
http://dl.atrpms.net/all/ivtv-1.2.0-6.src.rpm

Now builds against 2.6.27 headers (w/o __user) as well. Since the package is not that big _smp_mflags just messes up the build output, so I prefer to keep it w/o (it's a soft SHOULD in the Packaging guidelines and not even mentioned in the review guidelines :).

Comment 39 Jarod Wilson 2008-08-20 13:50:21 UTC
Yep, that builds on rawhide now. Two things:

1) for the benefit of others looking in the future, you might want to add a brief comment to the spec about why no _smp_mflags.

2) the changelog needs to be fixed up, its missing entries for 1.2.0-5 and 1.2.0-6 (which makes rpmlint cranky).

Consider #1, fix #2, and package APPROVED.

Comment 40 Axel Thimm 2008-08-23 19:22:36 UTC
Thanks! I'll add a changelog for the latest entry, but there is another issue up: 

Upstream has released a version for F10 upwards (or 2.6.27 kernel upwards to be more precise, e.g. if/when F9 goes 2.6.27 it will be for F9 as well), which by itself is not an issue, but by doing so the package was renamed from ivtv to ivtv-utils.

So for the review/cvs I need approval for

o ivtv 1.2.x version as submitted for F8/F9, *but not devel (!)*, and
o ivtv-utils 1.3.x *only for devel*

Or we could just named this ivtv-utils from the start (e.g. for F8/F9 as well) cheating a bit on the "old" 1.2.x software.

What do you think?

Comment 41 Jarod Wilson 2008-08-25 13:37:02 UTC
I'd say just call it ivtv-utils everywhere, and Obsoletes/Provides ivtv. Hey, that way, you can even drop the epoch... :)

Comment 42 Axel Thimm 2008-08-30 16:55:56 UTC
Dropping epochs is sexy! :)

http://dl.atrpms.net/all/ivtv-utils.spec
http://dl.atrpms.net/all/ivtv-utils-1.2.0-6.src.rpm

This is what will be committed for F-8/F-9, F10 is similar, e.g. dropping a fix and upping the Version.

Comment 43 Nicolas Mailhot 2008-09-09 17:32:20 UTC
1.3.0 is out…

Comment 44 Axel Thimm 2008-10-05 10:14:18 UTC
New Package CVS Request
=======================
Package Name: ivtv-utils
Short Description: Tools for the iTVC15/16 and CX23415/16 driver
Owners: athimm
Branches: F-8 F-9 F-10
InitialCC:

Comment 45 Huzaifa S. Sidhpurwala 2008-10-06 09:33:57 UTC
cvs done

Comment 46 Bill Nottingham 2008-12-01 14:54:18 UTC
Was someone going to actually build this?

Comment 47 Jarod Wilson 2008-12-02 19:50:29 UTC
I've committed and built ivtv-utils 1.3.0 for rawhide, working on F10 and F9 now (going to leave out F8, not worth it at this point, I think, and not sure if the kernel is sufficiently new for the latest ivtv-utils or not).

Comment 48 Fedora Update System 2008-12-02 20:20:47 UTC
ivtv-utils-1.3.0-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/ivtv-utils-1.3.0-1.fc9

Comment 49 Fedora Update System 2008-12-02 20:21:30 UTC
ivtv-utils-1.3.0-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/ivtv-utils-1.3.0-1.fc10

Comment 50 Fedora Update System 2008-12-07 04:22:30 UTC
ivtv-utils-1.3.0-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 51 Fedora Update System 2008-12-07 04:26:00 UTC
ivtv-utils-1.3.0-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.