Bug 671883

Not a formal review, just some feedback from trying to build this package in rawhide-mock:


1. I am facing several warnings of this kind:

make[2]: Entering directory `/builddir/build/BUILD/v4l-utils-0.8.2-test/utils/libv4l2util'
make[2]: warning: jobserver unavailable: using -j1.  Add `+' to parent make rule.

... no idea about the cause. Wild guess: Broken makefiles.

2. A file does not compile:
cc -Wp,-MMD,"keytable.d",-MQ,"keytable.o",-MP -c -I../../include -I../../lib/include -D_GNU_SOURCE -O2 -g -pipe -Wall 
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -o keytable.o keytable.c
keytable.c:87:66: error: expected ',' or ';' before 'V4L_UTILS_VERSION'

3. After the error from 2. above occurs, make doesn't abort the build process, but continues.


1. + 3. together make be think, something might be basically broken with this package's makefiles.


[Removing msmp_flags seems to work-around 1., but doesn't help 2. and 3.]

(In reply to comment #1)
> Not a formal review, just some feedback from trying to build this package in
> rawhide-mock:
> 

Thanks for the feedback!

> 
> 1. I am facing several warnings of this kind:
> 
> make[2]: Entering directory
> `/builddir/build/BUILD/v4l-utils-0.8.2-test/utils/libv4l2util'
> make[2]: warning: jobserver unavailable: using -j1.  Add `+' to parent make
> rule.
> 
> ... no idea about the cause. Wild guess: Broken makefiles.

Fixed by this upstream commit:
http://git.linuxtv.org/v4l-utils.git?a=commitdiff;h=ad1bfbbea5960243fdaf93a6d3386ff0c65b6337

> 
> 2. A file does not compile:
> cc -Wp,-MMD,"keytable.d",-MQ,"keytable.o",-MP -c -I../../include
> -I../../lib/include -D_GNU_SOURCE -O2 -g -pipe -Wall 
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
> --param=ssp-buffer-size=4 -m64 -mtune=generic -o keytable.o keytable.c
> keytable.c:87:66: error: expected ',' or ';' before 'V4L_UTILS_VERSION'
> 

Ah, this happens when specifying CFLAGS explicitly, fixed by this upstream commit:
http://git.linuxtv.org/v4l-utils.git?a=commitdiff;h=d0b2dcf42230a337943164cd0bc2ebd1c4725543

> 3. After the error from 2. above occurs, make doesn't abort the build process,
> but continues.

Oh, good one! Bad error propagation in a for $SUBDIRS loop, fixed:
http://git.linuxtv.org/v4l-utils.git?a=commitdiff;h=f51ac9f8b4dd0265575fd41ebaa36327309e4ae7

There still is one other issue which needs to be taken care of upstream. Then I'll do a real 0.8.2 release upstream and drop a link to an updated srpm here.

Created attachment 474844 [details]
Version 2 of the v4l-utils.spec

This is the version 2 of the spec. This version, together with some patches
I've added upstream, fixes the build errors and properly installs the RC
keymaps and the udev rules.

Hi All,

Here is a new version, based on the just released official 0.8.2 release, fixing the issues mentioned in comment #1.

Spec URL: http://people.fedoraproject.org/~jwrdegoede/v4l-utils.spec
SRPM URL:
http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.2-2.fc15.src.rpm

A note about the rpmlint output wrt missing manpages we are working on this upstream (for example we added a ir-keytable manpage yesterday).

Regards,

Hans

(In reply to comment #4)
> http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.2-2.fc15.src.rpm

Doesn't build for me (mock -r fedora-rawhide-x86_64 v4l-utils-0.8.2-2.fc15.src.rpm):

..
make[2]: Entering directory `/builddir/build/BUILD/v4l-utils-0.8.2/lib/libv4l1'
cc -Wp,-MMD,"libv4l1.d",-MQ,"libv4l1.o",-MP -c -I../include -fvisibility=hidden -fPIC -I../../include -I../../lib/include -D_GNU_SOURCE -DV4L_UTILS_VERSION='"0.8.2"' -O2 -g -pipe -Wall -Wp,-D_FO
cc -Wp,-MMD,"log.d",-MQ,"log.o",-MP -c -I../include -fvisibility=hidden -fPIC -I../../include -I../../lib/include -D_GNU_SOURCE -DV4L_UTILS_VERSION='"0.8.2"' -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SO
log.c:22:28: fatal error: linux/videodev.h: No such file or directory
compilation terminated.
libv4l1.c:60:28: fatal error: linux/videodev.h: No such file or directory
compilation terminated.
make[2]: *** [log.o] Error 1

(In reply to comment #5)
> (In reply to comment #4)
> > http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.2-2.fc15.src.rpm
> 
> Doesn't build for me (mock -r fedora-rawhide-x86_64
> v4l-utils-0.8.2-2.fc15.src.rpm):
> 
> ..
> make[2]: Entering directory `/builddir/build/BUILD/v4l-utils-0.8.2/lib/libv4l1'
> cc -Wp,-MMD,"libv4l1.d",-MQ,"libv4l1.o",-MP -c -I../include -fvisibility=hidden
> -fPIC -I../../include -I../../lib/include -D_GNU_SOURCE
> -DV4L_UTILS_VERSION='"0.8.2"' -O2 -g -pipe -Wall -Wp,-D_FO
> cc -Wp,-MMD,"log.d",-MQ,"log.o",-MP -c -I../include -fvisibility=hidden -fPIC
> -I../../include -I../../lib/include -D_GNU_SOURCE -DV4L_UTILS_VERSION='"0.8.2"'
> -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SO
> log.c:22:28: fatal error: linux/videodev.h: No such file or directory
> compilation terminated.
> libv4l1.c:60:28: fatal error: linux/videodev.h: No such file or directory
> compilation terminated.
> make[2]: *** [log.o] Error 1

Oops, good point we need to deal with this upstream since linux/videodev.h is going away (deprecated) but for now adding a BuildRequires: kernel-headers fixes this:

Spec URL: http://people.fedoraproject.org/~jwrdegoede/v4l-utils.spec
SRPM URL:
http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.2-3.fc15.src.rpm

I'll start a discussion upstream about how to deal with the v4l1 API stuff libv4l1 needs from videodev.h

oops! sorry, I am not sure what used to happen but this is second time in Chromium browser when I tried to just CC myself, package component reset to first package 0xFFFF

reverting accidental change back.

I've rebased my package to the new upstream release from yesterday:
http://people.fedoraproject.org/~jwrdegoede/v4l-utils.spec
http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.3-1.fc15.src.rpm

I'll review this.

Mostly OK. I think the QT app needs a desktop file (but it might not be an app that shoud) and there should be versioned required on the libs at a guess for the various utils.

- non -devel packages should require fully versioned base
- packages containing GUI apps must include %{name}.desktop file


+ rpmlint output

rpmlint v4l* libv4l-0.8.3-1.fc15.x86_64.rpm libv4l-devel-0.8.3-1.fc15.x86_64.rpm qv4l2-0.8.3-1.fc15.x86_64.rpm
v4l-utils.src: W: spelling-error %description -l en_US ctl -> cyl, ct, cl
v4l-utils.src: W: spelling-error %description -l en_US ir -> IR, Ir, or
v4l-utils.src: W: spelling-error %description -l en_US keytable -> key table, key-table, marketable
v4l-utils.src: W: spelling-error %description -l en_US ivtv -> invt, Ives, Ivan
v4l-utils.src: W: spelling-error %description -l en_US sysfs -> sysops, sysop, systems
v4l-utils.x86_64: W: spelling-error %description -l en_US ctl -> cyl, ct, cl
v4l-utils.x86_64: W: spelling-error %description -l en_US ir -> IR, Ir, or
v4l-utils.x86_64: W: spelling-error %description -l en_US keytable -> key table, key-table, marketable
v4l-utils.x86_64: W: spelling-error %description -l en_US ivtv -> invt, Ives, Ivan
v4l-utils.x86_64: W: spelling-error %description -l en_US sysfs -> sysops, sysop, systems
v4l-utils.x86_64: W: no-manual-page-for-binary v4l2-sysfs-path
v4l-utils.x86_64: W: no-manual-page-for-binary ivtv-ctl
v4l-utils.x86_64: W: no-manual-page-for-binary v4l2-ctl
v4l-utils.x86_64: W: no-manual-page-for-binary cx18-ctl
v4l-utils-devel-tools.x86_64: W: spelling-error %description -l en_US dbg -> db, dg, deg
v4l-utils-devel-tools.x86_64: W: no-manual-page-for-binary decode_tm6000
v4l-utils-devel-tools.x86_64: W: no-manual-page-for-binary v4l2-dbg
v4l-utils-devel-tools.x86_64: W: no-manual-page-for-binary v4l2-compliance
qv4l2.x86_64: W: no-manual-page-for-binary qv4l2
7 packages and 1 specfiles checked; 0 errors, 19 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license
+ latest version packaged

+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  f25ad639717d7411a58f10a9e378d7db  v4l-utils-0.8.3.tar.bz2
+ package successfully builds on at least one architecture
  tested using koji scratch build
  http://koji.fedoraproject.org/koji/taskinfo?taskID=2829854
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun+ does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ Package perserves timestamps on install
+ Permissions on files must be set properly 
+ %defattr line
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package runtime 
+ header files should be in -devel
n/a static libraries should be in -static
- non -devel packages should require fully versioned base
+ packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ devel must require the fully versioned base
+ packages should not contain libtool .la files
- packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ filenames must be valid UTF-8

Optional:

+ if there is no license file, packager should query upstream to include it
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock/koji
+ the package should build into binary RPMs on all supported architectures
n/a review should test the package functions as described
+ scriptlets should be sane
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin
- Package should have man files

(In reply to comment #10)
> Mostly OK. I think the QT app needs a desktop file (but it might not be an app
> that shoud) and there should be versioned required on the libs at a guess for
> the various utils.
...
> - packages containing GUI apps must include %{name}.desktop file

I've created a desktop file upstream:

http://git.linuxtv.org/v4l-utils.git?a=commitdiff;h=82feef4bea16947f9621cd3061f317af2d4bbf27

The descriptions are currently on English and Portuguese only, but it should be easy to later add other descriptions. It is also providing icons on for the common resolutions found in Fedora, plus a svg icon.

(In reply to comment #10)
> Mostly OK. I think the QT app needs a desktop file (but it might not be an app
> that shoud) and there should be versioned required on the libs at a guess for
> the various utils.
> 

Thanks for the review, and sorry for the slow reply (I was swamped with work, on a business trip amongst other things.

> - non -devel packages should require fully versioned base
> - packages containing GUI apps must include %{name}.desktop file
> 

Both fixed, here is a new version:
http://people.fedoraproject.org/~jwrdegoede/v4l-utils.spec
http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.3-2.fc15.src.rpm

(In reply to comment #12)
> (In reply to comment #10)
> > Mostly OK. I think the QT app needs a desktop file (but it might not be an app
> > that shoud) and there should be versioned required on the libs at a guess for
> > the various utils.
> > 
> 
> Thanks for the review, and sorry for the slow reply (I was swamped with work,
> on a business trip amongst other things.
> 
> > - non -devel packages should require fully versioned base
> > - packages containing GUI apps must include %{name}.desktop file
> > 
> 
> Both fixed, here is a new version:
> http://people.fedoraproject.org/~jwrdegoede/v4l-utils.spec
> http://people.fedoraproject.org/~jwrdegoede/v4l-utils-0.8.3-2.fc15.src.rpm

Build fails. You need to add a dep on desktop-file-utils for the desktop file processing.

http://koji.fedoraproject.org/koji/getfile?taskID=2910226&name=build.log

But other than that it all looks OK so APPROVED!

(In reply to comment #13)
> Build fails. You need to add a dep on desktop-file-utils for the desktop file
> processing.

Oh, duh, thanks for catching this. And thanks for the review!

New Package SCM Request
=======================
Package Name: v4l-utils
Short Description: Utilities for video4linux and DVB devices
Owners: jwrdegoede
Branches: f14 f15
InitialCC:

Git done (by process-git-requests).

v4l-utils-0.8.3-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/v4l-utils-0.8.3-2.fc15

v4l-utils-0.8.3-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/v4l-utils-0.8.3-2.fc14

Package Change Request
======================
Package Name: v4l-utils
New Branches: el6
Owners: mchehab
InitialCC: jwrdegoede

Hi,

Mauro (mchehab) would like to see an epel version of v4l-utils, and is willing
to maintain it. This is fine with me.

Regards,

Hans

Git done (by process-git-requests).

v4l-utils-0.8.3-2.fc15 has been pushed to the Fedora 15 testing repository.

v4l-utils-0.8.3-2.fc15 has been pushed to the Fedora 15 stable repository.

v4l-utils-0.8.3-2.fc14 has been pushed to the Fedora 14 stable repository.