Bug 841335 - Review Request: gnusim8085 - Graphical simulator for 8085 assembly language
Review Request: gnusim8085 - Graphical simulator for 8085 assembly language
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-18 13:50 EDT by Patrick Uiterwijk
Modified: 2012-08-05 02:39 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-27 21:16:29 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
proposed spec file changes (1.88 KB, patch)
2012-07-18 15:35 EDT, Michael Schwendt
no flags Details | Diff

  None (edit)
Description Patrick Uiterwijk 2012-07-18 13:50:16 EDT
Spec URL: http://puiterwijk.fedorapeople.org/packages/gnusim8085/gnusim8085-1.spec
SRPM URL: http://puiterwijk.fedorapeople.org/packages/gnusim8085/gnusim8085-1.3.7-1.fc17.src.rpm
Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4248884
Fedora Account System Username: puiterwijk
Description:
GNUSim8085 is a graphical simulator for Intel 8085
microprocessor assembly language. It has some very
nice features including a keypad which can be used
to write assembly language programs with much ease.
It also has stack, memory and port viewers which
can be used for debugging the programs.
Comment 1 Patrick Uiterwijk 2012-07-18 14:09:55 EDT
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.
Comment 2 Michael Schwendt 2012-07-18 15:34:50 EDT
* 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?
Comment 3 Michael Schwendt 2012-07-18 15:35:25 EDT
Created attachment 598987 [details]
proposed spec file changes
Comment 4 Michael Schwendt 2012-07-18 15:36:43 EDT
Perhaps instead of

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

use the more explicit

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

to match the --docdir configure definition.
Comment 5 Patrick Uiterwijk 2012-07-18 16:34:00 EDT
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.
Comment 6 Michael Schwendt 2012-07-19 04:51:36 EDT
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
Comment 7 Patrick Uiterwijk 2012-07-19 05:12:44 EDT
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.
Comment 8 Patrick Uiterwijk 2012-07-19 05:14:23 EDT
New Package SCM Request
=======================
Package Name: gnusim8085
Short Description: Graphical simulator for 8085 assembly language
Owners: puiterwijk
Branches: el5 el6 f16 f17
InitialCC:
Comment 9 Gwyn Ciesla 2012-07-19 08:45:41 EDT
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.
Comment 10 Patrick Uiterwijk 2012-07-19 10:05:44 EDT
New Package SCM Request
=======================
Package Name: gnusim8085
Short Description: Graphical simulator for 8085 assembly language
Owners: puiterwijk
Branches: f16 f17
InitialCC:
Comment 11 Patrick Uiterwijk 2012-07-19 10:06:40 EDT
Package Change Request
======================
Package Name: gnusim8085
New Branches: f16 f17
Owners: puiterwijk

Sorry for my mistaken new request.
Comment 12 Michael Schwendt 2012-07-19 10:31:00 EDT
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
Comment 13 Gwyn Ciesla 2012-07-19 10:56:40 EDT
Git done (by process-git-requests).

Agreed, the non-responsive process is likely warranted.
Comment 14 Patrick Uiterwijk 2012-07-19 11:06:30 EDT
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)?
Comment 15 Fedora Update System 2012-07-19 14:24:49 EDT
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
Comment 16 Fedora Update System 2012-07-19 14:24:59 EDT
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
Comment 17 Fedora Update System 2012-07-19 14:25:09 EDT
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
Comment 18 Fedora Update System 2012-07-19 18:38:50 EDT
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).
Comment 19 Patrick Uiterwijk 2012-07-20 04:40:05 EDT
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).
Comment 20 Fedora Update System 2012-07-27 21:16:29 EDT
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.
Comment 21 Fedora Update System 2012-07-27 21:18:41 EDT
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.
Comment 22 Fedora Update System 2012-08-05 02:39:55 EDT
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.

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