Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: <main package name here> - <short summary here>
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Summary: Review Request: <main package name here> - <short summary here> Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: eric@brouhaha.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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.fc1... 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Eric Smith eric@brouhaha.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: <main |Review Request: lcdtest - |package name here> - <short |display test pattern |summary here> |generator
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #1 from Mamoru Tasaka mtasaka@ioa.s.u-tokyo.ac.jp 2010-01-27 03:22:03 EST --- *** Bug 241099 has been marked as a duplicate of this bug. ***
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |jussi.lehtola@iki.fi AssignedTo|nobody@fedoraproject.org |jussi.lehtola@iki.fi Flag| |fedora-review?
--- Comment #2 from Jussi Lehtola jussi.lehtola@iki.fi 2010-01-27 04:13:04 EST --- Assigning.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #3 from Jussi Lehtola jussi.lehtola@iki.fi 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #4 from Jussi Lehtola jussi.lehtola@iki.fi 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #5 from Eric Smith eric@brouhaha.com 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!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #6 from Jussi Lehtola jussi.lehtola@iki.fi 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #7 from Eric Smith eric@brouhaha.com 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.fc1... Koji scratch build for f12-dist: http://koji.fedoraproject.org/koji/taskinfo?taskID=1948905
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #8 from Eric Smith eric@brouhaha.com 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.fc1... Koji scratch build for f12-dist: http://koji.fedoraproject.org/koji/taskinfo?taskID=1948930
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #9 from Jussi Lehtola jussi.lehtola@iki.fi 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #10 from Jussi Lehtola jussi.lehtola@iki.fi 2010-01-28 14:17:13 EST --- This package is
APPROVED
You can continue by requesting the CVS branches.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #11 from Eric Smith eric@brouhaha.com 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:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Eric Smith eric@brouhaha.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #12 from Eric Smith eric@brouhaha.com 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:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #13 from Kevin Fenzi kevin@tummy.com 2010-01-31 14:06:44 EST --- CVS done (by process-cvs-requests.py).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #14 from Fedora Update System updates@fedoraproject.org 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #15 from Fedora Update System updates@fedoraproject.org 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Fixed In Version| |1.18-3.fc12 Resolution| |ERRATA
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Eric Smith eric@brouhaha.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs+ |fedora-cvs?
--- Comment #16 from Eric Smith eric@brouhaha.com 2012-03-11 03:23:56 EDT --- Package Change Request ====================== Package Name: lcdtest New Branches: el6 Owners: brouhaha
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #17 from Jon Ciesla limburgher@gmail.com 2012-03-12 08:01:55 EDT --- Git done (by process-git-requests).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
--- Comment #18 from Fedora Update System updates@fedoraproject.org 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=559117
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|1.18-3.fc12 |lcdtest-1.18-6.el6
--- Comment #19 from Fedora Update System updates@fedoraproject.org 2012-03-28 16:38:51 EDT --- lcdtest-1.18-6.el6 has been pushed to the Fedora EPEL 6 stable repository.
package-review@lists.fedoraproject.org