Bug 841335

Extra information:
This review request is meant for un-deprecating gnusim8085 as it is deprecated since 2011-07-25.
The original review request was ticket #504077.

* Latest upstream release has been updated to. Good.

$ sha256sum gnusim8085-1.3.7.tar.gz 
e09b56089276eed91fb9df3c1e7e2aa4bf091859cfc62612521b45617167d525  gnusim8085-1.3.7.tar.gz


* rpmlint output is clean.


> License:	GPLv2

"GPLv2+" according to the source file preambles.

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#.22or_later_version.22_licenses

$ find src -name \*.c|wc -l
31
$ grep "any later version" src/*.c|wc -l
31
$ find src -name \*.h|wc -l
31
$ grep "any later version" src/*.h|wc -l
31


> URL:		http://gnusim8085.sourceforge.net

| We have moved to new domain. Please update your bookmark.  You should be
| redirected to our new website in 10 seconds. If not please click here.

-> http://gnusim8085.org/


> BuildRequires:	automake libtool 

Apparently only needed because an autoconf recheck is triggered by the
"sed" based change to configure.in. 

That made me curious. ;-)

There has been a crash failing to find the documentation files: bug 542945

The fix is a brute-force sed substitution without any guard:

> sed -i \
>     "s|share/doc/\${PACKAGE}|share/doc/%{name}-%{version}|" \
>     configure.in
> sed -i "s|/usr/local/doc/GNUSim8085|%{_docdir}/%{name}-%{version}|"
> src/callbacks.c

One ought to be careful with such "sed" substitution, because if they don't match any longer, the command doesn't fail, and you don't notice. Hence it's superior to add a safety-check, such as a separate "grep".

Anyway:

The first sed modifies the line

  packagedocdir=share/doc/${PACKAGE}

in configure.in, but I could not find any other place where this variable would
be used.

The second sed replaces a hardcoded string that is no longer present
in version 1.3.7, but instead a different variable is used:

    g_string_append (tutorial_text, PACKAGE_DOC_DIR);  

and it is defined in src/Makefile.am as:

   $ grep PACKAGE_DOC_DIR src/Makefile.am 
  	 -DPACKAGE_DOC_DIR=\"$(docdir)\"\

So, instead of the two sed substitutions, running configure like this redefined PACKAGE_DOC_DIR:

   %configure --docdir %{_datadir}/doc/%{name}-%{version}

However, reading further the spec file, I found it to be dangerous. During %install, it does

   rm -rf %{buildroot}%{_docdir}

to remove _any_ installed files in /usr/share/doc (whatever may have been installed there intentionally!), then it adds doc files
manually via %doc in the %files section. Why is that dangerous? The default location for %doc files is %{_datadir}/doc/%{name}-%{version}/ which happily conflicts with any files already installed in there. Using %doc to install doc files overrides any files which are in that directory already.

"make install" already installs all documentation except for the
license file "COPYING". So, if that gets installed manually during
%install, everything would be available, and using %doc is not necessary
anymore.


> make %{?_smp_mflags} CFLAGS="%{optflags}

In your koji test build, I noticed the missing verbosity of compiler/linker output. Adding V=1 to the make invocation fixes that, so one can see all the build details.


> mkdir -p %{buildroot}%{_mandir}/man1

Superfluous, as a man page is available in there already. Command can be removed.


> Summary:	A 8085 Simulator

Not a blocker, but a little bit more verbose could be better:
Summary: Graphical simulator for 8085 assembly language


> %{_mandir}/man1/%{name}.1.gz

Better: %{_mandir}/man1/%{name}.1*

The on-the-fly compression to gzip could change eventually or be
disabled/changed in a different build environment.


* Patch for suggested changes will follow.


* What do you think?

Created attachment 598987 [details]
proposed spec file changes

Perhaps instead of

  %{_datadir}/doc/%{name}*

use the more explicit

  %{_datadir}/doc/%{name}-%{version}/

to match the --docdir configure definition.

New Spec URL: http://puiterwijk.fedorapeople.org/packages/gnusim8085/gnusim8085-2.spec
New SRPM URL: http://puiterwijk.fedorapeople.org/packages/gnusim8085/gnusim8085-1.3.7-2.fc17.src.rpm
New Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4251625

I'm sorry I didn't do those simple things right.
I have pulled in your suggestions.

Final look:

$ rpmlint gnusim8085-1.3.7-2.fc17.x86_64.rpm 
gnusim8085.x86_64: W: spurious-executable-perm /usr/share/doc/gnusim8085-1.3.7/COPYING

Ha, that's what happens when one touches packages. :-)


$ rpmls -p gnusim8085-1.3.7-2.fc17.x86_64.rpm 
lists a directory that doesn't contain any files (have missed that before):
/usr/share/pixmaps/gnusim8085


> -URL:           http://gnusim8085.org/
> +URL:           http://www.gnusim8085.org/

Maybe, maybe not. ;) The latter here redirects to http://gnusim8085.org/


> -%{_datadir}/doc/%{name}*
> +%{_datadir}/doc/%{name}-%{version}/*

https://fedoraproject.org/wiki/Packaging:UnownedDirectories

 =>  %{_datadir}/doc/%{name}-%{version}/


As a second thought, a little bit safer than that path for the doc files would be

  %configure --docdir %{_datadir}/doc/%{name}
and
  %{_datadir}/doc/%{name}/
in the files list. That path doesn't conflict with using %doc to add files. However, if any %doc files were to be added manually, you would end up with two doc directories, %{_datadir}/doc/%{name} and %{_datadir}/doc/%{name}-%{version}, so that would be a different pitfall. Adding a comment that using %doc to add files manually bears a risk could be helpful, too.


No minor update needed for this review. These are small issues you can fix more conveniently within Fedora package git.

APPROVED

Thank you very much for your comments, I will fix them and go through everything again before pushing the update.

I think that /usr/share/doc/%{name}-%{version} is safer, because the double dir situation.

New Package SCM Request
=======================
Package Name: gnusim8085
Short Description: Graphical simulator for 8085 assembly language
Owners: puiterwijk
Branches: el5 el6 f16 f17
InitialCC:

Unretired devel, please file an SCM Package Change request for f17 and f16
branches.  EL-5 and EL-6 already exist and are owned by sherry151 and
chitlesh, you can request co-maintainership or start the non-responsive
maintainer process if applicable.

New Package SCM Request
=======================
Package Name: gnusim8085
Short Description: Graphical simulator for 8085 assembly language
Owners: puiterwijk
Branches: f16 f17
InitialCC:

Package Change Request
======================
Package Name: gnusim8085
New Branches: f16 f17
Owners: puiterwijk

Sorry for my mistaken new request.

Re: comment 9 / Jon Ciesla

Not so impressive that chitlesh is a co-maintainer indeed, but has not responded to the FTBFS/bug reports since 2010, which effectively lead to releng retiring this package:

http://bugz.fedoraproject.org/gnusim8085
http://koji.fedoraproject.org/koji/packageinfo?packageID=8600

Git done (by process-git-requests).

Agreed, the non-responsive process is likely warranted.

I have taken sent an email to the previous maintainer (sherry151) requesting him to transfer EPEL-5 and EPEL-6 to me.

But could this package be unblocked for f18, since koji currently will not allow me to build for it (http://koji.fedoraproject.org/koji/taskinfo?taskID=4264898)?

gnusim8085-1.3.7-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/gnusim8085-1.3.7-3.fc17

gnusim8085-1.3.7-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/gnusim8085-1.3.7-3.el6

gnusim8085-1.3.7-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/gnusim8085-1.3.7-3.fc16

Package gnusim8085-1.3.7-3.el6:
* should fix your issue,
* was pushed to the Fedora EPEL 6 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=epel-testing gnusim8085-1.3.7-3.el6'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-EPEL-2012-6460/gnusim8085-1.3.7-3.el6
then log in and leave karma (feedback).

I have submitted updates for f16, f17 and el6.

I have deprecated the el5 branch, since the current versions cannot be built there (it requires libraries that are not possible to bring into el5 because a previous major version is already in the official el5 repo's).

gnusim8085-1.3.7-3.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

gnusim8085-1.3.7-3.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

gnusim8085-1.3.7-3.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.