Bug 450816

Informal package review:
========================

-Summary: Teletext decoder/browser
 Name: alevt
 Version: 1.6.2
 Release: 1%{?dist}
-License: GPLv2
+Summary: Teletext decoder/Browser
 Group:  Applications/Multimedia
+License: GPLv2
+URL: http://goron.de/~froese
 Source: http://goron.de/~froese/%{name}/%{name}-%{version}.tar.gz
 Source1: alevt.desktop
 Patch0: alevt-1.6.2-pixmap.patch
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
-URL: http://goron.de/~froese


* Please, keep the usual order of fields. 
* e.g. 
Name:
Version:
Release:
Summary:
Group:
License:
URL:
Source0:
BuildRoot:
BuildRequires:
Requires:


 %build
-#overwrite $OPT to produce debuginfo
-make %{?_smp_mflags} -e OPT="-O2 -w"
+#overwrite $OPT to produce standard Fedora build with propper FLAGS
+
+# alevt does not have standard build system, so we call configure
+# to populate CFLAGS, then we move them to another var which persists
+# and we *unset CFLAGS* and have alevt build system to populate it
+%configure || true
+FLAGS=${CFLAGS}
+unset CFLAGS
+# will produce lot of garbage on output
+make %{?_smp_mflags} -e OPT="${FLAGS}"

* Hope it's selfdescriptive.
* The debuginfos were somewhat useless without the "-g" option. Now it uses
Fedora's own CFLAGS automatically. 


 %install
 rm -rf %{buildroot}
-mkdir -p $RPM_BUILD_ROOT/%{_bindir}
-mkdir -p $RPM_BUILD_ROOT/%{_mandir}/man1
+mkdir -p %{buildroot}%{_bindir}
+mkdir -p %{buildroot}%{_mandir}/man1
 make USR_X11R6=/usr MAN=share/man rpm-install
 desktop-file-install --vendor="fedora" \
-	--dir=${RPM_BUILD_ROOT}%{_datadir}/applications %{SOURCE1}
+	--dir=%{buildroot}%{_datadir}/applications %{SOURCE1}


* Be consistent in using macro style v. variable style 
 
 %clean
@@ -42,9 +50,7 @@ rm -rf %{buildroot}
 %{_bindir}/alevt-date
 %{_bindir}/alevt-cap
 %{_datadir}/applications/*%{name}.desktop
-%doc %{_mandir}/man1/alevt.1x.gz
-%doc %{_mandir}/man1/alevt-date.1.gz
-%doc %{_mandir}/man1/alevt-cap.1.gz
+%{_mandir}/man?/%{name}*
 %{_datadir}/pixmaps/mini-alevt.xpm
 %doc README CHANGELOG COPYRIGHT
 

* The one line will rule them all :)

@@ -62,6 +68,7 @@ rm -rf %{buildroot}
 
 * Sun May 23 1999 Karsten Hopp <karsten>
 - several minor patches of Marios spec-file:
-  german descriptions
-  buildroot (patched Makefile)
-  some changed install-paths
+- german descriptions
+- buildroot (patched Makefile)
+- some changed install-paths
+


* Just typo-enhancements 
--

Incorporate the changes, please, so the package is closer to formal review.

Debuginfos:
===========

* See the differences in sizes (first column in Bytes), the more is "better" :).

Your old OPT="...":
-------------------
   8348  /usr/lib/debug/usr/bin/alevt-cap.debug
   6352  /usr/lib/debug/usr/bin/alevt-date.debug
  12780  /usr/lib/debug/usr/bin/alevt.debug

With "... -g":
----------
  71760  /usr/lib/debug/usr/bin/alevt-cap.debug
  37412  /usr/lib/debug/usr/bin/alevt-date.debug
 149152  /usr/lib/debug/usr/bin/alevt.debug

Fedora CFLAGS:
--------------
  75376  /usr/lib/debug/usr/bin/alevt-cap.debug
  39556  /usr/lib/debug/usr/bin/alevt-date.debug
 156104  /usr/lib/debug/usr/bin/alevt.debug

(In reply to comment #1)


> * The debuginfos were somewhat useless without the "-g" option. Now it uses
> Fedora's own CFLAGS automatically. 

My intention was to use -g from the beginning -w was there by mistake.
Anyway the best aproach is fedora's own CFLAGS.

I've modified the file and bump release:
http://lucilanga.fedorapeople.org/alevt.spec
http://lucilanga.fedorapeople.org/alevt-1.6.2-2.fc9.src.rpm

Proposing this patch, which is more fedora best practices aware:

* still populates the OPT variable but without invoking %configure (which fails
right after)
* less variable playing/messing 
* updated comment


--- alevt.spec  2008-06-29 19:26:04.000000000 +0200
+++ alevt.spec_new      2008-06-30 10:36:39.000000000 +0200
@@ -23,14 +23,11 @@ This program decodes and displays Videot
 %build
 #overwrite $OPT to produce standard Fedora build with propper FLAGS
 
-# alevt does not have standard build system, so we call configure
-# to populate CFLAGS, then we move them to another var which persists
-# and we *unset CFLAGS* and have alevt build system to populate it
-%configure || true
-FLAGS=${CFLAGS}
-unset CFLAGS
+# alevt does not have standard build system, so we populate OPT, 
+# which is internal build variable to accomodate Fedora opt flags
+
 # will produce lot of garbage on output
-make %{?_smp_mflags} -e OPT="${FLAGS}"
+make %{?_smp_mflags} -e OPT="%{optflags}"
 
 
 %install

Updated spec file and bump release:

http://lucilanga.fedorapeople.org/alevt.spec
http://lucilanga.fedorapeople.org/alevt-1.6.2-3.fc9.src.rpm

> make USR_X11R6=/usr MAN=share/man rpm-install

Omit hardcoded paths like "/usr". Better is to use RPM macros
http://fedoraproject.org/wiki/Packaging/RPMMacros This is mandatory.

> desktop-file-install --vendor="fedora" \
> 	--dir=%{buildroot}%{_datadir}/applications %{SOURCE1}

'--vendor="fedora"' is going to be obsoleted in short time, remove it (even that
Package guidelines says otherwise)

(In reply to comment #6)


> Omit hardcoded paths like "/usr". Better is to use RPM macros
> http://fedoraproject.org/wiki/Packaging/RPMMacros This is mandatory.
updated to use %{prefix}
(USR_X11R6 is mainly used for for includes, package's install will correctly
pickup buildroot.)


> '--vendor="fedora"' is going to be obsoleted in short time, remove it (even that
> Package guidelines says otherwise)
dropped it.

... and bumped version
new files:
http://lucilanga.fedorapeople.org/alevt.spec
http://lucilanga.fedorapeople.org/alevt-1.6.2-4.fc9.src.rpm

Thanks for prompt changes, will do official review soon.

[root@dhcp-lab-192 SPECS]# diff -pruN alevt.spec_bak alevt.spec
--- alevt.spec_bak	2008-07-28 17:43:35.364713214 +0200
+++ alevt.spec	2008-07-28 18:13:27.216716710 +0200
@@ -1,32 +1,36 @@
 Name: alevt
 Version: 1.6.2
 Release: 4%{?dist}
-Summary: Teletext decoder/browser
+Summary: This program decodes and displays Videotext/Teletext from a /dev/vbi
device
 Group:  Applications/Multimedia
 License: GPLv2
 URL: http://goron.de/~froese
 Source: http://goron.de/~froese/%{name}/%{name}-%{version}.tar.gz
 Source1: alevt.desktop
 Patch0: alevt-1.6.2-pixmap.patch
+Patch1: alevt-1.6.2-manpath.patch
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 BuildRequires: libX11-devel
 BuildRequires: libpng-devel
 BuildRequires: desktop-file-utils
 
 %description
-This program decodes and displays Videotext/Teletext from a /dev/vbi device.
+AleVT is a teletext/videotext decoder and browser for the
+bttv driver (/dev/vbi) and X11.  It features multiple windows,
+a page cache, regexp searching, built-in manual, and more.
+There's also a program to get the time from teletext and
+one to capture teletext pages from scripts.
+
 
 %prep
 %setup -q
 %patch0 -p1 -b .pixmap
+%patch1 -p1 -b .manpath
 
 %build
-#overwrite $OPT to produce standard Fedora build with propper FLAGS
-
 # alevt does not have standard build system, so we populate OPT, 
-# which is internal build variable to accomodate Fedora opt flags
-
-# will produce lot of garbage on output
+# which is internal build variable to accommodate Fedora opt flags
+# This will produce lot of garbage on output.
 make %{?_smp_mflags} -e OPT="%{optflags}"
 
 
@@ -35,7 +39,7 @@ rm -rf %{buildroot}
 mkdir -p %{buildroot}%{_bindir}
 mkdir -p %{buildroot}%{_mandir}/man1
 
-make USR_X11R6=%{_prefix} MAN=share/man rpm-install
+make USR_X11R6=%{_prefix} MAN=%{_mandir} rpm-install
 desktop-file-install \
 	--dir=%{buildroot}%{_datadir}/applications %{SOURCE1}
 


* It's mostly description, summary and comments changes. Feel free to tweak them. 

* There's bigger change according to hard-coded man path in MAN variable, now,
with patch, it's in Fedora-macro-way.


[root@dhcp-lab-192 BUILD]# cat ../SOURCES/alevt-1.6.2-manpath.patch 
--- alevt-1.6.2/Makefile	2008-07-28 18:08:04.778713433 +0200
+++ alevt-1.6.2/Makefile_manpath	2008-07-28 18:09:28.572715966 +0200
@@ -66,9 +66,9 @@ rpm-install: all
 	install -m 0755 alevt        ${RPM_BUILD_ROOT}$(USR_X11R6)/bin
 	install -m 0755 alevt-date   ${RPM_BUILD_ROOT}$(USR_X11R6)/bin
 	install -m 0755 alevt-cap    ${RPM_BUILD_ROOT}$(USR_X11R6)/bin
-	install -m 0644 alevt.1x     ${RPM_BUILD_ROOT}$(USR_X11R6)/$(MAN)/man1
-	install -m 0644 alevt-date.1 ${RPM_BUILD_ROOT}$(USR_X11R6)/$(MAN)/man1
-	install -m 0644 alevt-cap.1  ${RPM_BUILD_ROOT}$(USR_X11R6)/$(MAN)/man1
+	install -m 0644 alevt.1x     ${RPM_BUILD_ROOT}$(MAN)/man1
+	install -m 0644 alevt-date.1 ${RPM_BUILD_ROOT}$(MAN)/man1
+	install -m 0644 alevt-cap.1  ${RPM_BUILD_ROOT}$(MAN)/man1
 	install -d 0755 $(RPM_BUILD_ROOT)$(USR_X11R6)/share/pixmaps
 	install -m 0644 contrib/mini-alevt.xpm $(RPM_BUILD_ROOT)$(USR_X11R6)/share/pixmaps


* When is the last point done, I'll finish the review.

I've added your patch (thanks).
I altered the description:

AleVT is a teletext/videotext decoder and browser for the
bttv driver (/dev/vbi) and X11.

to

AleVT is a teletext/videotext decoder and browser for the
vbi (/dev/vbi) device and X11.

because alevt is not driver dependant, it works with any vbi device and 'bttv
driver' seems confusing.

... and bumped version

http://lucilanga.fedorapeople.org/alevt.spec
http://lucilanga.fedorapeople.org/alevt-1.6.2-5.fc9.src.rpm

MUST Items:

[PASS] - MUST: rpmlint must be run on every package. 
[PASS] - MUST: The package must be named according to the Package Naming Guidelines .
[PASS] - MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption on Package Naming Guidelines .
[PASS] - MUST: The package must meet the Packaging Guidelines .
[PASS] - MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[PASS] - MUST: The License field in the package spec file must match the actual license.
[PASS] - MUST: If (and only if) the source package includes the text of the license(s) ...
[PASS] - MUST: The spec file must be written in American English.
[PASS] - MUST: The spec file for the package MUST be legible. If the reviewer is unable to read 
[PASS] - MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. 
[PASS] - MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.

* PASS on i386, under mock and koji

[PASS] - MUST: If the package does not successfully compile, build or work on an architecture, 
[PASS] - MUST: All build dependencies must be listed in BuildRequires

* PASS on i386, under mock and koji

[ NA ] - MUST: The spec file MUST handle locales properly. 
[ NA ] - MUST: Every binary RPM package which stores shared library files
[ NA ] - MUST: If the package is designed to be relocatable, the packager must state 
[PASS] - MUST: A package must own all directories that it creates. 
[PASS] - MUST: A package must not contain any duplicate files in the %files listing.
[PASS] - MUST: Permissions on files must be set properly. Executables should be set with 
[PASS] - MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}
[PASS] - MUST: Each package must consistently use macros, as described in the 
[PASS] - MUST: The package must contain code, or permissable content. 
[ NA ] - MUST: Large documentation files should go in a -doc subpackage. 
[PASS] - MUST: If a package includes something as %doc, it must not affect the runtime of the application. 
[ NA ] - MUST: Header files must be in a -devel package.
[ NA ] - MUST: Static libraries must be in a -static package.
[ NA ]- MUST: Packages containing pkgconfig(.pc) files 
[ NA ] - MUST: If a package contains library files with a suffix 
[ NA ] - MUST: In the vast majority of cases, devel packages must require 
[ NA ] - MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec.
[PASS] - MUST: Packages containing GUI applications must include a %{name}.desktop file
[PASS] - MUST: Packages must not own files or directories already owned by other packages. 
[PASS] - MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}
[PASS] - MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:

[ NA ] - SHOULD: If the source package does not include license text(s) 

* includes

[ NA ] - SHOULD: The description and summary sections in the package spec file should contain 
[mock] - SHOULD: The reviewer should test that the package builds in mock. 
[koji] - SHOULD: The package should compile and build into binary rpms on all supported architectures.
[ NA ] - SHOULD: The reviewer should test that the package functions as described. 

* lack of applicable HW

[ NA ] - SHOULD: If scriptlets are used, those scriptlets must be sane. 
[ NA ] - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[ NA ] - SHOULD: The placement of pkgconfig(.pc) 
[ NA ] - SHOULD: If the package has file dependencies outside of /etc, /bin, 
---


REVIEWED

New Package CVS Request
=======================
Package Name: alevt
Short Description: Teletext decoder/browser
Owners: lucilanga
Branches: F-8 F-9 EL-5
InitialCC: 
Cvsextras Commits: yes

cvs done.

alevt-1.6.2-5.fc8 has been submitted as an update for Fedora 8

alevt-1.6.2-5.fc9 has been submitted as an update for Fedora 9

alevt-1.6.2-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

alevt-1.6.2-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.