Bug 193479 - Review Request: xwrits
Review Request: xwrits
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-29 10:20 EDT by Jeff Layton
Modified: 2014-06-18 03:35 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-28 08:01:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Jeff Layton 2006-05-29 10:20:28 EDT
Spec URL: http://people.redhat.com/jlayton/xwrits/xwrits.spec
SRPM URL: http://people.redhat.com/jlayton/xwrits/xwrits-2.22-1.src.rpm
Description: 
Xwrits reminds you to take wrist breaks, which
should help you prevent or manage a repetitive
stress injury. It pops up an X window when you
should rest; you click on that window, then take a
break.

Xwrits's graphics are brightly colored pictures of
a wrist and the attached hand. The wrist clenches
and stretches ``as if in pain'' when you should
rest, slumps relaxed during the break, and points
forward valiantly when the break is over. It is
trapped behind bars while the keyboard is locked.
Other gestures are included.

Extensive command line options let you control how
often xwrits appears. It can escalate its behavior
over time -- by putting up more flashing windows
or actually locking you out of the keyboard, for
example -- which makes it harder to cheat.

(This is my first package and I'm looking for a sponsor)
Comment 1 Jonathan Underwood 2006-05-29 14:01:05 EDT
Hello Jeffrey,

Not yet a proper review, but a couple of points:

1) Use the dist tag:
Release: 1%{?dist}

2) Why not use %setup -q, as is standard

3) Use either $RPM_BUILD_ROOT or %{buildroot}, and do it consistently, don't mix
and match - choose one and stick to it.

4) There should be a rm -rf $RPM_BUILD_ROOT at the beginning of %install

5) Consider using make %{?_smp_mflags} in build.

Fix up these points and I'll run it through a mock build.
Comment 2 Jeff Layton 2006-05-29 16:41:41 EDT
Thanks for having a look. I've made the changes you recommended most were pretty
straightforward.

The only one that wasn't was adding %{?_smp_mflags} to make. I don't have an SMP
box handy to test how well that works for this package. I'll plan to test it on
one tomorrow though

Let me know if you see any other problems.
Comment 3 Jeff Layton 2006-05-30 10:20:38 EDT
Tested building the SRPM with %{?_smp_mflags} and it seems to work correctly. I
also made a slight change to the .desktop file to make it so that it runs xwrits
with some more options.

Comment 4 Kevin Fenzi 2006-06-06 21:43:46 EDT
Greetings. 
Is the current spec/src.rpm the ones at the beginning of this bug? 
It's a good idea to increment release numbers and add changelog entries even
while the package is under review, so we can check changes against previous
revsions... 

can you post an updated version with the changes you made in comment #2 and #3? 
I can look at reviewing this in the next few days... 
Comment 5 Kevin Fenzi 2006-06-16 17:26:17 EDT
Re-adding comment in that was lost from bugzilla crash: 
------- Additional Comments From jlayton@redhat.com  2006-06-11 09:41 EST -------
Sorry for the delay. The current spec/src.rpm were the new ones, but you're
correct that I should have bumped the release number.

I've done that and uploaded new versions:

http://people.redhat.com/jlayton/xwrits/xwrits.spec
http://people.redhat.com/jlayton/xwrits/xwrits-2.22-2.src.rpm
Comment 6 Kevin Fenzi 2006-06-16 18:26:25 EDT
I would be happy to review this and potentially sponsor you... 
you might want to take a look at: 
http://fedoraproject.org/wiki/Extras/HowToGetSponsored

Look for a review coming sometime this weekend... 
Comment 7 Kevin Fenzi 2006-06-19 22:46:14 EDT
Sorry I didn't get to this review this weekend... 

Here's a review.

OK - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
d49fbcc02dc09d1586281f3b294245da  xwrits-2.22.tar.gz
d49fbcc02dc09d1586281f3b294245da  xwrits-2.22.tar.gz.1
OK - Package compiles and builds on at least one arch.
n/a - Package needs ExcludeArch
OK - BuildRequires correct
n/a - Spec handles locales/find_lang
n/a - Spec has needed ldconfig in post and postun
n/a - Package is relocatable and has a reason to be.
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
See below - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
n/a - -doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.
n/a - Headers/static libs in -devel subpackage.
n/a - .pc files in -devel subpackage.
n/a - .so files in -devel subpackage.
n/a - -devel package Requires: %{name} = %{version}-%{release}
n/a - .la files are removed.
OK - Package is a GUI app and has a .desktop file
OK - Package doesn't own any directories other packages own.
See Below - No rpmlint output.

Issues:

1. Some rpmlint output:

W: xwrits no-version-in-last-changelog

Suggest: add "2.22-2" to the end of the
* Sun May 28 2006 Jeff Layton <jlayton@redhat.com>
line.

E: xwrits non-standard-dir-perm /usr/share/doc/xwrits-2.22 0644

Suggest: don't set a default perm in files...
%defattr(0644,root,root)

should be just

%defattr(-,root,root)

W: xwrits non-standard-dir-in-usr man

Suggest: Don't specify full path for man page:

/usr/man/man1/xwrits.1*

should be:

%{_mandir}/man1/*

W: xwrits empty-%post

Suggest: remove empty %post section?

E: xwrits configure-without-libdir-spec

Suggest: add '--libdir=${_libdir}' to the configure step.
Comment 8 Jeff Layton 2006-06-23 08:23:32 EDT
Thanks for the review, Kevin. I've made the suggested changes plus a few others
to more "macroize" the specfile. The new package and spec file are up on my
people page:

Spec URL: http://people.redhat.com/jlayton/xwrits/xwrits.spec
SRPM URL: http://people.redhat.com/jlayton/xwrits/xwrits-2.22-3.src.rpm

Let me know what you think.
Comment 9 Kevin Fenzi 2006-06-26 21:21:06 EDT
All the items in comment #7 are addressed. 
This package looks good to me now, so it's APPROVED. 

I am willing to go ahead and sponsor you. You don't have any comments on other 
review requests, but you have a good history in bugzilla contibuting. ;) 

You can continue the process by going to: 
http://fedoraproject.org/wiki/Extras/Contributors
on the "Get a Fedora Account" step. 

Please let me know via email or irc (I'm nirik on #fedora-extras) if you have 
any questions...
Comment 10 Jeff Layton 2006-06-28 08:01:57 EDT
Package is in CVS for devel and FC-5. I've done a build and it was successful.
Closing BZ. Thanks for the help, Kevin!

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