Bug 190390

*** Bug 190394 has been marked as a duplicate of this bug. ***

*** Bug 190393 has been marked as a duplicate of this bug. ***

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.

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.

One last question, could you post an URL to some amigaguides, so I can test this
program?

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 :)

Created attachment 130064 [details]
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.

Created attachment 130065 [details]
Patch fixing Home, End and Function keys

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.

Created attachment 130155 [details]
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.

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

Don't forget to close this with a resolution of next release once AGReader has
been built successfully

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)

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.