Bug 581334 - Review Request: asl - Macro Assembler AS
Summary: Review Request: asl - Macro Assembler AS
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 240807 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-04-11 20:15 UTC by Eric Smith
Modified: 2011-05-18 21:17 UTC (History)
3 users (show)

Fixed In Version: asl-1.42-0.5.bld77.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-04-25 14:00:07 UTC
mtasaka: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Eric Smith 2010-04-11 20:15:07 UTC
Spec URL: http://fedorapeople.org/~brouhaha/asl/asl.spec
SRPM URL: http://fedorapeople.org/~brouhaha/asl/asl-1.42-0.1.bld75.fc12.src.rpm
Koji scratch build for dist-f13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2108803

Description: 
AS is a portable macro cross-assembler for a variety of microprocessors
and controllers. Although it is mainly targeted at embedded processors
and single-board computers, you also find CPU families that are used in
workstations and PCs in the target list.

Comment 1 Eric Smith 2010-04-11 23:29:30 UTC
*** Bug 240807 has been marked as a duplicate of this bug. ***

Comment 2 Mamoru TASAKA 2010-04-12 18:33:34 UTC
Well, while I don't know I can have time for reviewing this
ticket in a few days, however some notes:

- build fails on ppc64
  http://koji.fedoraproject.org/koji/taskinfo?taskID=2111389

- http://www.pathname.com/fhs/pub/fhs-2.3.html#USRSHAREARCHITECTUREINDEPENDENTDATA
  says that files under /usr/share must be arch-independent, however
  it seems that /usr/share/asl/lib/*.msg binary files differ at least
  between i686 vs x86_64. These files should be moved to %_libdir.

Comment 3 Eric Smith 2010-04-12 23:44:46 UTC
Thank you for looking at it!

The PPC64 fail was because of a patch I'd added to make earlier upstream releases build on PPC64.  The patch is no longer needed, so I've now removed it.

The .msg files *are* arch-independent.  I did a scratch build for F-12 and verified that they came out the same on all four arches.  However, the files do contain a timestamp, and that could cause two different builds to have different (but fully compatible) .msg files.  Because they are arch-independent, I am not moving them to %_libdir.

The updated spec and SRPM are:

Spec URL: http://fedorapeople.org/~brouhaha/asl/asl.spec
SRPM URL: http://fedorapeople.org/~brouhaha/asl/asl-1.42-0.2.bld75.fc12.src.rpm

Comment 4 Mamoru TASAKA 2010-04-20 18:19:38 UTC
Initial comments


* License
  - ./doc_DE/dina4.sty does not allow any modification.
    This is not allowed on Fedora and this file must be removed
    from the source tarball. Please follow:
    https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

* BuildRoot
  - BuildRoot is no longer needed on Fedora (although rpmlint may complain
    without this)
    https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* BR
  - For tex related packages, it is recommended to use virtual Provides
    names for Requires/BuildRequires instead of using rpms' names directly,
    like: "BR: tex(latex)".

! using iconv
  - It is better that the lines using iconv should be moved to %prep,
    as with it rpmbuild -bi --short-circuit won't break.

* %lang
  - For DE related documents, it is preferable that they are marked
    as "%lang(de) %doc".

Comment 5 Eric Smith 2010-04-20 20:45:57 UTC
* License
The file doesn't appear to be used for anything.  I've reported the issue upstream, and for now I have added an rm command in the %prep section.

* BuildRoot
I'd like to keep this for now, as I may also submit this for EPEL5.

* BR
Change made per your suggestion.

! using iconv
I can't do it in %prep, as the file is generated.  I'd rather do it in %build, but unfortunately the upstream build system generates the doc files in "make install", so without doing significant changes to the makefiles, it needs to stay in %install.  If you think this is a serious problem, I'll talk with upstream.

* %lang
Change made per your suggestion.

The updated spec and SRPM are:

Spec URL: http://fedorapeople.org/~brouhaha/asl/asl.spec
SRPM URL: http://fedorapeople.org/~brouhaha/asl/asl-1.42-0.3.bld77.fc12.src.rpm

Comment 6 Mamoru TASAKA 2010-04-21 17:59:22 UTC
Well,

(In reply to comment #5)
> * License
> The file doesn't appear to be used for anything.  I've reported the issue
> upstream, and for now I have added an rm command in the %prep section.

- As I said in the comment 4, ./doc_DE/dina4.sty must be removed
  from the source tarball itself
  ( i.e. you have to once unpack the source tarball, remove this
    file and repackage the tarball for Fedora. It is better that
    repackaged tarball should be renamed like
    "asl-current-XXXX-clean.tar.bz2 )
  Once again please refer to
  https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

? By the way, while srpm release number contains "bld77", the
  source tarball says "bld75" is used.

> * BuildRoot
> I'd like to keep this for now, as I may also submit this for EPEL5.
- Okay.

> ! using iconv
> I can't do it in %prep, as the file is generated.  I'd rather do it in %build,
> but unfortunately the upstream build system generates the doc files in "make
> install", so without doing significant changes to the makefiles, it needs to
> stay in %install.  If you think this is a serious problem, I'll talk with
> upstream.
- Okay.

Comment 7 Eric Smith 2010-04-22 03:52:30 UTC
The policy on prohibited code says that such code has to be removed from the source tarball if there are patent or trademark issues that legally prevent shipping the file even in source form.  That is not the case for this file; the problem is just that we don't like the license.  Including the file in the source tarball but not using it or packaging it in the RPM doesn't have any patent or trademark issue.  My interpretation of the policy was that although we should try to get it resolved upstream, it wasn't required that we modify the tarball.

However, since you feel strongly about it, I have now removed the file following the same procedure specified for patent and trademark issues.

The updated spec and SRPM are:

Spec URL: http://fedorapeople.org/~brouhaha/asl/asl.spec
SRPM URL: http://fedorapeople.org/~brouhaha/asl/asl-1.42-0.4.bld77.fc12.src.rpm

Comment 8 Mamoru TASAKA 2010-04-22 17:41:51 UTC
Okay.

--------------------------------------------------
  This package (asl) is APPROVED by mtasaka
--------------------------------------------------

Comment 9 Eric Smith 2010-04-22 18:13:08 UTC
New Package CVS Request
=======================
Package Name: asl
Short Description: Macro Assembler AS
Owners: brouhaha
Branches: F-12 F-13

Comment 10 Kevin Fenzi 2010-04-24 00:39:18 UTC
CVS done (by process-cvs-requests.py).

Comment 11 Fedora Update System 2010-04-24 09:42:39 UTC
asl-1.42-0.5.bld77.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/asl-1.42-0.5.bld77.fc13

Comment 12 Fedora Update System 2010-04-24 09:43:43 UTC
asl-1.42-0.5.bld77.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/asl-1.42-0.5.bld77.fc12

Comment 13 Fedora Update System 2010-04-25 14:00:01 UTC
asl-1.42-0.5.bld77.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 14 Fedora Update System 2010-04-27 02:13:03 UTC
asl-1.42-0.5.bld77.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Eric Smith 2011-05-15 23:58:17 UTC
Package Change Request
======================
Package Name: asl
New Branches: el6
Owners: brouhaha

Comment 16 Jason Tibbitts 2011-05-18 21:17:57 UTC
Git done (by process-git-requests).


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