Bug 173380 - Review Request: nethack-vultures
Review Request: nethack-vultures
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
David Lawrence
http://www.darkarts.co.za/projects/vu...
:
Depends On:
Blocks: FE-ACCEPT 173385
  Show dependency treegraph
 
Reported: 2005-11-16 14:43 EST by Karen Pease
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-23 15:06:44 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)
Changes made after my last version of this package (3.65 KB, text/plain)
2005-11-17 16:01 EST, Ville Skyttä
no flags Details

  None (edit)
Description Karen Pease 2005-11-16 14:43:53 EST
Spec Name or Url: http://www.daughtersoftiresias.org/progs/vultures/nethack-vultures.spec
SRPM Name or Url: http://www.daughtersoftiresias.org/progs/vultures/nethack-vultures-1.10.1-0.1.src.rpm
Description: Vulture's Eye is a mouse-driven interface for NetHack that enhances the visuals, audio and accessibility of the game, yet retains all the
original gameplay and game features.  Vulture's Eye is based on Falcon's Eye, but is greatly extended.  Also included is Vulture's Claw, which is based on the Slash'Em core.
Comment 1 Ville Skyttä 2005-11-17 16:01:39 EST
Created attachment 121203 [details]
Changes made after my last version of this package

Having done most of the packaging work, I'm a bit biased reviewer, but will do 

early next week until someone picks it up until that. 
 
However, a few notes about the differences happened after the package this was 

based on (http://cachalot.mine.nu/4/SRPMS/nethack-vultures-1.10.1-0.1.src.rpm,
diff attached).

Please bump the release tag _always_ when you make changes to any package.  
Can be fixed by bumping to 1%{?dist} later after approved. 
 
Why was the log2stderr patch removed?  Without it, the program will try to 
trash the harddisk with logfiles in the working dirs.  I consider that a 
blocker. 

The "get rid of ncurses-devel build dep" was handled by just removing the build
dep.  The last time I tested, that broke the build.  Does it build cleanly for
you if you don't have ncurses-devel installed?

The "move manual to doc dir" TODO was taken care of by adding _another_ copy of
the manual (plus a lot of completely unneeded files) to the doc dir and
removing the symlinking between Eye and Claw's manual, resulting in not moving
the one manual to a proper place, but in a total of _three_ manuals included,
two of which in places where they're not needed, and another in doc dir but
with lots of unneeded cruft included.  Removing the manuals included in
%{_prefix}/games/vultures*/manual and changing %files to do
vultures/win/jtp/gamedata/manual/ would be the right thing to do (unless the
manuals are needed in the game dir(s)).

Finally (not a change from my previous version, but anyway), SDL_mixer has been
changed to pull in timidity++ so the dependency can be removed from here.
Comment 2 Karen Pease 2005-11-17 16:53:03 EST
I assumed that the log2stderr patch was the one that you sent us to include in 
the source tree, which we did (we've applied all the patches that you've sent 
us).  Although, now thinking back about it, that will only be an appropriate 
solution for future builds, not this one, even if that was the case.  Putting 
it back in. 
 
Sorry about the revision number - you're right, I did forget to bump it!  For 
some reason, I was thinking that your revision was .0.  Upped. 
 
I didn't try building with ncurses-devel missing, but I grepped through the 
source, and nothing in there that should be building that looked like it would 
need ncurses-devel - only obsolete window managers.  I suppose uninstalling 
ncurses-devel wouldn't be too much of a pain for me, so I'll try it out.  Ah, 
I see what's happening - the version that we have a release for here *is* 
building the obsolete window managers.  It's probably just easiest to put 
ncurses-devel in, rather than make another release.  Done.  When we make a 
release that doesn't build the other window managers, I'll take it back out. 
 
Remove the timidity++ dependency...  Done. 
 
The manual thing confuses me, because that this is not the code that I 
entered.  I think when I had backed out some broken code, I must have backed 
out my fixes (I had two rm statements earlier for the manuals before the "Save 
some space" comment, as well as removing them from the file list - that's why 
manual was removed from the for loop - and had narrower doc entries).  
Restoring the deleted code. 
 
Thanks, Ville.  Sorry for the errors. 
 
http://www.daughtersoftiresias.org/progs/vultures/nethack-vultures.spec 
http://www.daughtersoftiresias.org/progs/vultures/nethack-vultures-1.10.1-0.2.src.rpm 
 
 
Comment 3 Ville Skyttä 2005-11-21 14:07:35 EST
Ok, thanks, the 0.2 release looks better.  
  
Yep, you got the log2stderr stuff right, the intention of the stderr patch  
I sent "upstream" to you was that in the future, the log2stderr patch in this  
package could be later reduced to just #undef'ing (or removing the definition  
of) JTP_LOG_FILE.  
  
However, release 0.2 of the package adds the log2stderr patch back to the 
source RPM, but forgets to apply it.  Needs %patch3 in %prep.  
  
Regarding the ncurses-devel dependency, I vaguely remember that it had  
something to do with the -config patch enabling "#define LINUX", but I may  
be wrong.  Anyway, I agree that just having the build dependency in is ok for  
now. 
  
In case you're not aware of them, for finding missing build dependencies  
you'll probably find the mock package and/or the fedora-rmdevelrpms script in  
fedora-rpmdevtools useful.  
  
So, this is approved, as long as the missing %patch3 line is put in before the  
first build (you may do that after importing to CVS, while bumping the release  
tag to 1%{?dist} ;)).  
Comment 4 Karen Pease 2005-11-21 17:42:38 EST
Done  :) 
 
When I get a chance (an hour or two from now), I'll continue on with the 
process. 
 
Comment 5 Ville Skyttä 2005-11-22 04:30:32 EST
It seems you've added the 14MB binary tarball three times into CVS.  It 
belongs in the lookaside cache where it already is.  Please remove it from all 
branches in CVS. 
 
You know you can use eg. 1%{?dist} as the release in the specfile for all  
branches if you don't want to manually tinker with them between branches and 
ensure upgradeability, right?   Also, non-pre-release release tags usually 
don't start with "0."; my specfile had that but it was meant only for the 
duration of the package review. 
Comment 6 Karen Pease 2005-11-22 14:37:58 EST
Ok, thanks - I didn't know those things.  Where is this lookaside cache?  And 
how can I access it to put new release tarballs in there? 
 
Comment 7 Ville Skyttä 2005-11-22 15:06:20 EST
cvs-import.sh has already taken care of this for you when you imported the 
package.  For more info, see http://fedoraproject.org/wiki/Extras/UsingCvsFaq 
Comment 8 Karen Pease 2005-11-22 15:51:08 EST
Thanks!  If it's ok with you, I'm going to go ahead and mark this release 
pending. 
 
What about regular Falcon's Eye?  Should we revisit the issue of 
keeping/removing it in a month or so (after people have had a chance to use 
Vulture's Eye from RPM and we know that there are no significant issues that we 
didn't catch), or should we address keeping/removing it now? 
 
Comment 9 Ville Skyttä 2005-11-22 16:34:30 EST
If by "release pending" you mean that it has been built in the buildsys and is  
waiting to be pushed, sure.  At this point new packages are usually just 
closed as NEXTRELEASE though.  
http://fedoraproject.org/wiki/Extras/NewPackageProcess 
  
I think we should proceed with removing Falcon's Eye already now; it has been  
obsoleted by the vanilla nethack package for a while already and I'm not aware  
of any plans to revive it now that Vulture's * is in.  I'll take care of it.  
Comment 10 Karen Pease 2005-11-23 15:06:44 EST
Thanks! 

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