Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: bugzilla-sink@leemhuis.info ReportedBy: packages@amiga-hardware.com QAContact: fedora-package-review@redhat.com
Spec URL: http://www.dribble.org.uk/AGReader.spec SRPM URL: http://www.dribble.org.uk/AGReader-1.1-6.fc4.src.rpm Description:
Hi, I would appreciate a review of this package to get it into Fedora Extras. This is also my first package, so I would appreciate a sponsor. Thanks.
A viewer for the UNIX console which can read and display AmigaGuide files. It supports all of the v39 AmigaGuide specification possible and supports a large subset of the v40 specifications.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From packages@amiga-hardware.com 2006-05-01 17:43 EST ------- *** Bug 190394 has been marked as a duplicate of this bug. ***
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From packages@amiga-hardware.com 2006-05-01 17:45 EST ------- *** Bug 190393 has been marked as a duplicate of this bug. ***
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |j.w.r.degoede@hhs.nl OtherBugsDependingO|163776 |163778 nThis| |
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-26 10:11 EST ------- Hi,
I just came accross your DevIL rpms and thus your rpm site and I thought, now why doesn't he become an FE contributer, then I thought: maybe he already is a contributer but DevIL hasn't been reviewed yet. Searching for a DevIL review request didn't help. Searching for bugs with your email in it however has helped, so here we are.
I can sponsor people and you seem to need a sponsor so that makes us a good match I guess :)
First of all please carefully / thoroughly read: http://fedoraproject.org/wiki/Extras/Contributors
If you would have you would have known that you should have made this bug also block the FE-NEEDSPONSOR bug. Although many people seem to miss this and you do seem to have read the other guidelines (judging from your DevIL spec), if not please also thoroughly read: http://fedoraproject.org/wiki/Packaging/Guidelines http://fedoraproject.org/wiki/Packaging/NamingGuidelines
I'll start todo a review for this package as described on: http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
Could you please submit DevIL and cegui for review too, then once all 3 are approved you can create an account, I'll sponsor you and then you can import all 3 of them.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-26 10:38 EST ------- Hmm,
Before I do a full Review of this one here is a list of things to fix on forehand:
1) You write: # It is perhaps preferable to patch the source for compilation with gcc 4+ than # force compilation with gcc 3.2 as is done here Yes IMHO that is a must fix item, please write a patch so that it will compile with gcc-4.1 (and submit it upstream also please) and use that instead of this hack. If you find it hard to write this patch / encounter problems feel free to ask for help. I'm more the willing to write this patch for you when asked.
2) Inconsistent macro usage: "%{__rm} -rf %{buildroot}" in %cleanb instead use just "rm -rf %{buildroot}" as you do in %install. If you want to use %__cmd, you should use it everywhere, so also for make, install etc. But please don't :)
Also I have doubts about the capitalization of the name, I thought it was prefered to use just lowercase, I've started a discussion about this on f-e-l.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-26 10:38 EST ------- One last question, could you post an URL to some amigaguides, so I can test this program?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From packages@amiga-hardware.com 2006-05-26 15:15 EST ------- Thanks very much for considering sponsoring me. Here's a link to an updated SRPM I did a couple of weeks back which fixed the inconsistent macro usage in the %clean section and also dropped the gcc32 patch file in favour of specifying CC=gcc32 on the make line - which I think is a saner method :).
http://www.dribble.org.uk/AGReader.spec http://www.dribble.org.uk/AGReader-1.1-7.iss.src.rpm
I would appreciate some help with regards to the gcc 4+ patch - not really being a C programmer. I'm aware of the issues regarding capital letters in package names but made a decision based on what the tarball was called "upstream" and that I'd released earlier versions of this package under that name. Although in this case the "upstream" may no longer actively be maintained which might make this package unsuitable for inclusion, in hindsight? Of course if it needs to be renamed - that's no problem :)
There are a couple of AmigaGuide files included within the SRPM called test.guide and agr.guide but here's some links to a few more examples
http://www.dribble.org.uk/gofetch.guide http://www.dribble.org.uk/qdevice.guide
I will submit the cegui and DevIL RPMs shortly. Thanks very much :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-27 05:45 EST ------- Created an attachment (id=130064) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=130064&action=vie...) Patch fixing gcc41 compile and all compiler warnings
Ok,
Attached is a patch fixing compiling with gcc4.1 and all compiler warnings. also I've removed the -s from the link command in the Makefile so that a proper -debuginfo package can be generated by rpmbuild.
About the name, the name as is is fine, especially if you've already released rpms with the name like this.
On to a full review:
MUST: ===== 0 rpmlint output is: W: AGReader incoherent-version-in-changelog 1.1-7.iss 1.1-7 This needs fixing. * Package and spec file named appropriately * Packaged according to packaging guidelines * License (GPL) ok, license file not included because it isn't included by upstream * spec file is legible and in Am. English. * Source matches upstream * Compiles and builds on devel-x86_64 * BR: ok * No locales * No shared libraries * Not relocatable * Package owns / or requires all dirs * No duplicate files & Permissions ok * %clean & macro usage OK * Contains code only * %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files. * no gui -> no .desktop file required
MUST fix: ========= * Drop CC=gcc32 from make command params and instead use the attached patch * Add CFLAGS="$RPM_OPT_FLAGS" to make command params so that the rpmoptflags get used during compilation. * Fix incoherent Changelog version. * Home, End, F1, F2, F3 Keys don't work as advertised in the manual, I've a fix
and I'll attach a patch for this.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-27 05:54 EST ------- Created an attachment (id=130065) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=130065&action=vie...) Patch fixing Home, End and Function keys
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From packages@amiga-hardware.com 2006-05-27 09:26 EST ------- Many thanks for the patches Hans. Here's the latest spec and srpm which should fix those issues.
http://www.dribble.org.uk/AGReader.spec http://www.dribble.org.uk/AGReader-1.1-8.src.rpm
I'll also try and submit those patches upstream. I'm working this weekend, but will get on with the other two rpms asap.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader: Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #130065|0 |1 is obsolete| |
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-28 06:10 EST ------- Created an attachment (id=130155) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=130155&action=vie...) Patch fixing Home, End and Function keys
Yesterday I realised after leaving my PC, that I forgot to add the escape sequences used by the function keys under the Linux Console, so here is a new version of the patch which adds the esc-sequences for the Function keys under the console. So now the function keys for this console ag reader not only work under xterm but also actually on the console :). Please use this version of the patch to build a new SRPM before importing into CVS, or replace the patch immidiatly after import (iow before building).
The new version looks good, Approved! I'll set the status to FE-ACCEPTED as soon as the other 2 are approved too and you get your CVS access, otherwise this one my stay FE-ACCEPTED for a couple of days without being imported into CVS, which will make the automated weekly FE-status reports unhappy.
Talking about the automated weekly FE-status reports, for these reports it is important that the Summary of a review bug is always in the form of: Review Request: %name - %summary So not %name: %summary, please change the summary of this bug, the same goes for the DevIL and cegui bugs, which are even more wrong with the <> added.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader - Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
packages@amiga-hardware.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: AGReader: |Review Request: AGReader - |Console reader for viewing |Console reader for viewing |AmigaGuide files |AmigaGuide files
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader - Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From packages@amiga-hardware.com 2006-05-28 13:36 EST ------- Thanks very much for your help on this. Latest version incorporates the new patch, plus a lowercase provides alias. I've also fixed the bugzilla summary for this and the other two.
http://www.dribble.org.uk/AGReader.spec http://www.dribble.org.uk/AGReader-1.1-9.src.rpm
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader - Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163778 |163779 nThis| |
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-05-31 15:33 EST ------- Don't forget to close this with a resolution of next release once AGReader has been built successfully
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader - Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-06-01 09:05 EST ------- I noticed that you've imported this one into CVS but haven't build it yet, are there any problems with building? (same goes for DevIL)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader - Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
------- Additional Comments From packages@amiga-hardware.com 2006-06-01 18:36 EST ------- Oh, I haven't tried yet. I've been working a lot of overtime the last couple of days and was also waiting for the fc4 and fc5 branches to be created, but that's all ready now, so I'm going to try and build them shortly. No problem, I'll mark them both as NEXTRELEASE once they've built successfully.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: AGReader - Console reader for viewing AmigaGuide files
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=190390
packages@amiga-hardware.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla