Bug 241099

Summary: Review Request: lcdtest - utility to display monitor test patterns
Product: [Fedora] Fedora Reporter: Eric Smith <spacewar>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED DUPLICATE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: mtasaka
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-07-17 12:00:47 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Eric Smith 2007-05-24 01:47:42 EDT
Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest.spec
SRPM URL: http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest-1.06-1.src.rpm
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.  lctest uses the SDL library.
Comment 1 manuel wolfshant 2007-05-24 04:48:15 EDT
Hi Eric
there are a few problems with the spec, but luckily they are easy to fix
- BuildRoot should have one of the values listed at
- The spec must include a %clean section. Please add
  rm -rf $RPM_BUILD_ROOT
after the %install section
- Using %defattr(-,root,root,-) is saner

Please also note that SDL-devel is already pulled in by SDL_image-devel, so
there is no need to BR it.

And now the sad part: mock build (building for F7/x86_64) fails with the
following error:

+ scons destdir=/var/tmp/lcdtest-1.06-build bindir=/usr/bin
scons: Reading SConscript files ...
<type 'exceptions.AttributeError'>: SConsEnvironment instance has no attribute
  File "SConstruct", line 139:
    exports = {'env' : env})
  File "/usr/lib/scons/SCons/Script/SConscript.py", line 581:
    return apply(method, args, kw)
  File "/usr/lib/scons/SCons/Script/SConscript.py", line 508:
    return apply(_SConscript, [self.fs,] + files, {'exports' : exports})
  File "/usr/lib/scons/SCons/Script/SConscript.py", line 239:
    exec _file_ in stack[-1].globals
  File "src/SConscript", line 67:
    env.MergeFlags (os.environ ['CFLAGS'])
error: Bad exit status from /var/tmp/rpm-tmp.75447 (%build)

RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.75447 (%build)

Comment 2 manuel wolfshant 2007-05-24 04:57:03 EDT
Build fails in FC6, too.

Also, I have just noticed that you do not use %{?_smp_mflags}
Comment 3 Eric Smith 2007-05-25 21:09:57 EDT
Thank you for reviewing my package.  I have updated the spec based on the
review, and made the new spec and SRPM available:

Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest.spec

I build with mock on FC6.  Apparently the reason it failed to build for you is
that it is dependent on SCons >= 0.97, which was just updated in FE6 recently. 
I have added that version dependency to the spec.
Comment 4 manuel wolfshant 2007-05-25 21:49:39 EDT
Excellent, things are almost fine. Some cosmetic fixes are needed to make
rpmlint happy:
Source RPM:
W: lcdtest summary-not-capitalized displays monitor test patterns
->Obvious fix
W: lcdtest rpm-buildroot-usage %build destdir=$RPM_BUILD_ROOT \
->Ignorable, scons needs this
W: lcdtest macro-in-%changelog _smp_mflags
->use %% instead of %
W: lcdtest mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 11)
->please stick with either spaces or tabs, if possible

rpmlint of lcdtest:
W: lcdtest summary-not-capitalized displays monitor test patterns
-> will be fixed in the same time with the src.rpm

Except for the above (and the fact that scons-0.9.7 has not yet been pushed in
devel) everything seems fine, including using the program on FC6/x86_64.

Have you not have been in the position of needing a sponsor I would have
approved the package.

here comes a formal review, to help potential sponsors:
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream, is latest version, SHA1SUM is
- package compiles on FC6 (x86_64) [*]
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- cosmetic changes of spec, see on top of this comment

[*] as of 25.05.2007, needed BR scons>=0.9.7 is not yet available in devel,
probably due to F7 freeze
Comment 5 Mamoru TASAKA 2007-06-05 13:11:15 EDT
Some quick notes:

* Disttag
  - Adding %{?dist} to release number is recommended
    for easier maintainence on different branches.

* Unneeded Requires:
Requires:	SDL, SDL_image
  - This is unnessary (and *should not* needed) as rpmbuild
    checks libraries' dependency and automatically adds them
    to Requires.

* Unneeded macros
%define _prefix    /usr
%define _bindir    %{_prefix}/bin
%define _mandir    %{_prefix}/share/man
  - These macros are already defined and are not needed.

* And read the comment 4 by wolfshant-san.
Comment 6 Eric Smith 2007-06-05 18:07:02 EDT
Thank you for reviewing my package.  I have updated the spec based on the
reviews, and made the new spec and SRPM available:

Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest.spec

Latest changelog entries:
* Tue Jun 05 2007 Eric Smith <eric@brouhaha.com> 1.07-2
- added dist tag
- removed unnecessary macros
- removed unnecessary Requires

* Sat May 26 2007 Eric Smith <eric@brouhaha.com> 1.07-1
- update to 1.07
- capitalize summary
- use spaces consistently in spec rather than tabs
Comment 7 Eric Smith 2007-06-05 19:53:21 EDT
Updated to upstream release 1.08, adds a new diagonal crosshatch test pattern.

Spec URL: http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest.spec
Comment 8 Mamoru TASAKA 2007-06-05 22:50:07 EDT
Okay. Assigning to me.
I have not checked your newest srpm in detail yet, however
as far as I glance at your spec file, this package seems almost

NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package review requests which are waiting for someone to
review can be checked on:

Review guidelines are described mainly on:
Comment 9 Eric Smith 2007-06-06 02:59:28 EDT
I have two other review requests pending:

asl - macro assembler - review nearly completed

drawtiming - Generates digital signal timing diagrams from a textual description
still needs a reviewer

asl and lcdtest were my first two attempts at contributing packages, and as such
my first submissions had problems, but I've learned a lot about the requirements
and think they are now in good shape.  I now routinely use mock and rpmlint.
Comment 10 Eric Smith 2007-06-06 03:02:16 EDT
FYI, the link you supplied for review requests waiting for a reviewer does not
work for me; it returns a message "The search named mtasaka-review-noone does
not exist."

Possibly this URL might be suitable instead:
Comment 11 Mamoru TASAKA 2007-06-06 04:02:04 EDT
(In reply to comment #10)
> FYI, the link you supplied for review requests waiting for a reviewer does not
> work for me; it returns a message "The search named mtasaka-review-noone does
> not exist."

Still does not work for you? Maybe the link can be used
only by me?

> Possibly this URL might be suitable instead:
> https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=163776
FE-NEW bugtracker is no longer used (we are still wanting for
the other methods which can replace FE-NEW bugtracker, and
mtasaka-review-noone mark works for me...)

* This package is okay
* From a quick glance, the other reviews you requested seem
  almost okay
  - For asl:
make install %{_smp_mflags} DESTDIR=$RPM_BUILD_ROOT
    for make install, while I have not checked asl, there
    are many packages where parallel make for "make install" causes
    race condition problem
    (for example, the timing of "making directory" and "putting
     files in the directory")

   - For drawtiming:
./configure CFLAGS="$RPM_OPT_FLAGS" \
            --bindir=%{_bindir} \
            --libdir=%{_libdir} \
     - Perhaps the above can be replaced with just %configure?
       (please check what %configure does by
        `rpm --eval %configure`)

  This package (lcdtest) is APPROVED by me

Please follow the procedure written on
from "Get a Fedora Account"
When you requested someone to sponsor you (in the procedure
above), please make a note on this bug that you did so.

If you want to push this package also on F-7, you
also have to check:
after the URL above.

!! Well, recenctly Fedora package system changed a lot !!
   If you have some questions, please let me know.

Comment 12 Jason Tibbitts 2007-06-06 11:25:01 EDT
If you're looking for open review tickets, this will be faster than making
complicated bugzilla queries: 
Comment 13 Eric Smith 2007-06-06 23:27:20 EDT
I have registered for a Fedora account, signed and submitted the Contributor
license Agreement, verified that the account shows cla_done, and requested to
join groups cvsextras and fedorabugs.

Thank you for sponsoring me.  I will make the changes you have requested to my
submitted packages.
Comment 14 Mamoru TASAKA 2007-06-06 23:30:34 EDT
Okay. Now I am sponsoring you.
Comment 15 Mamoru TASAKA 2007-06-11 10:26:58 EDT
Are you meeting some trouble?
Comment 16 Eric Smith 2007-06-11 13:11:59 EDT
No trouble, I've just been a bit short on spare time this past week.  I should
have the latest round of changes to this and my other two submitted packages
completed and checked into CVS later this week.

Comment 17 Mamoru TASAKA 2007-06-19 11:18:49 EDT
Again, ping?
Comment 18 Mamoru TASAKA 2007-06-26 08:56:23 EDT
ping again?
Comment 19 Mamoru TASAKA 2007-07-03 11:46:09 EDT
ping again?
Comment 20 Mamoru TASAKA 2007-07-10 11:54:11 EDT
Comment 21 Mamoru TASAKA 2007-07-10 11:58:05 EDT
Once removing my sponsorship. This bug will be closed if
no response is received from the reporter within ONE WEEK.
Comment 22 Mamoru TASAKA 2007-07-17 12:00:47 EDT

If someone wants to import this package into Fedora,
please file a new review request, thank you!
Comment 23 Mamoru TASAKA 2010-01-27 03:22:03 EST

*** This bug has been marked as a duplicate of bug 559117 ***