This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 559117 - Review Request: lcdtest - display test pattern generator
Review Request: lcdtest - display test pattern generator
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
: 241099 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-27 03:04 EST by Eric Smith
Modified: 2012-03-28 16:38 EDT (History)
4 users (show)

See Also:
Fixed In Version: lcdtest-1.18-6.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-02-01 20:15:34 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
susi.lehtola: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Eric Smith 2010-01-27 03:04:55 EST
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-1.fc12.src.rpm
Koji scratch build for f12-dist: http://koji.fedoraproject.org/koji/taskinfo?taskID=1947314

Description: 
lcdtest is a utility to display LCD monitor test patterns.  It may be
useful for adjusting the pixel clock frequency and phase on LCD
monitors when using analog inputs, and for finding pixels that are
stuck on or off.

Note: I am not yet sponsored, but Jussi Lehtola has offered to sponsor me provided that I submit another package and do another package review.  See bug #558061 for details.
Comment 1 Mamoru TASAKA 2010-01-27 03:22:03 EST
*** Bug 241099 has been marked as a duplicate of this bug. ***
Comment 2 Susi Lehtola 2010-01-27 04:13:04 EST
Assigning.
Comment 3 Susi Lehtola 2010-01-27 07:16:58 EST
- 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
Comment 4 Susi Lehtola 2010-01-27 07:17:37 EST
Do another unofficial review and apply for membership in the packager group in FAS, and you can consider yourself sponsored.
Comment 5 Eric Smith 2010-01-27 14:27:01 EST
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!
Comment 6 Susi Lehtola 2010-01-27 14:47:11 EST
(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.
Comment 7 Eric Smith 2010-01-27 15:06:28 EST
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
Comment 8 Eric Smith 2010-01-27 15:33:26 EST
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
Comment 9 Susi Lehtola 2010-01-27 18:22:44 EST
(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.
Comment 10 Susi Lehtola 2010-01-28 14:17:13 EST
This package is

APPROVED

You can continue by requesting the CVS branches.
Comment 11 Eric Smith 2010-01-28 16:08:17 EST
New Package CVS Request
=======================
Package Name: levmar
Short Description: Displays monitor test patterns
Owners: brouhaha
Branches: F-12
InitialCC:
Comment 12 Eric Smith 2010-01-28 17:46:14 EST
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:
Comment 13 Kevin Fenzi 2010-01-31 14:06:44 EST
CVS done (by process-cvs-requests.py).
Comment 14 Fedora Update System 2010-01-31 20:51:02 EST
lcdtest-1.18-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/lcdtest-1.18-3.fc12
Comment 15 Fedora Update System 2010-02-01 20:15:29 EST
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.
Comment 16 Eric Smith 2012-03-11 03:23:56 EDT
Package Change Request
======================
Package Name: lcdtest
New Branches: el6
Owners: brouhaha
Comment 17 Jon Ciesla 2012-03-12 08:01:55 EDT
Git done (by process-git-requests).
Comment 18 Fedora Update System 2012-03-12 15:22:48 EDT
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
Comment 19 Fedora Update System 2012-03-28 16:38:51 EDT
lcdtest-1.18-6.el6 has been pushed to the Fedora EPEL 6 stable repository.

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