Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 185721 - Review Request: yadex - Doom level/wad editor
Summary: Review Request: yadex - Doom level/wad editor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-03-17 04:39 UTC by Wart
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-04-10 23:53:18 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Patch fixing gcc41 compile (1.26 KB, patch)
2006-03-17 10:57 UTC, Hans de Goede
no flags Details | Diff

Description Wart 2006-03-17 04:39:32 UTC
Spec Name or Url: http://www.kobold.org/~wart/fedora/yadex.spec
SRPM Name or Url: http://www.kobold.org/~wart/fedora/yadex-1.7.0-1.src.rpm
Description: 
A Doom/DoomII/Heretic level editor

Comment 1 Wart 2006-03-17 04:41:46 UTC
I just realized the .desktop file is missing.  I'll add it shortly.

Comment 3 Hans de Goede 2006-03-17 10:43:09 UTC
Question: Why the Requires: freedoom?

Problem on FC-5:
src/wadlist.cc: In member function 'void Wad_list::del()':
src/wadlist.cc:178: error: no match for 'operator=' in
'((Wad_list*)this)->Wad__list::priv->Wad_list_priv::iter = 0'
/usr/lib/gcc/x86_64-redhat-linux/4.1.0/../../../../include/c++/4.1.0/bits/stl_list.h:112:
note: candidates are: std::_List_iterator<boost::shared_ptr<Wad_file> >&)

I'll see if I can fix this, also some 64 bit related warnings to check out
(printf argument size).



Comment 4 Hans de Goede 2006-03-17 10:57:51 UTC
Created attachment 126273 [details]
Patch fixing gcc41 compile

I might find more problems, I justed wanted to share this one asap, so that we
don't do double work.

Comment 5 Wart 2006-03-17 16:18:12 UTC
(In reply to comment #3)
> Question: Why the Requires: freedoom?

Yadex won't run without an iwad, so I modified the default configuration to use
the freedoom iwad.  Without an iwad yadex will complain and quit, and I
guarantee that will result in a bug report the first time someone tries to use
it.  :)  Best to avoid the issue and Requires: an iwad.

Comment 6 Wart 2006-03-17 18:30:58 UTC
(In reply to comment #3)
> Problem on FC-5:
> src/wadlist.cc: In member function 'void Wad_list::del()':
> src/wadlist.cc:178: error: no match for 'operator=' in
> '((Wad_list*)this)->Wad__list::priv->Wad_list_priv::iter = 0'
>
/usr/lib/gcc/x86_64-redhat-linux/4.1.0/../../../../include/c++/4.1.0/bits/stl_list.h:112:
> note: candidates are: std::_List_iterator<boost::shared_ptr<Wad_file> >&)

I've got to be more careful about testing the build on multiple platforms before
submitting these things.

I'll add your patch and do some testing on FC-5 i386 before updating the package.


Comment 7 Hans de Goede 2006-03-18 09:07:24 UTC
I thought it would be a good idea to start the full review while waiting for an
updated version with my patch included. This review is done with my patch
included, otherwise I wouldn't ger very far.


MUST
====
* rpmlint output clean
* Package named correctly
* GPL license OK.
* spec file legible, in Am. English
* Source matches upstream
* Successfully compiles and builds on at least one platform (FC-5 x86_64)
* no locale data, shared libraries, or static libraries
* No excessive Requires: or BR:
* Summary and description ok
* macro use consistent
* package owns the directories that it creates.
* Not relocatable
* %doc does not affect runtime

MUSTFIX
=======
* Does not compile on FC-5 without the attached patch


Comment 8 Wart 2006-03-18 20:13:03 UTC
Patch included.  The sprite viewer doesn't seem to work on FC4-x86_64 or
FC5-i386.  Type "v" at the yadex command prompt to view sprites and you'll get a
new window that hangs.

http://www.kobold.org/~wart/fedora/yadex-1.7.0-3.src.rpm
http://www.kobold.org/~wart/fedora/yadex.spec

Comment 9 Hans de Goede 2006-03-20 20:08:48 UTC
Hmm,

I though I could reproduce this, but after a recompile with some debugging
printf's added its gone, and now even a clean compile doesn't do it anymore,
maybe a compilerbug which got fixed in one of the last updates?


Comment 10 Wart 2006-03-22 05:47:35 UTC
I'll investigate on my end once I've finished reconfiguring my desktop with
FC-5.  Don't expect any updates for about a week.

Comment 11 Hans de Goede 2006-04-08 06:54:14 UTC
Any progress on this?


Comment 12 Wart 2006-04-10 01:25:55 UTC
User error.  I was trying to use the mouse in the "view texture" mode, when it
seems that it only responds to up/down arrow.  The sprite and texture viewers
don't seem to properly refresh the display on Expose events, but it's not a
critical problem.

Right now I'm trying to figure out why it fails to build in mock but not on my
desktop.

Comment 13 Wart 2006-04-10 06:25:33 UTC
It was a local mock configuration error.  Everything should be good now.

Comment 14 Hans de Goede 2006-04-10 09:15:29 UTC
Ok,

I'll try to todo a full review soon then. Unfortunatly I _really_ should start
doing some stuff for my work right now. I know I just submitted a package for
review myself, thats because a Dutch language pack was recently released for an
educational game, so no my little one can play it once packaged. Thus I couldn't
help myself :)

I'll also take a look at the bsd-games setgid stuff when I find the time.


Comment 15 Hans de Goede 2006-04-10 09:55:03 UTC
I just saw I already started a full review, so its easy to finish it here we go
again:

MUST
====
* rpmlint output OK:
E: yadex configure-without-libdir-spec
W: yadex patch-not-applied Patch1:
http://glbsp.sourceforge.net/yadex/Yadex_170_Hexen.diff
Which are both ok, for the patch warning see the comment in the spec file, the
other is no problem since there are no files installed to %{_libdir} and there
is a good reason to not use %configure (see comment in spec)
* Package named correctly
* GPL license OK.
* spec file legible, in Am. English
* Source matches upstream
* Successfully compiles and builds on at least one platform (devel x86_64 & i386)
* no locale data, shared libraries, or static libraries
* No excessive Requires: or BR:
* Summary and description ok
* macro use consistent
* package owns the directories that it creates.
* Not relocatable
* %doc does not affect runtime

Approved!


Comment 16 Wart 2006-04-10 23:53:18 UTC
Imported and built.  Thanks!


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