Bug 291741 - Review Request: gnome-hearts - Game of Hearts implementation for gnome
Review Request: gnome-hearts - Game of Hearts implementation for gnome
Status: CLOSED DUPLICATE of bug 435572
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
: Reopened
: 291721 (view as bug list)
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2007-09-14 19:24 EDT by Richi Plana
Modified: 2008-03-02 11:01 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-17 09:37:54 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Latest spec file (2.55 KB, text/plain)
2007-11-17 19:52 EST, Richi Plana
no flags Details

  None (edit)
Description Richi Plana 2007-09-14 19:24:10 EDT
Spec URL: http://richip.dhs.org/~richip/gnome-hearts.spec
SRPM URL: http://richip.dhs.org/~richip/gnome-hearts-0.2-1.fc7.src.rpm
Description: gnome-hearts is an implementation of the game of Hearts for Gnome. The implementation is still at its infancy but the game is currently playable. This is also an early attempt to be active in the Fedora community.
Comment 1 Jason Tibbitts 2007-09-14 19:34:36 EDT
*** Bug 291721 has been marked as a duplicate of this bug. ***
Comment 2 Jason Tibbitts 2007-09-29 16:20:23 EDT
This fails to build for me:

checking for perl... /usr/bin/perl
checking for XML::Parser... configure: error: XML::Parser perl module is
required for intltool
error: Bad exit status from /var/tmp/rpm-tmp.78686 (%build)

Just a missing build dependency on perl(XML::Parser); I added it and the build
fails later:

checking for python extension module directory...
${exec_prefix}/lib64/python2.5/site-packages
Traceback (most recent call last):
  File "<string>", line 2, in <module>
  File "/usr/lib64/python2.5/distutils/sysconfig.py", line 497, in get_config_vars
    func()
  File "/usr/lib64/python2.5/distutils/sysconfig.py", line 356, in _init_posix
    raise DistutilsPlatformError(my_msg)
distutils.errors.DistutilsPlatformError: invalid Python installation: unable to
open /usr/lib64/python2.5/config/Makefile (No such file or directory)
not found

I added a build dependency on python-devel and the build fails later:

file=`echo hu | sed 's,.*/,,'`.gmo \
          && rm -f $file &&  -o $file hu.po
/bin/sh: line 1: -o: command not found
make[2]: *** [hu.gmo] Error 127

I added yet another build dependency, on gettext this time, and the build succeeds.

If you already have build system access (which it seems you do since this bug
isn't blocking FE-NEEDSPONSOR) then you can do a scratch build in koji with
  koji build --scratch dist-fc8 gnome-hearts-0.2-1.fc7.src.rpm
or (if you have the system for it) you can install mock locally and test doing
clean builds there; see
http://fedoraproject.org/wiki/PackageMaintainers/MockTricks.  Please be kind to
the overworked reviewers and try to make sure your package builds properly in a
clean buildroot before submitting it.

Once building, there are some rpmlint warnings:

gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/players/pauline.py 0644
(and many like it)

It seems to be a python habit to begin any file containing python code with the
usual #! line even though the file is not intended to ever be run from the
command line.  Most of the time I ignore such complaints, but please check to
make sure those files aren't ever to be run directly; if they are, then they
should be executable.

gnome-hearts.x86_64: E: explicit-lib-dependency libglade2
gnome-hearts.x86_64: E: explicit-lib-dependency libgnomeui

You should not include Requires: lines for libraries which rpm will find on its own.

gnome-hearts.x86_64: E: no-changelogname-tag

Your %changelog is empty; you must include proper content there; see the
Changelogs section of http://fedoraproject.org/wiki/Packaging/Guidelines

gnome-hearts.x86_64: W: invalid-license GPL

You must specify the proper GPL version; see http://fedoraproject.org/wiki/Licensing

gnome-hearts.x86_64: E: invalid-desktopfile
/usr/share/applications/gnome-hearts.desktop

You're missing the required call to desktop-file-install in %install; it would
then fail your build with an error:
/tmp/gnome-hearts.desktop: error: file contains multiple assignments of key "Name"

This is simply because the file has "Name=No name" and "Name=Hearts".  Please
see the "Desktop files" section of
http://fedoraproject.org/wiki/Packaging/Guidelines for more information on the
guidelines surrounding desktop files.

Please do try running rpmlint on your packages (both the binary and source RPMs)
before submission; it often times spews complaints which aren't relevant and
it's OK to ask about those complaints in your review, but it gives you a chance
to clean up the easy fixes.
Comment 3 Mamoru TASAKA 2007-10-28 08:34:48 EDT
Richi, ping?
Comment 4 Richi Plana 2007-10-28 10:41:08 EDT
Still interested. Just reading the guidelines now and will be setting up the
proper environment. Real life getting in the way of some of it. Was there an
immediate request for it?
Comment 5 Mamoru TASAKA 2007-10-28 11:33:15 EDT
Usually if no response from the reporter is received for more than
one month after potential reviewers wrote some comments (in this
review report the last comment is Jason's), the bug will be closed
within one week.
Comment 6 Sander Marechal 2007-11-09 10:18:41 EST
Hello,

I am Sander Marechal, the upstream developer of gnome-hearts

> Once building, there are some rpmlint warnings:
> 
> gnome-hearts.x86_64: E: non-executable-script
> /usr/lib/python2.5/site-packages/gnome-hearts/players/pauline.py 0644
> (and many like it)
> 
> It seems to be a python habit to begin any file containing python code with the
> usual #! line even though the file is not intended to ever be run from the
> command line.  Most of the time I ignore such complaints, but please check to
> make sure those files aren't ever to be run directly; if they are, then they
> should be executable.

Things like this should probably be reported upstream instead of ignored. I
happened to chance upon this bug when searching Google for references to my
game. Else I wouldn't have known about this.

Anyway, it's been fixed in upstream subversion and will be part of the upcoming
2.1 release: http://svn.jejik.com/viewvc.cgi/gnome-hearts?view=rev&revision=163

> it would then fail your build with an error:
> /tmp/gnome-hearts.desktop: error: file contains multiple assignments of key "Name"
> 
> This is simply because the file has "Name=No name" and "Name=Hearts".

This has been fixed as well:
http://svn.jejik.com/viewvc.cgi/gnome-hearts?view=rev&revision=159

Kind regards,

-- 
Sander Marechal
Comment 7 Jason Tibbitts 2007-11-09 11:07:15 EST
Sander,

Yes, you're correct that these issues should be reported upstream.  That is the
duty of the maintainer of the package (Richi, in this case).  Unfortunately us
package reviewers have far too many packages to review to be able to contact
upstream ourselves.

I hope Richi will update this package to the 2.1 release soon after it's ready.
Comment 8 Richi Plana 2007-11-09 11:32:21 EST
Not to worry. I would have updated upstream had I known it was actually an issue
with them as opposed to just being a packaging issue (I'm new to packaging).

In the case of gnome-hearts, is there a preferred method by which you (Sander)
would wish to be informed? Currently, I don't think you have a bugzilla system
set up. Would you like to use Fedora's packaging efforts for gnome-hearts in the
meantime?
Comment 9 Sander Marechal 2007-11-09 18:26:02 EST
My bugzilla is listed on the project website and in the documentation. It's
http://bugzilla.jejik.com (product "hearts").

I assume that it's possible to (semi) automatically forward bugs from Red Hat to
my bugzilla? If so, that would be my preferred method. Currently the same thing
is done by Debian's BTS and Ubuntu's Launchpad.
Comment 10 Mamoru TASAKA 2007-11-17 08:27:53 EST
Please someone update the status of this bug.
Comment 11 Richi Plana 2007-11-17 11:36:12 EST
Will do a release this weekend. I was planning on waiting for the 0.2.1 version,
but it's not yet released (or it's not on the website, yet).
Comment 12 Michel Alexandre Salim 2007-11-17 12:24:49 EST
In the case that SVN contains specific patches that you want to use, you can
just release based on the last stable release, get the required patches from
SVN, and apply them during the build setup phase with %patch
Comment 13 Sander Marechal 2007-11-17 12:43:38 EST
> I was planning on waiting for the 0.2.1 version

It should be out in a week or two. The code is basically done. All that's left
is fixing up the NEWS, README, changelog, and importing the last translations
from launchpad. That kind of stuff. The real reason I haven't released it yet is
because of a hardware upgrade on my server. I'm waiting for the last few
component to install a new SATA hardware RAID. That server runs my svn,
bugzilla, etcetera.

I don't want to do a release and then pull the plug on my bugzilla and
subversion the next day for the upgrade. I'm playing it safe. Upgrade first,
release shortly after.

If you want, you can use the code from branches/0.2.x HEAD for the redhat
package. It's just a bunch of bugfixes on top of the 0.2 release. No new features.
Comment 14 Richi Plana 2007-11-17 13:36:26 EST
I'm actually wondering if a versioning based on SVN might be a good thing to do
right now since, I'm assuming, these are all development releases anyway. On my
own machine, I'd probably have rpmbuild pull source directly from SVN. But as
this is going on koji, I might have to do the %patch route.

Thanks for the input.
Comment 15 Jason Tibbitts 2007-11-17 14:04:02 EST
You can do an "svn export" and then tar up the result and include that as the
Source tarball in your package.  All you need to do is to make sure it's
reproducible (so you should check out a specific version) and you need to
document the procedure in comments in the specfile.  See
http://fedoraproject.org/wiki/Packaging/SourceURL
Comment 16 Richi Plana 2007-11-17 15:53:33 EST
By the way, this is actually my first attempt at packaging and I thought I was
following the procedure for first-time packagers. I don't think I have "build
system access" as Jason Tibbits mentions above. I do not know why this bug isn't
blocking FE-NEEDSPONSOR. Perhaps I missed something in the process, but I tried
to follow it as best as I could and is why I'm in my current status.
Comment 17 Richi Plana 2007-11-17 16:29:53 EST
Almost have things ready. Just the matter of certain py modules marked
non-executable yet starting with "#!". Apparently they're not meant to be
executed. What should the line be replaced with? There's nothing specific
mentioned in http://fedoraproject.org/wiki/Packaging/Python
Comment 18 Sander Marechal 2007-11-17 18:15:51 EST
> Just the matter of certain py modules marked
> non-executable yet starting with "#!".

Where? I thought I fixed them all. Are you pulling from
svn.jejik.com/gnome-hearts/branches/0.2.x or from trunk? The fixes are in
branches/0.2.x
Comment 19 Richi Plana 2007-11-17 19:12:09 EST
Remaining rpmlint errors and warnings:

rpmlint ../RPMS/x86_64/gnome-hearts-0.2-1.svn163.fc7.x86_64.rpm 
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/players/pauline.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/players/luis.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/hearts.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/players/mike.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/definitions.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/player.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/players/john.py 0644
gnome-hearts.x86_64: E: non-executable-script
/usr/lib/python2.5/site-packages/gnome-hearts/stock_ai.py 0644
gnome-hearts.x86_64: W: unstripped-binary-or-object /usr/bin/gnome-hearts
gnome-hearts.x86_64: E: invalid-desktopfile
/usr/share/applications/gnome-hearts.desktop

I'm not sure why I'm getting a Warning for an unstripped-binary
(/usr/bin/gnome-hearts).

As for the invalid .desktop:

$ desktop-file-validate /usr/share/applications/gnome-hearts.desktop 
/usr/share/applications/gnome-hearts.desktop: error: file contains multiple
assignments of key "Name"

(Name=No Name) which I thought was fixed but the svn version I have seems to
still have it.
Comment 20 Richi Plana 2007-11-17 19:13:32 EST
Whoops .. my mistake. I was pulling from trunk. Will retry.
Comment 21 Richi Plana 2007-11-17 19:52:40 EST
Created attachment 262611 [details]
Latest spec file
Comment 22 Richi Plana 2007-11-17 19:53:33 EST
Here's my latest attempt:

SRPM URL: http://richip.dhs.org/~richip/gnome-hearts-0.2-1.svn163.fc7.src.rpm

I'm attaching the spec file. I have yet to set up a mock system, and I still
don't have access or have the knowledge to access the build system. If I could
get a pointer to directions, I would greatly appreciate it.
Comment 23 Jason Tibbitts 2007-11-17 20:20:58 EST
If you're still working on this package, perhaps this ticket shouldn't be closed
NOTABUG.
Comment 24 Richi Plana 2007-11-17 21:32:46 EST
Closing was accidental.
Comment 25 Mamoru TASAKA 2007-11-22 06:35:47 EST
Rebuild fails at least on i386.

http://koji.fedoraproject.org/koji/taskinfo?taskID=254252
Comment 26 Sander Marechal 2007-11-22 10:48:17 EST
I see that the build server tries to run the ./bootstrap script. That script is
a tool for humans, not machines. Among other things it tries to download
config.sub and config.guess from the internet, and create an acinclude file. It
would be a bit silly that you cannot build a package just because the GNU CVS
happens to be down for maintenance for example :-)

If you want to create a package from my subversion sources instead of a release
package, then I suggest you checkout a copy from my subversion server and create
a release package yourself and use that instead.

Checkout branches/0.2.x from my subversion and run "bootstrap", "configure" and
"make dist". Use the resulting .tar.gz as the basis for the Red Hat package.
Comment 27 Richi Plana 2007-11-22 10:50:59 EST
Thanks. I've had a chance to do a mock and have fixed the error you got while
building on koji. Please give the following a whack:

http://richip.dhs.org/~richip/gnome-hearts-0.2-2.svn163.fc8.src.rpm
Comment 28 Richi Plana 2007-11-22 10:53:31 EST
Hi, Sander.

(In reply to comment #26)
> If you want to create a package from my subversion sources instead of a release
> package, then I suggest you checkout a copy from my subversion server and create
> a release package yourself and use that instead.
> 
> Checkout branches/0.2.x from my subversion and run "bootstrap", "configure" and
> "make dist". Use the resulting .tar.gz as the basis for the Red Hat package.

Thanks. I've checked out and package a copy of SVN as well as included cached
copies of the required downloads. Fortunately, the bootstrap script author
decided to write her/his script in such a way that it checks for the existence
and won't download the scripts if they exists. Please give the new version a
try. If it works, I'll also submit the package GPass for approval.
Comment 29 Christopher Aillon 2007-11-22 19:30:35 EST
Richi, hey cool.  Glad you're taking this on.  Some drive by comments if you
don't mind:

First, I'll answer your question in the specfile.  If you have something listed
as Source1, you reference it in your spec with %{SOURCE1}.  Same with Source2 =>
%{SOURCE2}.  That said, please do not ship config.guess or config.sub at all. 
All most SRPMs should really have is the specfile and the tarball.  (Plus any
source patches, but in this case you probably won't have those if you are
pulling from svn or shipping exact releases).

Back to the tarball... If you are building a tarball yourself, please make sure
that it ships with a pre-generated configure.  The builder should not have to
_ever_ run bootstrap or autoconf.  This means, run it yourself and make sure
configure is included.  ;-)

Some other comments:

your %build section is too busy (because of the bootstrap stuff... your ideal
%build will just be calling configure and then make).

Since you are installing a .desktop file, you must register it with the system
using desktop-file-install

Since you are installing langpacks, you need to make proper use of the
%find_lang macro (and properly in the header of %files)

Thanks for your work here.  Look forward to seeing it in the repo!
Comment 30 Richi Plana 2007-11-22 20:41:21 EST
Hey, Chris. Thanks for the feedback!

(In reply to comment #29)
> %{SOURCE2}.  That said, please do not ship config.guess or config.sub at all. 
> All most SRPMs should really have is the specfile and the tarball.  (Plus any
> source patches, but in this case you probably won't have those if you are
> pulling from svn or shipping exact releases).
> 
> Back to the tarball... If you are building a tarball yourself, please make sure
> that it ships with a pre-generated configure.  The builder should not have to
> _ever_ run bootstrap or autoconf.  This means, run it yourself and make sure
> configure is included.  ;-)
> 
> Some other comments:
> 
> your %build section is too busy (because of the bootstrap stuff... your ideal
> %build will just be calling configure and then make).

If you haven't guessed by now, the reason "configure" doesn't exist is that I'm
pulling the source from SVN. I thought of generating configure myself before
archiving the source tree, but I just assumed that going through the whole
autoconf thing was the better route as I was afraid there might be something on
my platform that was specific to it and that autoconf on the build machine would
do differently. That's 'cause I don't really know how autoconf works, ;).

> Since you are installing a .desktop file, you must register it with the system
> using desktop-file-install

I was wondering at what else I needed to do during install and uninstallation.
Unfortunately, the Packaging Guidline doesn't go too deeply into platform-,
subsystem-, or DE-specific processes. Things like "desktop-file-install" or
"export GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL=1", I only glean from examples.
(Or did I miss it in the Guidelines?)

> Since you are installing langpacks, you need to make proper use of the
> %find_lang macro (and properly in the header of %files)

Right. Sorry about that. I actually came across that guideline but only applied
it to the second package I was preparing (gpass). I'll work on it. Is that not a
check that can be added to rpmlint?

> Thanks for your work here.  Look forward to seeing it in the repo!

Me, too! It's one of my favorite card games and I get to play it on my machine.
'Twould be nice to have the multiplayer, networked version soon.

I'll make another release tonight.
Comment 31 Christopher Aillon 2007-11-23 08:07:57 EST
(In reply to comment #30)
> If you haven't guessed by now, the reason "configure" doesn't exist is that I'm
> pulling the source from SVN. I thought of generating configure myself before
> archiving the source tree, but I just assumed that going through the whole
> autoconf thing was the better route as I was afraid there might be something on
> my platform that was specific to it and that autoconf on the build machine would
> do differently. That's 'cause I don't really know how autoconf works, ;).

To give you some idea as to why... the reason configure scripts typically aren't
shipped in the repository are because they are generated from other files.  So
you'd have to make a change to the configure.ac or .in file, and then have to
re-generate configure, and then commit that to the repository as well.  Some
projects that want to ship configure will have a bot watch for changes to the
relevant files and regenerate and re-commit, such as Mozilla.  But generally
it's much easier to just deal with it when creating a tarball.  This means if
you're creating the tarball, the responsibility is sort of yours.  ;-)


> > Since you are installing a .desktop file, you must register it with the system
> > using desktop-file-install
> 
> I was wondering at what else I needed to do during install and uninstallation.
> Unfortunately, the Packaging Guidline doesn't go too deeply into platform-,
> subsystem-, or DE-specific processes. Things like "desktop-file-install" or
> "export GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL=1", I only glean from examples.
> (Or did I miss it in the Guidelines?)

http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=%28Guidelines%29#head-254ddf07aae20a23ced8cecc219d8f73926e9755

I guess I don't see any gconf stuff in the guidelines, but it doesn't look like
gnome-hearts ships any gconf schemas yet (unless I'm missing them) so you don't
have to worry about it.

> 
> > Since you are installing langpacks, you need to make proper use of the
> > %find_lang macro (and properly in the header of %files)
> 
> Right. Sorry about that. I actually came across that guideline but only applied
> it to the second package I was preparing (gpass). I'll work on it. Is that not a
> check that can be added to rpmlint?

Might be!  Consider filing a bug against rpmlint?
Comment 32 Richi Plana 2007-11-23 09:38:30 EST
I've made another release at:

http://richip.dhs.org/~richip/gnome-hearts-0.2-2.svn163.fc8.src.rpm

with the changes put forward by Christopher Aillon.

One thing to note, though, is that the original source package had a Make target
of install-desktopDATA which uses "install" to copy the .desktop file. Should
this be removed from upstream? From the packages? Or be left alone? Would it
make sense for upstream to use desktop-file-install? Or should that target just
be removed?
Comment 33 Mamoru TASAKA 2007-11-23 10:50:48 EST
Some comments:
* I guess copying them from config.{guess,sub} from %_datadir/libtool 
  is sufficient for "sh bootstrap" (and this work for me).
  (BR: libtool is needed)

* Don't use "make install-strip". Stripping binaries by yourself 
  disables debuginfo rpm creation.

* Please use %find_lang, desktop-file-install (BR: desktop-file-utils
  needed)

(In reply to comment #32)
> One thing to note, though, is that the original source package had a Make target
> of install-desktopDATA which uses "install" to copy the .desktop file. Should
> this be removed from upstream? From the packages? Or be left alone? Would it
> make sense for upstream to use desktop-file-install? Or should that target just
> be removed?
I don't think there is any reason upstream should change the behavior about
installing desktop file.

Comment 34 Mamoru TASAKA 2007-11-23 10:51:51 EST
(In reply to comment #33)
> Some comments:
> * I guess copying them from config.{guess,sub} from %_datadir/libtool 
>   is sufficient for "sh bootstrap" (and this work for me).
>   (BR: libtool is needed)

I meant "I guess copying config.{guess,sub} from %_datadir/libtool
is sufficient".
Comment 35 Richi Plana 2007-11-23 11:49:39 EST
(In reply to comment #33)
> Some comments:
> * I guess copying them from config.{guess,sub} from %_datadir/libtool 
>   is sufficient for "sh bootstrap" (and this work for me).
>   (BR: libtool is needed)

Personally, I prefer my srpms to have options to compile from autoconf. At least
it gets to check that the packages are still compatible with the newest version
not to mention some packages may need it across different versions of Fedora.
But since it's currently discouraged, I've generated configure myself and used
that as source.

> * Don't use "make install-strip". Stripping binaries by yourself 
>   disables debuginfo rpm creation.

How do I get rid of the rpmlint warning that keeps complaining of unstripped
binaries?

> * Please use %find_lang, desktop-file-install (BR: desktop-file-utils
>   needed)

The latest version of the src.rpm linked above (gnome-hearts ... oops .. I
linked to the wrong src.rpm ... it should be:

http://richip.dhs.org/~richip/gnome-hearts-0.2-3.svn163.fc8.src.rpm
                                               ^
). That one has the fixes. I'll generate a new one to fix the "make
install-strip" as soon as I get a reply as to what to do about the rpmlint
warnings (disregard?)
> 
> (In reply to comment #32)
> > One thing to note, though, is that the original source package had a Make target
> > of install-desktopDATA which uses "install" to copy the .desktop file. Should
> > this be removed from upstream? From the packages? Or be left alone? Would it
> > make sense for upstream to use desktop-file-install? Or should that target just
> > be removed?
> I don't think there is any reason upstream should change the behavior about
> installing desktop file.
> 
> 

Comment 36 Mamoru TASAKA 2007-11-23 12:10:39 EST
(In reply to comment #35)
> > * Don't use "make install-strip". Stripping binaries by yourself 
> >   disables debuginfo rpm creation.
> 
> How do I get rid of the rpmlint warning that keeps complaining of unstripped
> binaries?

I guess you did some wrong setup for rpm.
/usr/lib/rpm/find-debuginfo.sh and /usr/lib/rpm/redhat/brp-strip-*
should strip binaries correctly.
Comment 37 Richi Plana 2007-11-23 12:56:14 EST
Hopefully this resolves all the remaining issues:

http://richip.dhs.org/~richip/gnome-hearts-0.2-4.svn163.fc8.src.rpm

The reason that I was getting rpmlint errors was that I wasn't generating
debuginfo. I should have checked the mock build for rpmlint.
Comment 38 Mamoru TASAKA 2007-11-24 05:43:31 EST
Well, perhaps almost okay. However perhaps it will be next monday
when I check this review request next time.

Some quick comments:
- License seems GPLv2+
- %python_sitelib macro seems unused so please remove this macro
  and unused %{python_sitelib}/gnome-hearts comment.
- scrollkeeper-update must be called
  Please refer to
  http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
- Please add
--------------------------------------------
  DESTDIR="install -p"
--------------------------------------------
  to "make install" to keep timestamps on installed files.
  While there are some cases in which this method doesn't work,
  this method usually works for recent Makefiles.
- Including license text as %doc is needed when possible.
  Please add "COPYING{-DOCS}" to %doc.

And:
-------------------------------------------------------------
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 collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


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 39 Richi Plana 2007-11-25 17:25:13 EST
(In reply to comment #38)
> - License seems GPLv2+

I'm no license expert so I'll take your word for it. How can one tell?

> - Please add
> --------------------------------------------
>   DESTDIR="install -p"
> --------------------------------------------
>   to "make install" to keep timestamps on installed files.
>   While there are some cases in which this method doesn't work,
>   this method usually works for recent Makefiles.

I'm not sure how this works. Do you mean to replace 'DESTDIR=$RPM_BUILD_ROOT'
with 'DESTDIR="install -p"'? Or did you mean something like 'INSTALL="install
-p"'? When I tried that, it seemed to work and it seemed like make picked up
that environment variable.

I'll assume that for now and released:

http://richip.dhs.org/~richip/gnome-hearts-0.2-5.svn163.fc8.src.rpm

Comment 40 Christopher Aillon 2007-11-26 06:54:42 EST
Btw, get rid of the %if use_network stuff.  Everywhere.  Just looking back at
the history of things, your spec from comment 0 looked pretty much like it's
supposed to look.  Just change the version/revision, and add the
desktop-file-install stuff, find_lang stuff, add the extra BuildRequires, and
install -p stuff.  Then make sure it passes rpmlint and builds in mock, and it'd
be pretty much ready to ship.  The original spec looked so much cleaner.  :-)
Comment 41 Mamoru TASAKA 2007-11-26 06:59:47 EST
(In reply to comment #39)
> (In reply to comment #38)
> > - License seems GPLv2+
> 
> I'm no license expert so I'll take your word for it. How can one tell?
  Now we are checking all source files in the tarball to find out
  the license.

> 
> > - Please add
> > --------------------------------------------
> >   DESTDIR="install -p"
> > --------------------------------------------
> >   to "make install" to keep timestamps on installed files.
> >   While there are some cases in which this method doesn't work,
> >   this method usually works for recent Makefiles.
> 
> Or did you mean something like 'INSTALL="install -p"'? 
  My bad.. Actually I meant INSTALL="install -p"...

  Now I will wait for your pre-review or another review request
  (as I wrote in comment 38).
Comment 42 Richi Plana 2007-11-26 11:28:12 EST
(In reply to comment #40)
> Btw, get rid of the %if use_network stuff.  Everywhere.  Just looking back at
> the history of things, your spec from comment 0 looked pretty much like it's
> supposed to look.  Just change the version/revision, and add the
> desktop-file-install stuff, find_lang stuff, add the extra BuildRequires, and
> install -p stuff.  Then make sure it passes rpmlint and builds in mock, and it'd
> be pretty much ready to ship.  The original spec looked so much cleaner.  :-)

Mind if I just clean it up a bit? I'd rather have use_network there while I
track the alpha releases and I'd like to avoid having two spec files for now
(I've already had several cases of editing one file when I meant to edit the other).
Comment 43 Christopher Aillon 2007-11-29 06:55:05 EST
(In reply to comment #42)

Actually, yeah I do kind of mind.  It's something I'd block on before letting it
into Fedora.  That should never be run from rpmbuild, really.  If you want to be
able to generate sources quickly during tracking svn, I'd suggest a shell script.

./gen_gnome_hearts_tarball.sh 0.2 163

#!/bin/sh
rpm_ver_tag=$1
svn_rev=$2

ghdir=gnome-hearts-${rpm_ver_tag}

rm -rf ${ghdir}
svn export -r ${svn_rev} svn://svn.jejik.com/gnome-hearts/branches/0.2.x ${ghdir}
cd ${ghdir}
  sh ./bootstrap
  strapped=$?
  if [ $strapped != 0 ]; then
    exit $strapped
  fi
cd -
tar -czvf gnome-hearts-${rpm_ver_tag}.svn${svn_rev}.tar.gz ${ghdir}
Comment 44 Mamoru TASAKA 2007-12-03 07:44:48 EST
ping? I am waiting your pre-review.
Comment 45 Richi Plana 2007-12-03 10:30:23 EST
(In reply to comment #44)
> ping? I am waiting your pre-review.

Will work on one or two tonight. I was going for submitting a new package, but
I'm having a hard time taming autotools for gpass on F8.
Comment 46 Mamoru TASAKA 2007-12-16 00:09:31 EST
ping again?
Comment 47 Mamoru TASAKA 2007-12-26 13:54:19 EST
ping again?
Comment 48 Mamoru TASAKA 2008-01-09 10:20:53 EST
I will close this bug if no response from the reporter is
received within ONE WEEK.
Comment 49 Mamoru TASAKA 2008-01-17 09:37:54 EST
CLOSING.

If someone wants to import this package into Fedora, please file a new
review request and mark this bug a duplicate of the new bug.

Thank you!
Comment 50 Christopher Aillon 2008-03-01 18:50:55 EST
FYI, I filed bug 435572, based on Richi's spec.
Comment 51 Mamoru TASAKA 2008-03-02 11:01:49 EST

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

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