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.
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.
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.
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.
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...)
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.
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?
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
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}
Corrected the sed, configure, mkdir issues. Where is the patch naming convention documented? If it isn't, what should it look like?
(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.
Fixed. Spec URL: http://zanoni.jcomserv.net/fedora/nagi/nagi.spec SRPM URL: http://zanoni.jcomserv.net/fedora/nagi/nagi-2.06-2.src.rpm
I just compiled this for the dgae review, which led to me noticing the following: MUST FIX: use and honor $RPM_OPT_FLAGS
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.
And. * Please keep timestamps For install, please use "install -p" (check the "Timestamps" section of http://fedoraproject.org/wiki/Packaging/Guidelines )
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
Xavier, if you'd like, Hans has expressed an interest in carrying on with this review.
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.
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!
New Package CVS Request ======================= Package Name: nagi Short Description: An interpreter for AGI games Owners: limb Branches: FC-5 FC-6 F-7 InitialCC: Many thanks, Hans. :) (gets out back-scratcher)
Cvs done.
Built in all branches.
(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