[Bug 510743] Review Request: aranym - Atari Running on Any Machine

Fri Aug 21 13:03:49 UTC 2009

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=510743

--- Comment #14 from Andrea Musuruane <musuruan at gmail.com>  2009-08-21 09:03:47 EDT ---
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?

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.