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 Review | Assignee: | Jarod Wilson <jarod> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | high | ||
Version: | rawhide | CC: | 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
starting this review ... 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... 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 Any news ? We are getting short to have it added in Fedora 8 !... (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. (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+. (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 (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. 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. 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 ^^ the last sentence was for the ivtv-firmware Any new Axel ? Requires: perl-Video-ivtv, perl-Video-Frequencies ^^ we cannot have this at least (and others dependencies extracted for perls) ! 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... 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 (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. 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 ) Yes, the perl-Video-* packages need to be removed. Will do so in the next package update. ping ? 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. Is there already pkgs available in updates-testing/koji? I've a system waiting for this and could do some testing. > 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/ 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 ? > 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.
Single card: WinTV-Express, Multi-PAL, 44809 LF, REV E1A5. Combo card: PAL-B/G-I-,D/K-SECAM 23559 LF REV E491. I can't review this package anymore as the submitter and I are in conflict. Reassigned to noby - Feel free to take it Taking over review... Changing summary slightly. This isn't the driver, that's in the kernel... :) Axel, any news on this? :) (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. Any chance we could squeeze it into F9 still? 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? So did anything ever happen here? 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?) (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. :) (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. 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 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 :). 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. 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? I'd say just call it ivtv-utils everywhere, and Obsoletes/Provides ivtv. Hey, that way, you can even drop the epoch... :) 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. 1.3.0 is out… 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: cvs done Was someone going to actually build this? 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). 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 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 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. 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. |