Bug 241099 - Review Request: lcdtest - utility to display monitor test patterns
Review Request: lcdtest - utility to display monitor test patterns
Status: CLOSED DUPLICATE of bug 559117
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2007-05-24 01:47 EDT by Eric Smith
Modified: 2010-01-27 03:22 EST (History)
1 user (show)

See Also:
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:


Attachments (Terms of Use)

  None (edit)
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
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.  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
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473
- The spec must include a %clean section. Please add
  %clean
  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
mandir=/usr/share/man/man1
scons: Reading SConscript files ...
<type 'exceptions.AttributeError'>: SConsEnvironment instance has no attribute
'MergeFlags':
  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
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest-1.06-3.src.rpm

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:
GOOD
- 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
9da6385dad834ae8073bb0ef7620e32122cec4d9
- 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
NEEDSWORK
- 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.
    http://fedoraproject.org/wiki/Packaging/DistTag

* 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
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest-1.07-2.fc6.src.rpm

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
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/fc6/lcdtest/lcdtest-1.08-1.fc6.src.rpm
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
okay.

Then:
-------------------------------------------------------------
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 :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

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:
https://bugzilla.redhat.com/bugzilla/buglist.cgi?cmdtype=runnamed&namedcmd=mtasaka-review-noone

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 9 Eric Smith 2007-06-06 02:59:28 EDT
I have two other review requests pending:

asl - macro assembler - review nearly completed
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240807

drawtiming - Generates digital signal timing diagrams from a textual description
still needs a reviewer
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=241971

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:
https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=163776
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...)

Well,
* This package is okay
* From a quick glance, the other reviews you requested seem
  almost okay
! NOTE:
  - 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} \
            --mandir=%{_mandir}
--------------------------------------------------
     - Perhaps the above can be replaced with just %configure?
       (please check what %configure does by
        `rpm --eval %configure`)

Okay.
-------------------------------------------------
  This package (lcdtest) is APPROVED by me
-------------------------------------------------

Please follow the procedure written on
http://fedoraproject.org/wiki/PackageMaintainers/Join
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:
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
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: 
   http://fedoraproject.org/PackageReviewStatus/NEW.html
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.

Thanks!
Eric
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
again?
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
Closing.

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 ***

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