Bug 491886 - Review Request: xa - 6502/65816 cross-assembler
Review Request: xa - 6502/65816 cross-assembler
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Till Maas
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-24 10:26 EDT by Dan Horák
Modified: 2009-04-15 03:47 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-15 03:47:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
opensource: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dan Horák 2009-03-24 10:26:35 EDT
Spec URL: http://fedora.danny.cz/xa.spec
SRPM URL: http://fedora.danny.cz/xa-2.3.5-1.fc11.src.rpm

Description:
xa is a high-speed, two-pass portable cross-assembler. It understands
mnemonics and generates code for NMOS 6502s (such as 6502A, 6504, 6507,
6510, 7501, 8500, 8501, 8502 ...), CMOS 6502s (65C02 and Rockwell R65C02)
and the 65816.
Comment 1 Till Maas 2009-03-31 07:46:27 EDT
- %{_prefix} should be used instead of /usr in %install
- Instead of the chmod a-x for the manpages, you could fix the Makefile to use install -m 0644 for the manpages.
- you should pass "INSTALL=install -p" to make, to preserve the timestamps of the manpages
- can you maybe convince upstream to use PREFIX instead of DESTDIR for the prefix (/usr/local), but add a $(DESTDIR) in install: to follow common practice?
Comment 2 Dan Horák 2009-03-31 10:56:32 EDT
(In reply to comment #1)
> - %{_prefix} should be used instead of /usr in %install
> - you should pass "INSTALL=install -p" to make, to preserve the timestamps of
> the manpages

fixed

> - can you maybe convince upstream to use PREFIX instead of DESTDIR for the
> prefix (/usr/local), but add a $(DESTDIR) in install: to follow common
> practice?  
> - Instead of the chmod a-x for the manpages, you could fix the Makefile to use
> install -m 0644 for the manpages.

will be in a patch that I will submit upstream


Updated Spec URL: http://fedora.danny.cz/xa.spec
Updated SRPM URL: http://fedora.danny.cz/xa-2.3.5-2.fc11.src.rpm

* Tue Mar 31 2009 Dan Horak <dan[at]danny.cz> - 2.3.5-2
- don't use hardcoded /usr
- preserve timestamps when using "install"
- add patch for getline() conflict in recent rawhide
Comment 3 Till Maas 2009-04-13 09:03:50 EDT
[OK] rpmlint output: silent
[OK] Spec in %{name}.spec format

[OK] license allowed: GPLv2
[OK] license matches shortname in License: tag
[OK] license in tarball and included in %doc: COPYING

[OK] package is code or permissive content:

{NOT OK} patches sent to upstream and commented
https://fedoraproject.org/wiki/Packaging/PatchUpstreamStatus
https://fedoraproject.org/wiki/Packaging/Minutes20080506

[OK] Source0 is a working URL
{N/A} Sourceforge URL is Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
<OK> SourceX / PatchY prefixed with %{name}


[OK] Source0 matches Upstream:
edd15aa8674fb86225faf34e56d5cab2  xa-2.3.5.tar.gz

[OK] Package builds on all platforms:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1293948
[N/A] ExcludeArch bugs are filed and commented:
[N/A] BuildRequires are complete (mock builds)
(OK) No file dependencies outside of /etc /bin /sbin /usr/bin /usr/sbin 

[N/A] %find_lang used for locales

[N/A] Every (sub)package containing libraries runs ldconfig
[N/A] .h (header) files are in -devel subpackage
[N/A] .a (static libraries) are in -static subpackage
[N/A] contains .pc (pkgconfig) files and has Requires: pkgconfig
(N/A) .pc files are in -devel subpackage
[N/A] contains .so.X(.Y) files and .so is in -devel
[N/A] -devel subpackage has Requires: %{name} = %{version}-%{release}
[N/A] .la files (libtool) are not included


[N/A] Has GUI and includes %{name}.desktop
[N/A] Follows desktop entry spec
[N/A] Valid .desktop Name
[N/A] Valid .desktop GenericName
[N/A] Valid .desktop Categories

[N/A] Valid .desktop StartupNotify
[N/A] .desktop file installed with desktop-file-install in %install

[OK] Prefix: /usr not used (not relocatable)

[OK] Owns all created directories
[OK] no duplicates in %files
[OK] %defattr(-,root,root,-) is in every %files section
[OK] Does not own files or dirs from other packages
[OK] included filenames are in UTF-8

[OK] %clean is rm -rf %{buildroot} or $RPM_BUILD_ROOT 
[OK] %install starts with rm -rf %{buildroot} or $RPM_BUILD_ROOT 

[OK] Consistent macro usage

[N/A] large documentation is -doc subpackage
[OK] %doc does not affect runtime

{GOOD ENOUGH} no pre-built binaries (.a, .so*, executable)
https://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
The tarball contains these binaries, which seem not to be shipped or used in
the build:
o65 executable, version 0, 6502, 16 bit, byte reloc, alignment 1
loader/ex2
loader/example2

{OK} well known BuildRoot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

{OK} PreReq not used
{OK} RPM_OPT_FLAGS honoured
{OK} Useful debuginfo generated
{OK} no duplication of system libraries
{OK} no rpath
{NOT OK} Timestamps preserved with cp and install
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
The INSTALL="install -p" has to move to the make invocation in %install

{OK} Uses parallel make (%{?_smp_mflags})
{OK} Requires(pre,post) style notation not used
{OK} only writes to tmp /var/tmp $TMPDIR %{_tmppath} %{_builddir} (and %{buildroot} on %install and %clean)
{OK} no Conflicts
{OK} nothing installed in /srv
{OK} Changelog in allowed format
{OK} does not use Scriptlets
<N/A> Architecture independent packages have: BuildArch: noarch
<OK> Sane Provides: and Requires:

{OK} Follows Naming Guidelines


A comment about the getline patch is missing and the 'INSTALL="install -p"' has to be moved. These are only minor issues, please fix them before you import them.

Btw. the name of the binary "xa" may be too generic, please consider to get upstream to rename it to a longer name, e.g. xa65, which is debian's package name.

APPROVED
Comment 4 Dan Horák 2009-04-14 02:31:12 EDT
Thanks for the review, the patches were sent to the upstream maintainer by personal email, comments will be added. I will talk to him about renaming the binary to xa65, it will not only lower the chance of conflict with some other tool, but also bring consistency with the other tools in the package.
Comment 5 Dan Horák 2009-04-14 02:32:18 EDT
New Package CVS Request
=======================
Package Name: xa
Short Description: 6502/65816 cross-assembler
Owners: sharkcz
Branches: F-9 F-10 EL-5
Comment 6 Kevin Fenzi 2009-04-14 12:08:47 EDT
cvs done.
Comment 7 Dan Horák 2009-04-15 03:47:21 EDT
imported and built

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