Bug 450816 - Review Request: alevt - Teletext decoder/browser
Review Request: alevt - Teletext decoder/browser
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michal Nowak
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-11 04:00 EDT by Lucian Langa
Modified: 2013-03-07 21:04 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-05 12:49:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mnowak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lucian Langa 2008-06-11 04:00:53 EDT
Spec URL: http://lucilanga.fedorapeople.org/alevt.spec
SRPM URL: http://lucilanga.fedorapeople.org/alevt-1.6.2-1.fc9.src.rpm
Description: This program decodes and displays Videotext/Teletext from a /dev/vbi device.

Notes: 
Default target for pixmap file was wrong so it had to be patched.
Also default build didn't produce any debug info so I had to override OPT variable.
Comment 1 Michal Nowak 2008-06-25 15:37:17 EDT
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@delix.de>
 - 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.
Comment 2 Michal Nowak 2008-06-25 15:44:47 EDT
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
Comment 3 Lucian Langa 2008-06-29 13:31:31 EDT
(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

Comment 4 Michal Nowak 2008-06-30 04:41:13 EDT
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
Comment 5 Lucian Langa 2008-06-30 05:25:21 EDT
Updated spec file and bump release:

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

Comment 6 Michal Nowak 2008-07-22 08:58:00 EDT
> 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)
Comment 7 Lucian Langa 2008-07-22 10:52:18 EDT
(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
Comment 8 Michal Nowak 2008-07-23 02:48:27 EDT
Thanks for prompt changes, will do official review soon.
Comment 9 Michal Nowak 2008-07-28 12:21:33 EDT
[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.
Comment 10 Lucian Langa 2008-07-28 13:58:33 EDT
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
Comment 11 Michal Nowak 2008-08-04 10:08:56 EDT
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
Comment 12 Lucian Langa 2008-08-04 10:23:35 EDT
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
Comment 13 Kevin Fenzi 2008-08-04 14:35:38 EDT
cvs done.
Comment 14 Fedora Update System 2008-08-05 12:45:44 EDT
alevt-1.6.2-5.fc8 has been submitted as an update for Fedora 8
Comment 15 Fedora Update System 2008-08-05 12:48:26 EDT
alevt-1.6.2-5.fc9 has been submitted as an update for Fedora 9
Comment 16 Fedora Update System 2008-08-12 14:20:28 EDT
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.
Comment 17 Fedora Update System 2008-08-12 14:24:54 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.