Bug 239812 - Review Request: nagi - An interpreter for AGI games
Review Request: nagi - An interpreter for AGI games
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
Depends On:
Blocks: 240194 240195
  Show dependency treegraph
 
Reported: 2007-05-11 10:11 EDT by Gwyn Ciesla
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-22 19:05:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
tcallawa: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Gwyn Ciesla 2007-05-11 10:11:52 EDT
Spec URL: http://zanoni.jcomserv.net/fedora/nagi/nagi.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/nagi/nagi-0.1-2002114.src.rpm
Description: NAGI is an interpreter for AGI games, such as the early Space Quest,
Leisure Suit Larry and King's Quest games.

This can run content created with agistudio, which is also up for review.
Comment 1 Andrea Musuruane 2007-05-15 15:15:56 EDT
I just had a glance at the spec file. Here it is what I noticed:

1. Do not use dos2unix. Use sed:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-41d4336fa1af8d74500eb89d3a22410cccc4117d

2. Release tag is not compliant with guidelines:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-e104844825856d7c45f2f0241586985c0495966b

3. the pwd command in %prep was probably there after some testing and should be
removed

Andrea.
Comment 2 Jason Tibbitts 2007-05-15 15:19:10 EDT
I was typing this up but someone else submitted comments first.  Here's what I
had written:

This is assigned but the fedora-review tag is unset.  Is someone reviewing this?

The build fails for me on x86_64:

+ make -f Makefile.linux
gcc -O2  -Wall -Winline -Wshadow -Wstrict-prototypes -DRAD_LINUX -DRAD_WARN -c
base.c -o base.o
In file included from base.c:7:
agi.h:5:21: error: SDL/SDL.h: No such file or directory

(and cascading failures)  Looks like a build depndency on SDL-devel is missing. 
Adding that gets things building.

The Version and Release are a bit off.  First, isn't this version 2.06?  The
upstream web seem to call it that.  If so, release should probably be just "1".
 If you're using some sort of prerelease snapshot then you'd have a release of
"0.1.2002114%{?dist}", but I'm not sure what's up there.

The tarball unpacks into multiple directories, but you have setup cd into one of
them (src) and then escape from that directory to get to the others.  That's
kind of bad form and will leave things laying around in the buildroot after the
package has built.  I suggest using something like
  %setup -qc ${name}-%{version}
to keep everything tidy.
Comment 3 Gwyn Ciesla 2007-05-15 15:57:27 EDT
Fixed 1 and 3 from #1, and SDL BR.  The name of the upstream source uses the
date, not the version.  I will switch it to the version.

Xavier Lamien has assigned it to himself, and possibly just forgot the flag. 
He's also reviewing agistudio.

Comment 4 Andrea Musuruane 2007-05-17 05:15:04 EDT
Some more remarks:

1. License in not GPL as stated in the spec file. It's released under the X11
license.

2. A desktop file is missing. Is there a good reason? nagi must be run only from
command line? (yes, I still hadn't time to run it...)
Comment 5 Gwyn Ciesla 2007-05-17 07:29:19 EDT
From #3: 

Fixed version.

From #4: 

1. Ah, so it is.  Oops. :)

2. Correct, command line only.  In the games I've packaged thus far that are in
review, I've included a desktop file that calls a wrapper script.
Comment 6 Gwyn Ciesla 2007-05-17 07:31:33 EDT
Forgot links to new SRPM, Spec.  But, rpmlint gives:
W: nagi invalid-license X11
The value of the License tag was not recognized.  Known values are:
"Academic Free License", "Adaptive Public License", "Apache License", "Apache
Software License", "Apple Public Source License", "Artistic", "Attribution
Assurance License", "BSD", "Computer Associates Trusted Open Source License",
"CDDL", "CPL", "CUA Office Public License", "EU DataGrid Software License",
"Eclipse Public License", "Educational Community License", "Eiffel Forum
License", "Entessa Public License", "Fair License", "Frameworx License",
"GPL", "LGPL", "Historical Permission Notice and Disclaimer", "IBM Public
License", "Intel Open Source License", "Jabber Open Source License", "Lucent
Public License", "MIT", "CVW License", "Motosoto License", "MPL", "NASA Open
Source Agreement", "Naumen Public License", "Nethack General Public License",
"Nokia Open Source License", "OCLC Research Public License", "Open Group Test
Suite License", "Open Software License", "PHP License", "Python license",
"Python Software Foundation License", "QPL", "RealNetworks Public Source
License", "Reciprocal Public License", "Ricoh Source Code Public License",
"Sleepycat License", "Sun Industry Standards Source License", "Sun Public
License", "Sybase Open Watcom Public License", "University of Illinois/NCSA
Open Source License", "Vovida Software License", "W3C License", "wxWindows
Library License", "X.Net License", "Zope Public License", "zlib/libpng
License", "Creative Commons Attribution", "Creative Commons Attribution-
NoDerivs", "Creative Commons Attribution-NonCommercial-NoDerivs", "Creative
Commons Attribution-NonCommercial", "Creative Commons Attribution-
NonCommercial-ShareAlike", "Creative Commons Attribution-ShareAlike", "Design
Public License", "GFDL", "LaTeX Project Public License", "OpenContent
License", "Open Publication License", "Public Domain", "Ruby License", "SIL
Open Font License", "Charityware", "Commercial", "Distributable", "Freeware",
"Non-distributable", "Proprietary", "Shareware", "Redistributable, no
modification permitted".
If the license is close to an existing one, you can use '<license> style'.

Which one should I use?
Comment 7 Gwyn Ciesla 2007-05-17 07:36:59 EDT
Disregard, MIT works just fine.

Spec URL: http://zanoni.jcomserv.net/fedora/nagi/nagi.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/nagi/nagi-2.06-1.src.rpm
Comment 8 Andrea Musuruane 2007-05-17 08:22:55 EDT
Instead of using %{__sed} you should use sed, for constistency with how you
wrote make, rm, install, etc.

I think you shouldn't use the original debian names for the patches, but rename
them the fedora way (i.e. adding the version). 

You may optionally track the fact that these patches (and Source1) comes from
Debian with some comment lines or a line in the changelog (that maybe useful for
you and others to remember where these originated).

There is no configure, so why adding this comment line?
#./configure --prefix=/usr --mandir=%{_mandir} --libdir=%{_libdir}

In the %install section:

mkdir -p %{buildroot}

is not needed. Just create the directory you need before installing something
there. Therefore, before:

install -D -m755 ../bin/nagi %{buildroot}/%{_bindir}/nagi

add the following line (as you have done in the rest of this section):

mkdir -p %{buildroot}/%{_bindir}
Comment 9 Gwyn Ciesla 2007-05-17 08:39:02 EDT
Corrected the sed, configure, mkdir issues.  Where is the patch naming
convention documented?  If it isn't, what should it look like?
Comment 10 Andrea Musuruane 2007-05-17 09:01:49 EDT
(In reply to comment #9)
> Where is the patch naming
> convention documented?  If it isn't, what should it look like?

I can't search for it right now, but if we trust my memory :-) you should use
the following syntax:

name-version-patch_description.patch

Please note that version should not be written as %{version}, since you could
reuse it in a newer release.
Comment 12 Hans de Goede 2007-05-17 14:09:06 EDT
I just compiled this for the dgae review, which led to me noticing the following:
MUST FIX: use and honor $RPM_OPT_FLAGS
Comment 13 Mamoru TASAKA 2007-05-17 14:17:31 EDT
One more note:

---------------------------------------------------------
%prep
%setup -qn src

%build
.........
sed -i 's/\r//' ../license.txt
sed -i 's/\r//' ../readme.html
.........
%files
%defattr(-,root,root)
%doc ../license.txt ../readme.html
.........
-----------------------------------------------------------

This situation cannot be accepted.
When I rebuild this srpm, license.txt, readme.txt are
left undeleted under BUILD/ directory.
Comment 14 Mamoru TASAKA 2007-05-17 14:29:20 EDT
And.
* Please keep timestamps
  For install, please use "install -p" (check the "Timestamps" section
  of http://fedoraproject.org/wiki/Packaging/Guidelines )
Comment 15 Gwyn Ciesla 2007-05-17 14:39:50 EDT
Fixed OPT_FLAGS (I think), build path issue and timestamps.

Spec URL: http://zanoni.jcomserv.net/fedora/nagi/nagi.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/nagi/nagi-2.06-3.src.rpm
Comment 16 Gwyn Ciesla 2007-05-18 07:45:07 EDT
Xavier, if you'd like, Hans has expressed an interest in carrying on with this
review.
Comment 17 Gwyn Ciesla 2007-05-21 14:23:53 EDT
Hmm. Hans, if you have the time, since he hasn't started anything formal yet,
you have my blessing to take over.

Xavier, feel free to jump back in if you get a chance.  Of if you have anything
you need reviewed yourself.
Comment 18 Hans de Goede 2007-05-22 09:13:35 EDT
MUST:
=====
* rpmlint output is silent
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
* BR: ok
* No locales
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed
* no .desktop file required

Approved!
Comment 19 Gwyn Ciesla 2007-05-22 09:15:40 EDT
New Package CVS Request
=======================
Package Name: nagi
Short Description: An interpreter for AGI games
Owners: limb@jcomserv.net
Branches: FC-5 FC-6 F-7
InitialCC: 

Many thanks, Hans. :) (gets out back-scratcher)
Comment 20 Tom "spot" Callaway 2007-05-22 09:36:11 EDT
Cvs done.
Comment 21 Gwyn Ciesla 2007-05-22 19:05:03 EDT
Built in all branches.
Comment 22 Hans de Goede 2007-05-23 16:47:43 EDT
(In reply to comment #19)
> Many thanks, Hans. :) (gets out back-scratcher)

Well, you can return the favor by reviewing blobAndConquer, see bug 241069
And let me know if you need any help with agistudio

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