Bug 510743

Don't you plan to make a separate (noarch) package for AFROS? AFAIK the release schedules of Aranym and AFROS are independent. I have a preliminary version at http://fedora.danny.cz/

Good idea. I have updated the spec file.

I'm not qualified to do a review but here are some informal comments:

1: The link to the source RPM is broken so I can only comment on the spec file.

2: Don't you think the package name should be "ARAnyM" rather than "aranym"? The Naming Guidelines say: «If they refer to their application as "ORBit", you should use "ORBit" as the package name, and not "orbit".» (https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Case_Sensitivity) The website of the project seems to use "ARAnyM" consistently.

3: There is (according to the spec) a patch called "aranym-0.9.8beta.diff". Such a name makes me wonder "differences between aranym-0.9.8beta and what?" It looks like it might contain the changes from (for example) 0.9.0 to 0.9.8beta, but I suppose that's not what it is because the source is already version 0.9.8beta. I think a patch name should say something about what the patch does. Alternatively, a comment might help.

4: I think you forgot to run RPMlint:

aranym.spec: E: no-cleaning-of-buildroot %install
This is caused by a typo. Please fix.

aranym.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 52)
Please replace the tabs with spaces. They serve no purpose and can make the file look ugly in other people's editors.

(RPMlint also says:
aranym.spec:62: W: configure-without-libdir-spec
aranym.spec:69: W: configure-without-libdir-spec
aranym.spec:73: W: configure-without-libdir-spec
I can't really comment on that as I can't see the configure script.)

(In reply to comment #3)
> 2: Don't you think the package name should be "ARAnyM" rather than "aranym"?
> The Naming Guidelines say: «If they refer to their application as "ORBit", you
> should use "ORBit" as the package name, and not "orbit".»
> (https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Case_Sensitivity)
> The website of the project seems to use "ARAnyM" consistently.

IMHO it is fine the actual package name.

"When naming a package, the name should match the upstream tarball or project name from which this software came. In some cases, this naming choice may be more complicated. If this package has been packaged by other distributions/packagers in the past, then you should try to match their name for consistency. In any case, try to use your best judgement, and other developers will help in the final decision." 
(https://fedoraproject.org/wiki/Packaging:NamingGuidelines#General_Naming)
 
Please note that upstream called its RPMs aranym-0.x.y-z.i386.rpm.

> (RPMlint also says:
> aranym.spec:62: W: configure-without-libdir-spec
> aranym.spec:69: W: configure-without-libdir-spec
> aranym.spec:73: W: configure-without-libdir-spec
> I can't really comment on that as I can't see the configure script.)  

This is usually given because you invoke configure directly and not the %configure macro. Is there any reason in not using the macro?

HTH,

Andrea.

* I think that you should install the icons under
%{_datadir}/icons/hicolor/32x32/apps 
%{_datadir}/icons/hicolor/48x48/apps 

and use the following snipplets:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

and require:
Requires:       hicolor-icon-theme

* The release number is not right according to the guidelines:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Version

* I couldn't see a MimeType key in the desktop entry. Therefore there should be no need for the following scriptlet:

%post
update-mime-database %{_datadir}/mime &> /dev/null || :

%postun
update-mime-database %{_datadir}/mime &> /dev/null || :

* This is not required
Requires:       desktop-file-utils

* gcc-c++ is not BR. It is always included.

* SDL-devel is not BR. It is already pulled in by SDL_image-devel. There could probabily be more devel pacakges like this. I didn't check them all.

%configure doesn't work because it is being built in a subdir.  There should be a general solution for that, since building outside of the srcdir is often preferred.

Spec URL: http://home.mnet-online.de/whitebox/aranym.spec
SRPM URL: http://home.mnet-online.de/whitebox/aranym-0.9.8-0.1.fc11.beta.src.rpm

I think I have addressed all issues.

(In reply to comment #7)
> Spec URL: http://home.mnet-online.de/whitebox/aranym.spec
> SRPM URL:
> http://home.mnet-online.de/whitebox/aranym-0.9.8-0.1.fc11.beta.src.rpm
> 
> I think I have addressed all issues.  

At first sight, the release tag problem still persist. It should be 0.1.beta%{?dist}

Your changelog should always reflect your changes. Now, after your changes, you only have:

* Wed Aug 5 2009 Andreas Schwab <schwab> - 0.9.8-0.1.fc11.beta
- Initial version.

But this is not the initial version. You still made some changes.

I'll try to have a deeper look later.

Spec URL: http://home.mnet-online.de/whitebox/aranym.spec
SRPM URL: http://home.mnet-online.de/whitebox/aranym-0.9.8-0.1.beta.fc11.src.rpm

I didn't bother to update the changelog because that package does not really "exist" yet.  But I can add some verbiage if required.

rpmlint output:
aranym.src:56: W: configure-without-libdir-spec
aranym.x86_64: W: file-not-utf8 /usr/share/doc/aranym/AUTHORS
aranym.x86_64: W: file-not-utf8 /usr/share/doc/aranym/changelog
aranym-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/aranym-0.9.8beta/src/natfeat/nfbootstrap.cpp
aranym-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/aranym-0.9.8beta/src/natfeat/nfbootstrap.h
3 packages and 0 specfiles checked; 0 errors, 5 warnings.

(In reply to comment #10)
Andreas, these need to be addressed:

> rpmlint output:
> aranym.src:56: W: configure-without-libdir-spec

I'll try to investigate the build system later.

> aranym.x86_64: W: file-not-utf8 /usr/share/doc/aranym/AUTHORS
> aranym.x86_64: W: file-not-utf8 /usr/share/doc/aranym/changelog

https://fedoraproject.org/wiki/Common_Rpmlint_issues#file-not-utf8

> aranym-debuginfo.x86_64: W: spurious-executable-perm
> /usr/src/debug/aranym-0.9.8beta/src/natfeat/nfbootstrap.cpp
> aranym-debuginfo.x86_64: W: spurious-executable-perm
> /usr/src/debug/aranym-0.9.8beta/src/natfeat/nfbootstrap.h

chmod -x src/natfeat/nfbootstrap.cpp src/natfeat/nfbootstrap.h

> > aranym.src:56: W: configure-without-libdir-spec

That's a bug/shortcoming in rpmlint.

53:%define subdir_configure \
54:cat >configure <<'EOF'\
55:#!/bin/sh\
56:exec ../configure "$@"\
57:EOF\
58:chmod +x configure\
59:%configure

Spec URL: http://home.mnet-online.de/whitebox/aranym.spec
SRPM URL: http://home.mnet-online.de/whitebox/aranym-0.9.8-0.2.beta.fc11.src.rpm

aranym.src:63: W: configure-without-libdir-spec
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

This is not a formal review. I miss the basic knowledge of how aranym works. Nonetheless, I hope this will help in getting aranym approved.

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistent macro usage.
OK - Meets Packaging Guidelines.
GPLv2+ - License
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
SEE BELOW - Spec is legible.
OK - Sources match upstream md5sum:
8a9fd404c8d72b1a2a23ea866d322132  aranym-0.9.8beta.tar.gz
8a9fd404c8d72b1a2a23ea866d322132  aranym-0.9.8beta.tar.gz.orig
NA - Package needs ExcludeArch
SEE BELOW - BuildRequires correct
NA - Spec handles locales/find_lang
NA - Package is relocatable and has a reason to be.
SEE BELOW - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
OK - Package is code or permissible content.
NA - Doc subpackage needed/used.
- Packages %doc files don't affect runtime.

NA - Headers/static libs in -devel subpackage.
NA - Spec has needed ldconfig in post and postun
NA - .pc files in -devel subpackage/requires pkgconfig
NA - .so files in -devel subpackage.
NA - -devel package Requires: %{name} = %{version}-%{release}
NA - .la files are removed.

SEE BELOW - Package is a GUI app and has a .desktop file

OK - Package compiles and builds on at least one arch.
Fedora 11/i386
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
SEE BELOW - No rpmlint output.
OK - final provides and requires are sane

SHOULD Items:

SEE BELOW - Should build in mock.
SEE BELOW - Should build on all supported archs
- Should function as described.
OK - Should have sane scriptlets.
NA - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. The layout of the SPEC file, although formally correct, isn't very readable. Please move the BuildRequires and Requires at the bottom of Header Information, just after the BuildRoot.

Even better, please do follow the layout of the SPEC template Fedora uses:
https://fedoraproject.org/wiki/Packaging:Guidelines#Writing_a_package_from_scratch

It is very handy to have one BuildRequires/Requires entry per line. Thus when diff'ing cvs revisions, it is very clear what have been added/removed.

2. Rename Patch to Patch0 

3. Patch0 should patch configure too and not only configure.ac thus making autoreconf no longer needed. Autoreconf shouldn't be used in the %prep or %build sections of a package's spec file:
https://fedoraproject.org/wiki/PackagingDrafts/AutoConf

4. You require autoreconf but you don't have autoconf among the BR. This is wrong and it implies the package cannot be built using mock. But please solve this issue getting rid of autoreconf as stated above.

5. Submit Patch1 upstream:
https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

6. Remove 
#Suggests:       afros

Make a file named README.Fedora and place it among the docs:
https://fedoraproject.org/wiki/Packaging:Guidelines#summary

State there that aranym needs afros to be useful and where to find it. Debian does something like this, if you need inspiration for the text:
http://patch-tracking.debian.net/patch/debianonly/view/aranym/0.9.8beta-1

7. Ignoring verifying the file mode because aratapif needs to be setuid root manually by the user is wrong:
%verify(not mode) %attr(755,root,root) %{_bindir}/aratapif

Install it with setuid root _and_ check that SELinux won't complain about it. If it complains at runtime, try to solve the issue. Do not delegate this to the final user.

8. The %{_docdir}/aranym directory is not the one where Fedora users expect to find the documentation in. It should be %{_docdir}/%{name}-%{version}.

You include INSTALL and you must not. I have some doubts about changelog too: it is too detailed to be useful to the final user, it points to the source code a lot and there is already a more readable NEWS file.
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

You could remove %{_docdir}/aranym in %build and specify the doc files in %files.

9. Try to build aranym as the following sniplet does. It is much more readable and you shouldn't get rpmlint warnings.

# JIT only works on i586
%ifarch %ix86
%configure --enable-jit-compiler
make %{?_smp_mflags}
mv aranym aranym-jit
make clean
%endif

%configure --enable-addressing=direct --enable-fullmmu --enable-lilo 
make %{?_smp_mflags}
mv aranym aranym-mmu
make clean

%configure --enable-addressing=direct
make %{?_smp_mflags}

BTW, why do you disable the (default) native debugger? If it is really needed, consider adding a comment.

10. Preserve time stamps with "touch -r" in the loop you are doing to fix char encoding.
https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

11. Please add a semicolon at the end of the "Categories" key, in the desktop file. Report this problem upstream.

desktop-file-install --dir=/home/fedora/rpmbuild/BUILDROOT/aranym-0.9.8-0.2.beta.fc11.i386/usr/share/applications contrib/aranym.desktop
contrib/aranym.desktop: key "Categories" is a list and does not have a semicolon as trailing character, fixing

12. These icons will not be used by desktop file because their names are not correct:
/usr/share/icons/hicolor/32x32/apps/icon-32.png
/usr/share/icons/hicolor/48x48/apps/icon-48.png

Files must be named "aranym.png" according to the desktop file.

13. You must also install desktop files for aranym-mmu and aranym-jit
https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

14. Do not use %define:
%define pre beta

Use %global:
https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

15. As I said I don't really know how aranym works and I saw that there are some executables and documentation in usr/share/aranym/. Why?

Spec URL: http://home.mnet-online.de/whitebox/aranym.spec
SRPM URL:
http://home.mnet-online.de/whitebox/aranym-0.9.8-0.3.beta.fc11.src.rpm

Thank you for the detailed review.  A few notes on your notes:

5. That's really a bug in zlib.

6. I've used requires(hint) instead.

9. That makes debugging of build problems harder, and can make the debuginfo package incomplete.  rpmlint should really be fixed, and configuring in a subdir should be supported by default.  IMHO srcdir != builddir should be the norm.

The included spec file uses --disable-nat-debug as well.

10. That wouldn't change anything since the installation won't preserve timestamps anyway.  And I've already fixed it upstream.

11. Fixed upstream.

15. The things in /usr/share/aranym are part of the emulation.

(In reply to comment #15)
> 5. That's really a bug in zlib.

If so, report a bug against zlib.

> 6. I've used requires(hint) instead.

AFAIK "requires(hint)" is exactly the same of "requires" in Fedora, bringing in something that doesn't exist in Fedora. Therefore it will be impossible to install aranym. 

You'll be able to include this after an afros package will be submitted for review and approved in Fedora.

Until then, I do think that a README is the right solution for this problem.

> 9. That makes debugging of build problems harder, and can make the debuginfo
> package incomplete.  rpmlint should really be fixed, and configuring in a
> subdir should be supported by default.  IMHO srcdir != builddir should be the
> norm.

I disagree, but as I'm not the official reviewer, I let this decision to him.

> The included spec file uses --disable-nat-debug as well.

This is not a good reason. For example other distributions don't disable it. Moreover, upstream spec files are often badly written, seldom updated and never follow Fedora guidelines. So, there should be some reason to disable it different from someone is doing this but I don't know why.

> 10. That wouldn't change anything since the installation won't preserve
> timestamps anyway.  And I've already fixed it upstream.

What have you fixed upstream: the preservation of timestamps or the encoding of the files? 

Anyway, as it is now, this loop will lose the timestamps of the original files:

for f in AUTHORS ChangeLog; do
  iconv -f ISO-8859-1 -t UTF-8 $f > $f.new
  mv $f.new $f
done

> 15. The things in /usr/share/aranym are part of the emulation.  

Even the README files? This is not the right place for documentation and files are not tagged like docs.

Actually, Requries(hint): specifies a dependency for the nonexistent "hint" scriptlet, and simply exploits the fact that rpm does not currently mark this as a syntax error.  It should not be used in any Fedora package.

I've seen Requires(hint) documented somewhere, but I can't remember where right now.

It is used by lyx, for example.

So that package needs to be fixed.  The fact that some other package does something it shouldn't be doing doesn't mean that it's OK for this package to do it.

For a discussion about Requires(hint) please read:
http://www.redhat.com/archives/fedora-packaging/2008-June/msg00071.html

But the real problem is that you are trying to require (even if optionally) something that doesn't exist! Afros is not in Fedora. This is wrong.

Note that at this time, both the review spec and srpm are awol:
Object not found!

The requested URL was not found on this server. If you entered the URL manually please check your spelling and try again.

If you think this is a server error, please contact the webmaster.
Error 404
home.mnet-online.de
Mon Oct 5 15:09:46 2009
Apache
====
Andreas, is that an accident, changed provider, or what ?
If you don't see yourself as continuing to squash your package into shape, then you might consider closing the Review Request, otherwise we'll be needing updated URLs.

Still no spec or package after another five weeks; it's time this was closed.