Bug 26

I request some assistance with improving the package:

1. Since I'm using a post release svn checkout, and the inbuilt ffmpeg is not being used {ie linking against ffmpeg bits instead}, should I rm -Rf ffmpeg.src before .tar.bz up the source ? This would make the source a lot smaller !

2. I have ended up with the i386 binary including two copies of the %doc files. If I comment that line out - then I get no doc files at all ! Why ?

3. Building the spec into .src.rpm and .i386.rpm was done on a 386 guest virtual machine on fedora-development {up2date 2008-02-08}. A spec I initially looked at from packman suggested adjustments are needed for x86_64. Can some one confirm build works on that and on ppc archs ?

4. While debuginfo is built, installing the -debuginfo rpm, gdb dvcut and run shows messages indicating debuginfo can't be found. I had a look through:
http://fedoraproject.org/wiki/Packaging/Debuginfo
Did my failed gdb test achiev it's purpose ? Do I need to either fix or remove the debuginfo config ?

(In reply to comment #1)
> I request some assistance with improving the package:
> 
> 1. Since I'm using a post release svn checkout, and the inbuilt ffmpeg is not
> being used {ie linking against ffmpeg bits instead}, should I rm -Rf ffmpeg.src
> before .tar.bz up the source ? This would make the source a lot smaller !
> 
> 2. I have ended up with the i386 binary including two copies of the %doc files.
> If I comment that line out - then I get no doc files at all ! Why ?

I recently built this on F7, the install section looks like this:
%install
rm -rf $RPM_BUILD_ROOT
make install PREFIX=%{_prefix} MANPATH=%{_mandir} \
 DESTDIR=$RPM_BUILD_ROOT FFMPEG=%{_includedir}/ffmpeg

MANPATH=%{_mandir} will do the right thing.

> 3. Building the spec into .src.rpm and .i386.rpm was done on a 386 guest
> virtual machine on fedora-development {up2date 2008-02-08}. A spec I initially
> looked at from packman suggested adjustments are needed for x86_64. Can some
> one confirm build works on that and on ppc archs ?
> 
> 4. While debuginfo is built, installing the -debuginfo rpm, gdb dvcut and run
> shows messages indicating debuginfo can't be found. I had a look through:
> http://fedoraproject.org/wiki/Packaging/Debuginfo
> Did my failed gdb test achiev it's purpose ? Do I need to either fix or remove
> the debuginfo config ?
> 

I also added a patch for debuginfo ie -g switch 

--- SConstruct	2008-02-05 21:07:01.000000000 +0000
+++ SConstruct.new	2008-02-05 22:33:10.000000000 +0000
@@ -45,7 +45,7 @@ if (debug>0):
   env.Append(CCFLAGS=['-g3','-Wall'])
   env.Append(LINKFLAGS=['-g3'])
 else:
-  env.Append(CCFLAGS=['-O3','-Wall'])
+  env.Append(CCFLAGS=['-O3','-Wall','-g'])
 
 env.Replace(CXXFILESUFFIX=".cpp")

In fact I'll attach the full spec to give you ideas.

Created attachment 4 [details]
dvbcut.dex.spec

(In reply to comment #2)
> In fact I'll attach the full spec to give you ideas.

dexter: were you planning on submitting the package to rpmfusion ?

Meanwhile: Updated spec/srpm with help from dexter's spec.
- patch SContruct to generate decent debuginfo
- fix install to use make install. Fixes docs install problem.
- add man to files
- del BuildRequires gettext desktop-file-utils since they aren't required.
- del Requires qt ffmpeg since rpm should take care of that.

spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
src:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.3.20080217svn116.fc9.src.rpm

(In reply to comment #4)
> - del BuildRequires gettext desktop-file-utils since they aren't required.

You are using desktop-file-install in %install so desktop-file-utils is required. Without it the command maybe not available (or is not available if you are using mock) and the build would fail.

(In reply to comment #5)
> (In reply to comment #4)
> > - del BuildRequires gettext desktop-file-utils since they aren't required.
> 
> You are using desktop-file-install in %install so desktop-file-utils is
> required. Without it the command maybe not available (or is not available if
> you are using mock) and the build would fail.

Thanks for picking that up. I had noticed in a Fedora review that the reviewer has said to remove BR: d-f-u, so thought that I should do the same. Rereading the Fedora packaging guidelines shows that I do need to include it.

Updated spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.4.20080314svn118.fc9.src.rpm

- add BuildRequires desktop-file-utils so that it builds properly in mock.
- update to new upstream svn version.
- drop patch0-4 since similar have been committed upstream.
{those patches were mainly removing warnings and allow build to complete using gcc-4.3.

Updated spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.7.20080621svn131.fc9.src.rpm
An F9 i386 build is available at:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.7.20080621svn131.fc9.i386.rpm

Picks up latest changes to the svn source code.

Any thing left for me to take care of with this package ?

I would be needing a reviewer and sponsor since this is my first rpmfusion package. For some previous packaging work, see:
- fedora glglobe {first package}:
https://bugzilla.redhat.com/show_bug.cgi?id=249992

- fedora pyvnc2swf {under review}:
https://bugzilla.redhat.com/show_bug.cgi?id=448201

- reviewing python storm {db library}
https://bugzilla.redhat.com/show_bug.cgi?id=430429

Dex: is there anything from your spec that you think should make it into this ?

(In reply to comment #7)
> I would be needing a reviewer and sponsor since this is my first rpmfusion
> package. 

You don't need a sponsor because you have been already sponsored in Fedora.

Just a slight addition, a name change for qt between f8 & f9
qt in f9 is == qt4 in f8 hence somthing like this should do the trick:

%if 0%{fedora} > 8
Buildrequires qt3-devel
%else
Buildrequires qt-devel
%endif

will ensure it builds against qt3 for current releases, remove when f8 eol.
I'll try a build and install later and get back.  

(In reply to comment #9)
> Just a slight addition, a name change for qt between f8 & f9
> qt in f9 is == qt4 in f8 hence somthing like this should do the trick:
I was going to argue that it compiles just fine against f9 qt{==qt4}, but this isn't the case. I didn't realize I had only qt3-devel installed, until:
$ rpm -q --provides qt-devel
libarthurplugin.so
libcontainerextension.so
libcustomwidgetplugin.so
libqt3supportwidgets.so
libtaskmenuextension.so
libworldtimeclockplugin.so
qt4-designer = 4.3.5-2.fc9
qt4-devel = 4.3.5-2.fc9
qt-devel = 1:4.3.5-2.fc9

$ rpm -q --provides qt3-devel
qt-devel = 1:3.3.8b-12.fc9
qt3-devel = 3.3.8b-12.fc9

Since dvbcut needs qt3, but both qt{3}-devel packages provide qt-devel, I confirm that we need conditional Build-Requires.

updated spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.7.20080621svn131.fc9.src.rpm

- add conditional build requires to ensure it builds against correct qt[3]-devel

Is there a reviewer willing to formally review this package ?

(In reply to comment #10)
> srpm: oops ;)
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.8.20080621svn131.fc9.src.rpm

(In reply to comment #8)

> You don't need a sponsor because you have been already sponsored in Fedora.

Calling anyone prepared to do the official review: perhaps you have a package awaiting same ?

(In reply to comment #12)
> Calling anyone prepared to do the official review: perhaps you have a package
> awaiting same ?

Try to swap this review contacting other sponsored packager with pending reviews in RPM Fusion Bugzilla.

Some quick notes:

* Compile process does not use {%optflags}. At least this is what I see when compiling the first file because...

* ...it does not build at least under F9/i386.

* You may want to have a look at these patches:
http://viewcvs.gentoo.org/viewcvs.py/gentoo-x86/media-video/dvbcut/files/

* Why are packaging an svn snapshot? What are the pro's against latest release? Is this snapshot stable? Is a more recent snapshot better than the one used?

* Missing Requires: hicolor-icon-theme. This is required because %{_datadir}/icons/hicolor/ directories are owned by this package.

* Desktop file has the following problems:
 - Icon=dvbcut.logo.24x24
   In this way, 16x16 and 32x32 icons will never be used. Install the icons with the same name under different directory and edit the Icon entry accordingly.
 - X-Desktop-File-Install-Version=0.4
   This is something that should write desktop-file-install and should not be present in the source.
 - Name[en_AU]=dvbcut
   I think this is not needed. It is the same as the default name

* Desktop file install should not be installed with a vendor name.

* The conditional based on Fedora version should be removed since in less than a month all Fedora maintained versions will be > 8.

* If there are no translations, remove the related commented lines.

(In reply to comment #14)
> * Missing Requires: hicolor-icon-theme. This is required because
> %{_datadir}/icons/hicolor/ directories are owned by this package.

this would not be required if the package would own that directory ... ;-)

(In reply to comment #15)
> this would not be required if the package would own that directory ... ;-)
But I can't do that, can I, since on a real system that package would already be owned by another package (ie hicolor-icon-theme) ?

(In reply to comment #14)
> Some quick notes:
> 
> * Compile process does not use {%optflags}. At least this is what I see when
> compiling the first file because...
> 
> * ...it does not build at least under F9/i386.
Interesting since I have done most of my dev on F8, then F9 i386.

> * You may want to have a look at these patches:
> http://viewcvs.gentoo.org/viewcvs.py/gentoo-x86/media-video/dvbcut/files/
Shall do.

> * Why are packaging an svn snapshot? What are the pro's against latest
> release?
Latest release is 18 months old. As I found bugs, or patches (eg gcc43 warns), include path changes I was submitting them, and they were being accepted or alternate fixes incorporated. One item was to do with paths and file names, but it's all fading from memory now.

> Is this snapshot stable?
I have been using the program as packaged since building it (on F9, i386).

> Is a more recent snapshot better than the one used?
If you think it reasonable, I'll make the adjustments you suggest in the second half of your notes, leaving it at the same svn revision.

Then I'll update to latest svn. {note that the author(s) made a little noise about a release about 6 months ago, but nothing has been forthcoming, there hasn't been much svn activity either.

Third thing and separately, I'll change to building with make, autoconf etc. This was introduced into svn about june when various users of the package kept having trouble getting it to build with SCons (along with me). So this will be quite a change to the spec.

The other thing that's happening is rawhide will soon get a new ffmpeg apparently, which will probably bust thing up ...

If you were to do full review, would you want to see the intermediate steps, or just the final converted to be current svn + autotool build changes ?

(In reply to comment #16)
> (In reply to comment #15)
> > this would not be required if the package would own that directory ... ;-)
> But I can't do that, can I, since on a real system that package would already
> be owned by another package (ie hicolor-icon-theme) ?

Exactly. You must require hicolor-icon-theme.

(In reply to comment #17)
> If you were to do full review, would you want to see the intermediate steps, or
> just the final converted to be current svn + autotool build changes ?

I'd like to see the final result. Because so many things (e.g. building system) will be different it make little sense in reviewing intermediate steps.

Ping me for reviewing it :)

(In reply to comment #19)
> Ping me for reviewing it :)

updated spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.11.20081218.fc9.src.rpm

lots o' changes.

$ rpmlint -i /home/davidt/rpmbuild/SRPMS/dvbcut-0.5.4-0.11.20081218.fc9.src.rpm /home/davidt/rpmbuild/RPMS/i386/dvbcut-0.5.4-0.11.20081218.fc9.i386.rpm /home/davidt/rpmbuild/RPMS/i386/dvbcut-debuginfo-0.5.4-0.11.20081218.fc9.i386.rpm
dvbcut.src:55: W: rpm-buildroot-usage %build ./configure --libdir=%{_libdir} --with-ffmpeg=%{_prefix} --with-ffmpeg-include=%{_includedir}/ffmpeg/ --prefix=%{buildroot}%{_prefix}
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it will
break short circuiting.

- not sure how to resolve that - if I remove %{buildroot} from --prefix=, then it won't install. Reconfigure with that buildroot part during install also doesn't make a successful build. Need help !


dvbcut.src: W: strange-permission dvbcut-snapshot.sh 0775
A file that you listed to include in your package has strange permissions.
Usually, a file should have 0644 permissions.
  
- script based on ffmpeg-snapshot script for making a dated svn snapshot archive. Is it allowed to be in the package ? Can I set it to a-x ?


dvbcut-debuginfo.i386: E: empty-debuginfo-package
This debuginfo package contains no files.  This is often a sign of binaries
being unexpectedly stripped too early during the build, rpmbuild not being
able to strip the binaries, the package actually being a noarch one but
erratically packaged as arch dependent, or something else.  Verify what the
case is, and if there's no way to produce useful debuginfo out of it, disable
creation of the debuginfo package.

- -g is passed as argument to gcc, along with the RPM_OPT_FLAGS, but this seems to be getting removed. Not sure yet what's causing that.

3 packages and 0 specfiles checked; 1 errors, 2 warnings.

updated spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.12.20081218.fc9.src.rpm

- fix generation of debuginfo
- test rebuild against ffmpeg-libs-0.4.9-0.57.20081217.fc11

rpmlint: src and bin
dvbcut.src:56: W: rpm-buildroot-usage %build ./configure --libdir=%{_libdir} --with-ffmpeg=%{_prefix} --with-ffmpeg-include=%{_includedir}/ffmpeg/ --prefix=%{buildroot}%{_prefix}

dvbcut.src: W: strange-permission dvbcut-snapshot.sh 0775

3 packages and 0 specfiles checked; 0 errors, 2 warnings.

updated spec:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-0.13.20081218.fc9.src.rpm

- remove execute permission from dvbcut-snapshot.sh
- add re-configure in %%install to move installdir reference from %%build

rpmlint is now clean.

I still cannot build on F10/x86_64. I get the following error when running configure:
[...]
checking for getpagesize... yes
checking for working mmap... yes
checking for main in -lqt-mt... no
checking for main in -lqt... no
configure: error: Qt library not found
errore: Stato d'uscita errato da /var/tmp/rpm-tmp.vCa3lr (%build)

Why aren't you using the configure macro? Doesn't "make install DESTDIR=%{buildroot}" work? I really don't like calling ./configure in the build section.

Desktop file install should not be installed with a vendor name.

Release tag is not correct. Version 0.5.4 has already been release.. Your are using a pre-release scheme instead of a post release scheme.
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Release

(In reply to comment #23)
> I still cannot build on F10/x86_64.
"* ...it does not build at least under F9/i386."
Is that a different machine ?
Do you think this will be missing BR's ? (might save me some time trying to work out why). I build on F9 i386 and F10 i386, OK. (no x86_64 at the moment).

> I get the following error when running
> configure:
> [...]
> checking for getpagesize... yes
> checking for working mmap... yes
> checking for main in -lqt-mt... no
> checking for main in -lqt... no
> configure: error: Qt library not found
> errore: Stato d'uscita errato da /var/tmp/rpm-tmp.vCa3lr (%build)
Is that with qt3-devel installed ?
(can you rpm -qa qt3\* |sort for me ?)

> Why aren't you using the configure macro?
Forgot it existed ;-) using it now in build.

> Doesn't "make install DESTDIR=%{buildroot}" work?
No. So I rerun configure to attach the buildroot info there. I might be able to push a build sys patch upstream if I knew why that doesn't work.

> I really don't like calling ./configure in the build section.
Do you mean ./configure should be in %setup ? Or to use %configure ?

> Desktop file install should not be installed with a vendor name.
Changed to --vendor="".

> Release tag is not correct. Version 0.5.4 has already been release.. Your are
> using a pre-release scheme instead of a post release scheme.
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Release
Oops, I misread the first few lines of the "complex example" as being part of versioning for a post release.
Fixed.

spec: http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-1.20081218.fc9.src.rpm

(In reply to comment #24)
> (In reply to comment #23)
> > I still cannot build on F10/x86_64.
> "* ...it does not build at least under F9/i386."
> Is that a different machine ?

Yes, of course :)

> Do you think this will be missing BR's ? (might save me some time trying to
> work out why). I build on F9 i386 and F10 i386, OK. (no x86_64 at the moment).

It certainly is a missing BR if you can build it.

> > I get the following error when running
> > configure:
> > [...]
> > checking for getpagesize... yes
> > checking for working mmap... yes
> > checking for main in -lqt-mt... no
> > checking for main in -lqt... no
> > configure: error: Qt library not found
> > errore: Stato d'uscita errato da /var/tmp/rpm-tmp.vCa3lr (%build)
> Is that with qt3-devel installed ?
> (can you rpm -qa qt3\* |sort for me ?)

I don't have access to the the F10/x86_64 computer right now, but qt3-devel was installed. I had to install it otherwise rpmbuild refuses to start the build because qt3-devel is amongst BR.

I'll later send you the output of "rpm -qa qt3\* |sort".
 
> > Why aren't you using the configure macro?
> Forgot it existed ;-) using it now in build.

Mmmm... I think you have to review how it works. It is clear you miss something. Try running this command from command line to see how the macro expands:

$ rpm --eval %configure

Thus you'll see that you don't have to pass --libdir=%{_libdir} because it is already passed by the macro.

export CXXFLAGS="$RPM_OPT_FLAGS" is already in the macro and not needed.

> > Doesn't "make install DESTDIR=%{buildroot}" work?
> No. So I rerun configure to attach the buildroot info there. I might be able to
> push a build sys patch upstream if I knew why that doesn't work.

I'll try to help you but I have to be able to fire a build :)

> > I really don't like calling ./configure in the build section.
> Do you mean ./configure should be in %setup ? Or to use %configure ?

I mean that ./configure or %configure should only be used in the %setup section.

(In reply to comment #24)
> (can you rpm -qa qt3\* |sort for me ?)

$ rpm -qa qt3\* |sort
qt3-3.3.8b-17.fc10.x86_64
qt3-devel-3.3.8b-17.fc10.x86_64

Problem is that both libqt and libqt-mt are not found. On my system I have libqt-mt but not the former with the BR you require.

$ locate libqt
/usr/lib64/qt-3.3/lib/libqt-mt.prl
/usr/lib64/qt-3.3/lib/libqt-mt.so
/usr/lib64/qt-3.3/lib/libqt-mt.so.3
/usr/lib64/qt-3.3/lib/libqt-mt.so.3.3
/usr/lib64/qt-3.3/lib/libqt-mt.so.3.3.8
[... other non qt stuff ...]

There's no libqt.so in Fedora, it's the obsolete build of Qt with thread support disabled, nothing should try to link that version. qt-mt is what should be linked to in Qt 3 apps. (That said, somebody ought to port that app to Qt 4, but that's not the point.)

But I think it only tries -lqt because it doesn't find -lqt-mt.

(In reply to comment #28)
> There's no libqt.so in Fedora, it's the obsolete build of Qt with thread
> support disabled, nothing should try to link that version. qt-mt is what should
> be linked to in Qt 3 apps. (That said, somebody ought to port that app to Qt 4,
> but that's not the point.)
Thanks for the info.

> But I think it only tries -lqt because it doesn't find -lqt-mt.
=====
checking a52dec/a52.h usability... yes
checking a52dec/a52.h presence... yes
checking for a52dec/a52.h... yes
checking for an ANSI C-conforming const... yes
checking for off_t... yes
checking for size_t... yes
checking for stdlib.h... (cached) yes
checking for unistd.h... (cached) yes
checking for getpagesize... yes
checking for working mmap... yes
checking for main in -lqt-mt... yes
configure: creating ./config.status
config.status: creating makefile
config.status: WARNING:  makefile.in seems to ignore the --datarootdir setting
config.status: creating src/Makefile
config.status: WARNING:  src/Makefile.in seems to ignore the --datarootdir setting
+ make -j3
make -C src all
make[1]: Entering directory `/home/davidt/rpmb
=====
Looks like x86_64 issue ? I'll try to mock and get a x86_64 going.

(In reply to comment #29)
> Looks like x86_64 issue ? I'll try to mock and get a x86_64 going.

It seems to me that the problem is that configure looks for the library in /usr/lib64/qt-3.3/lib64/ and not in /usr/lib64/qt-3.3/lib/.

(In reply to comment #30)
> It seems to me that the problem is that configure looks for the library in
> /usr/lib64/qt-3.3/lib64/ and not in /usr/lib64/qt-3.3/lib/.

OK, needed to force $QTDIR/lib onto the libpath. Now compiles OK on x86_64. I wonder if that will work for ppc/ppc64 as well. I couldn't find any examples of other qt3 using apps needing to do such trickery, nor improved configure.in examples to improve the existing configure.in.

spec: http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-3.20081218.fc9.src.rpm

- fix x86_64 configure by supplying qt lib dir
- add BR: autoconf to solve mock build issue
- del repeated parameters from build due to change to configure macro

Created attachment 65 [details]
Improved spec file

I've improved your spec file. It is attached to this bug report. Please, let me know what you think.

Changelog:
- removed ugly configure hack in %%install
    A much needed improvement. You must report upstream that there is no clean way to specify a DESTDIR.

- removed %%{?_smp_mflags} from make invocation
    It was because of smp_flags that the rpmbuild process didn't run the setversion.sh script.

- patched configure to fix qt lib dir
    I think it is much cleaner with sed. This patch must be reported upstream.

- cosmetic changes

(In reply to comment #33)
> Changelog:
> - removed ugly configure hack in %%install
>     A much needed improvement. You must report upstream that there is no clean
> way to specify a DESTDIR.
> 
> - removed %%{?_smp_mflags} from make invocation
>     It was because of smp_flags that the rpmbuild process didn't run the
> setversion.sh script.
Is that really a problem with the autoconf/configure/make ?

> - patched configure to fix qt lib dir
>     I think it is much cleaner with sed. This patch must be reported upstream.

Would those two patches be OK for upstream to use ? Will it stop it building in to /usr/local by default it ? (I'm just a bit of a hacker when it comes to this building stuff).

> - cosmetic changes
Yes, I could see that splitting up requires into separate lines could make it easier to check they are correct, and make for simpler future patches.
With the split lines in make, configure, same response, except that indenting by the same amount as a if/looping structure to me suggests that those lines are some how conditional, so I will add an extra indent for those lines.

Builds on x86_64 and i386.
rpmlint clean for x86_64 and i386.

(In reply to comment #34)
> > - removed %%{?_smp_mflags} from make invocation
> >     It was because of smp_flags that the rpmbuild process didn't run the
> > setversion.sh script.
> Is that really a problem with the autoconf/configure/make ?

Well, no. Some makefiles are not meant to be run in parallel. Dvbcut makefile is one of those. That's why I removed the %%{?_smp_mflags} macro from make invocation.

> > - patched configure to fix qt lib dir
> >     I think it is much cleaner with sed. This patch must be reported upstream.
> 
> Would those two patches be OK for upstream to use ? Will it stop it building in
> to /usr/local by default it ? (I'm just a bit of a hacker when it comes to this
> building stuff).

sed -i 's,-L$QTDIR/$mr_libdirname,-L$QTDIR/lib,' configure.in

This just specifies that qt libraries are always under lib subdirectory in $QTDIR. The name of the directory doesn't change depending on arch, only $QTDIR changes depending on arch. As long as other distributions do the same, this is fine.

What I wanted to tell you is that the problems found during this review must be reported upstream.

If you provide an updated package with your latest changes, I'll review it.

spec: http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-5.20081218.fc9.src.rpm

- include Andrea's improvements to the setup, build, and install for review.

(In reply to comment #36)
> http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-5.20081218.fc9.src.rpm

Not found.

buuurp: oops put that beer down ;) Only an hour until new year for me... thanks for your patience, Andrea.
src:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-5.20081218.fc10.src.rpm

Here is the review:

 +:ok, =:needs attention, -:needs fixing, N:not applicable.

MUST Items:
[+] MUST: rpmlint must be run on every package.
	Output is clean.
[-] MUST: The package must be named according to the Package Naming Guidelines.
	Use an %{alphatag} beginning with the date in YYYYMMDD format 
	(this is OK) and followed by up to 16 (ASCII) alphanumeric characters 
	of your choosing (this is not OK). For example: 20081218svn. See:
	http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages
[+] MUST: The spec file name must match the base package %{name}
[+] MUST: The package must meet the Packaging Guidelines.
[=] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
        No problem because this is RPM Fusion.
[-] MUST: The License field in the package spec file must match the actual
license.
	README.icons say that icons are under LGPLv2
	Source files says licences is GPLv2+
	Therefore licence must be GPLv2+ and LGPLv2, See:
	http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios
	http://fedoraproject.org/wiki/Licensing#Good_Licenses_3
[+] MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[-] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL.
	dvbcut-snapshot.sh doesn't seem to work. Maybe instructions are wrong
	or not detailed. I get cannot connect to server error.
[+] MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.
	Tested on F10/x86_64.
[+] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch.
[+] MUST: All build dependencies must be listed in BuildRequires
[N] MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro.
[N] MUST: Every binary RPM package which stores shared library files (not just
symlinks) in any of the dynamic linker's default paths, must call ldconfig in
%post and %postun.
[N] MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review
[+] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
[+] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[-] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.
	In the %files section use either dvbcut or %{name} not both.
[=] MUST: The package must contain code, or permissible content. This is
described in detail in the code vs. content section of Packaging Guidelines.
	This is not a problem since we are in RPM Fusion.
[N] MUST: Large documentation files should go in a doc subpackage.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[N] MUST: Header files must be in a -devel package.
[N] MUST: Static libraries must be in a -static package.
[N] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
[N] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must go in
a -devel package.
[N] MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release} 
[N] MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[N] SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream to include it.
[N] SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
[+] SHOULD: The reviewer should test that the package builds in mock.
	Tested on F10/x86_64
[N] SHOULD: The package should compile and build into binary rpms on all
supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[N] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[N] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and
this is usually for development purposes, so should be placed in a -devel pkg.
A reasonable exception is that the main pkg itself is a devel tool not
installed in a user runtime, e.g. gcc or gdb.
[N] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself.
[=] SHOULD: Packages should try to preserve timestamps of original installed
files.
	Timestamps are not preserved using the original makefile. This should
	be patched.


NEEDSWORK

(In reply to comment #39)
> [-] MUST: The package must be named according to the Package Naming Guidelines.
>         Use an %{alphatag} beginning with the date in YYYYMMDD format 
>         (this is OK) and followed by up to 16 (ASCII) alphanumeric characters 
>         of your choosing (this is not OK). For example: 20081218svn. See:
That could be read to be at 1<=charscount<=16, I guess. I had read the guideline in conjunction with looking at another spec (ffmpeg), which doesn't use an alphatag, there is quite a few packages in RPM Fusion that do this:
BasiliskII, SheepShaver, *madwifi, arcem, autopano-sift-C, ffmpeg, *iscsitarget, larabie, live555, slmodem, vdrsync, x264.

> [-] MUST: The License field in the package spec file must match the actual
> license.
>         README.icons say that icons are under LGPLv2
>         Source files says licences is GPLv2+
>         Therefore licence must be GPLv2+ and LGPLv2, See:
Would this apply ?
"7: LGPLv2.1 gives you permission to relicense the code under any version of the GPL since GPLv2. If you can switch the LGPLed code in this case to using an appropriate version of the GPL instead (as noted in the table), you can make this combination. "

I'm not sure what relicensing would entail: does it mean you have to adjust every file mentioning the LGPL 2.1 to GPL2 or greater ?

In any case, I have done what you initially suggested.

> [-] MUST: The sources used to build the package must match the upstream source,
> as provided in the spec URL.
>         dvbcut-snapshot.sh doesn't seem to work. Maybe instructions are wrong
>         or not detailed. I get cannot connect to server error.
I think this is probably just a temporary connection problem, since it works for me at the moment.

This did lead me to review the script; I adjusted the commented out command that nukes the internally included ffmpeg sources and patches from the svn tree to use a find rather than rm.

> [-] MUST: Each package must consistently use macros, as described in the macros
> section of Packaging Guidelines.
>         In the %files section use either dvbcut or %{name} not both.
Done.

> SHOULD Items:
> [=] SHOULD: Packages should try to preserve timestamps of original installed
> files.
>         Timestamps are not preserved using the original makefile. This should
>         be patched.
Isn't the only file that is built is dvbcut executable ? It would have the time that the rpmbuild %build completed. A moment later %install sticks it in the buildroot. Is that the concern you are raising ?

New spec: 
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
srpm:
http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-6.20090101svn138.fc9.src.rpm

Note the included sources now use the updated -snapshot script, and hence the postrelease date is today's date. This reduces the sources size from 750kB to 180kB.

Thanks for the detailed review :)

(In reply to comment #40)
> > [-] MUST: The package must be named according to the Package Naming Guidelines.
> >         Use an %{alphatag} beginning with the date in YYYYMMDD format 
> >         (this is OK) and followed by up to 16 (ASCII) alphanumeric characters 
> >         of your choosing (this is not OK). For example: 20081218svn. See:
> That could be read to be at 1<=charscount<=16, I guess. I had read the

I think you meant 0<=charscount<=16.

> guideline in conjunction with looking at another spec (ffmpeg), which doesn't
> use an alphatag, there is quite a few packages in RPM Fusion that do this:
> BasiliskII, SheepShaver, *madwifi, arcem, autopano-sift-C, ffmpeg,
> *iscsitarget, larabie, live555, slmodem, vdrsync, x264.

The change you made is OK for me. You may want to use a macro to handle the svn revision.

> > [-] MUST: The License field in the package spec file must match the actual
> > license.
> >         README.icons say that icons are under LGPLv2
> >         Source files says licences is GPLv2+
> >         Therefore licence must be GPLv2+ and LGPLv2, See:
> Would this apply ?
> "7: LGPLv2.1 gives you permission to relicense the code under any version of
> the GPL since GPLv2. If you can switch the LGPLed code in this case to using an
> appropriate version of the GPL instead (as noted in the table), you can make
> this combination. "
> 
> I'm not sure what relicensing would entail: does it mean you have to adjust
> every file mentioning the LGPL 2.1 to GPL2 or greater ?

I think the Fedora guidelines are quite clear about this case: "If your package contains files which are under multiple, distinct, and independent licenses, then the spec must reflect this by using "and" as a separator."

The change you made is OK.

> I think this is probably just a temporary connection problem, since it works
> for me at the moment.
> 
> This did lead me to review the script; I adjusted the commented out command
> that nukes the internally included ffmpeg sources and patches from the svn tree
> to use a find rather than rm.

Yes, it was a temporary problem.

I verified that the content of source archives matches the one created by the snapshot script.

> > SHOULD Items:
> > [=] SHOULD: Packages should try to preserve timestamps of original installed
> > files.
> >         Timestamps are not preserved using the original makefile. This should
> >         be patched.
> Isn't the only file that is built is dvbcut executable ? It would have the time
> that the rpmbuild %build completed. A moment later %install sticks it in the
> buildroot. Is that the concern you are raising ?

No. There is also the man entry, whose timestamp is not preserved. This is a SHOULD item and therefore it is not mandatory.

> New spec: 
> http://members.iinet.net.au/~timmsy/dvbcut/dvbcut.spec
> srpm:
> http://members.iinet.net.au/~timmsy/dvbcut/dvbcut-0.5.4-6.20090101svn138.fc9.src.rpm

Not found. I found dvbcut-0.5.4-6.20090101svn138.fc10.src.rpm thought. I reviewed this.


APPROVED.

Excellent. Thanks to all who had a hand in improving the dvbcut package ;-)

Package CVS request
======================
Package Name: dvbcut
Short Description: Clip and convert DVB transport streams to MPEG2 program streams
Owners: dtimms@iinet.net.au
Branches: F-9 F-10 EL-5
InitialCC:
Cvsextras Commits: yes
----------------------
License tag: free

(In reply to comment #42)
> Package CVS request
> ======================
> Package Name: dvbcut

done

dvbcut has been cvs-imported into devel F-10, F-9, EL-5
Built successfully on Fedora branches. Slight adjustment required for EL, and now succeeds.