Bug 559117

*** Bug 241099 has been marked as a duplicate of this bug. ***

Assigning.

- The compiler commands that are used are not visible, so the usage of optimization flags cannot be verified. Add
 --debug=presub
to the build command.

- I'd combine COPYING and README to a one-liner, 
 %doc COPYING README
The marking of the man page as %doc is an issue of taste, however you should replace the .gz to .* in case the man page compression format changes in the future.


Review:

rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. N/A

MUST: A package must own all directories that it creates or require the package that owns the directory. NEEDSWORK
- %{_datadir}/icons/hicolor/ is provided by hicolor-icon-theme, which is not provided by default. Either add Requires: hicolor-icon-theme, or move the icon to %{_datadir}/pixmaps/ which is a standard system directory.

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. OK
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Do another unofficial review and apply for membership in the packager group in FAS, and you can consider yourself sponsored.

Without adding the proposed "--debug=presub", SCons does output the commands used for compiling and installing to standard out.  I just did a "rpmbuild -ba lcdtest.spec >lcdtest.log 2>&1", and lcdtest.log contains:

gcc -o build/lcdtest.o -c -O2 -g -pipe -Wall -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wp,-D_FORTIFY_SOURCE=2 -DRELEASE=1.18 src/lcdtest.c
gcc -o build/lcdtest build/lcdtest.o -lSDL -lSDL_image -lSDL_ttf

That looks like the same kind of output I'd expect from make; do you still want the --debug=presub added?

I'll make the other changes you're suggested, and post updated spec and SRPM files for this and levmar once I've done another review.

Thanks!

(In reply to comment #5)
> That looks like the same kind of output I'd expect from make; do you still want
> the --debug=presub added?

Looks as if I were blind. So everything is OK, you don't need to add the debug switch.

OK.  Regarding the icon, I'll move it into /usr/share/pixmaps as recommended, but I thought I'd point out why I put it in /usr/share/icons/hicolor in the first place.  From the freedesktop.org Icon Theme Specification:

http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-0.11.html

"So, you're an application author, and want to install application icons so that they work in the KDE and Gnome menus. Minimally you should install a 48x48 icon in the hicolor theme. This means installing a PNG file in $prefix/share/icons/hicolor/48x48/apps."

I can't see any way to interpret this that allows for the hicolor theme to be optional; it seems that if Fedora intends to be compliant with freedesktop.org standards, the hicolor theme *must* be provided.

Anyhow, I'm not pushing for any change in Fedora policy, but since Fedora icon policy differs from the freedesktop.org standard, it might be desirable to update the desktop section of the packaging policy to explicitly state that the basic fedora icon requirement is to use /usr/share/pixmaps.  That might prevent another novice packager from making the same mistake I did.

Here's the updated lcdtest package:

Spec URL:
http://www.brouhaha.com/~eric/software/fedora/f12/lcdtest/lcdtest.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/f12/lcdtest/lcdtest-1.18-2.fc12.src.rpm
Koji scratch build for f12-dist:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1948905

Oops, then I forgot your request to change the files line for the man page.  Trying again:

Spec URL:
http://www.brouhaha.com/~eric/software/fedora/f12/lcdtest/lcdtest.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/f12/lcdtest/lcdtest-1.18-3.fc12.src.rpm
Koji scratch build for f12-dist:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1948930

(In reply to comment #7)
> OK.  Regarding the icon, I'll move it into /usr/share/pixmaps as recommended,
> but I thought I'd point out why I put it in /usr/share/icons/hicolor in the
> first place.  From the freedesktop.org Icon Theme Specification:

Hmm, I'm no expert, but the beginning of the "Directory Layout" section states:

"Icons and themes are looked for in a set of directories. By default, apps should look in $HOME/.icons (for backwards compatibility), in $XDG_DATA_DIRS/icons and in /usr/share/pixmaps (in that order)."

> I can't see any way to interpret this that allows for the hicolor theme to be
> optional; it seems that if Fedora intends to be compliant with freedesktop.org
> standards, the hicolor theme *must* be provided.
> 
> Anyhow, I'm not pushing for any change in Fedora policy, but since Fedora icon
> policy differs from the freedesktop.org standard, it might be desirable to
> update the desktop section of the packaging policy to explicitly state that the
> basic fedora icon requirement is to use /usr/share/pixmaps.  That might prevent
> another novice packager from making the same mistake I did.

OK. If you're interested in packaging standards, you can write up a proposal for the Fedora Packaging Committee, which can then propose the guideline to the Fedora Steering Committee.

**

This package is good to go. I won't give it a formal approval yet, since you wouldn't be able to do anything with it yet since you're not a member of the packaging group.

Do the one other informal review, and I will sponsor you.

This package is

APPROVED

You can continue by requesting the CVS branches.

New Package CVS Request
=======================
Package Name: levmar
Short Description: Displays monitor test patterns
Owners: brouhaha
Branches: F-12
InitialCC:

Got the package name wrong in the CVS request above.  Sorry!

New Package CVS Request
=======================
Package Name: lcdtest
Short Description: Displays monitor test patterns
Owners: brouhaha
Branches: F-12
InitialCC:

CVS done (by process-cvs-requests.py).

lcdtest-1.18-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/lcdtest-1.18-3.fc12

lcdtest-1.18-3.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: lcdtest
New Branches: el6
Owners: brouhaha

Git done (by process-git-requests).

lcdtest-1.18-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/lcdtest-1.18-6.el6

lcdtest-1.18-6.el6 has been pushed to the Fedora EPEL 6 stable repository.