-----------------------------------------------------------
I'm not a member of sponsors so I can only do a pre-review.
-----------------------------------------------------------

With that out of the way...

From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines:

* rpmlint is not silent. 
bash-3.1$ rpmlint ./bogl-0.1.18-12.i386.rpm
W: bogl no-url-tag
bash-3.1$ rpmlint ./bogl-bterm-0.1.18-12.i386.rpm
W: bogl-bterm no-url-tag
bash-3.1$ rpmlint ./bogl-debuginfo-0.1.18-12.i386.rpm
W: bogl-debuginfo no-url-tag
bash-3.1$ rpmlint ./bogl-devel-0.1.18-12.i386.rpm
W: bogl-devel no-url-tag
W: bogl-devel no-documentation
bash-3.1$ rpmlint ../../SRPMS/bogl-0.1.18-12.src.rpm
W: bogl no-url-tag
I can see from the comment in the specthat there really aren't any URL
presently, but I think that one should be provided/created.
* There is no use of the %find_lang macro in the spec. There are no locale files
so maybe this is not needed anyway?
* /usr/share/bogl is not owned by any package (use %dir)
* /usr/include/bogl is not owned by any package (use %dir)
* You are using: "Requires: bogl = %{epoch}:%{version}-%{release}".
  Why not: "Requires: %{name} = %{version}-%{release}" ? Se also my comment
about the epoch tag below.

From http://fedoraproject.org/wiki/Packaging/Guidelines:
* Timestamps: Consider using "install -p" and "cp -p". You could use
INSTALL="install -c -p" in your make install command

Other issues:
* I can't see any resaon why you need to use the epoch tag. Version numbers like
0.1.18 are not hard for RPM to parse at all. See e.g. here:
http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-tags.html#S3-RPM-INSIDE-REQUIRES-TAG
* There is no COPYING file in the top-level source directory. There should be.
* The following files do not have a license notice:
  - bogl-bgf.c
  - bogl-bgf.h
  - bogl-term.h
  - boxes.h
  - bterm.ti
  - mergebdf
  - README
  - README.BOGL-bterm
  - reduce-font.c
  - utils/add_changelog_line
* There is no explicit copyright notice in any of the source files _except_ for
the following:
  - bogl-term.c
  - bogl-vga16.c
  - *.bdf
* The *.bdf files are not under the GPL, but they appear to be free enough
* bogl does not use autoconf/automake. I really do find that the old Makefile
way is to inflexible.
* Please use "Release: 12%{?dist}" not "Release: 12"
* There is a lot of compile warnings. These warning should be reviewd for
seriousness. I know this is just me being overly strict, but I would prefer the
Werror compile flag to be mandatory for all F[C,E] packages.

HTH,
  jules

I will check this package later as I maintain jfbterm,
which is mainly used by Japanese people to display Kanji
characters on Linux frame buffers.

Well, for my viewpoint:

1. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
* Licensing
* Rpmlint
  - Well, rpmlint complains about no-url-tag.
    It seems that this package is currently maintained by debian
    people. I recommend that you add the URL which shows that
    this package is now maintained by debian.

    Also, you should add "debian/copyright" to main package AND -bterm package
    as -bterm package does not require main package.

* Tags
  - Use %?dist tag.
  - I recommend using %_datadir instead of /usr/share.

* Compiler flags
  - fedora specific compilation flags are not passed.
------------------------------------------------------
+ make DESTDIR=/var/tmp/bogl-0.1.18-12-root-mockbuild libdir=/usr/lib install
cc -O2 -g -D_GNU_SOURCE -Wall -D_GNU_SOURCE -DBOGL_CFB_FB=1 -DBOGL_VGA16_FB=1 -o
bdftobogl.o -c bdftobogl.c
cc   bdftobogl.o bogl.o bogl-font.o bogl-cfb.o bogl-pcfb.o bogl-tcfb.o
bogl-vga16.o   -o bdftobogl
./bdftobogl helvB10.bdf > helvB10.c
cc -O2 -g -D_GNU_SOURCE -Wall -D_GNU_SOURCE -DBOGL_CFB_FB=1 -DBOGL_VGA16_FB=1 -o
helvB10.lo -fPIC -c helvB10.c
./bdftobogl helvB12.bdf > helvB12.c
cc -O2 -g -D_GNU_SOURCE -Wall -D_GNU_SOURCE -DBOGL_CFB_FB=1 -DBOGL_VGA16_FB=1 -o
helvB12.lo -fPIC -c helvB12.c
./bdftobogl helvR10.bdf > helvR10.c
......
------------------------------------------------------

* Timestamps
  I always recommend to keep timestamps of
  - text files (including header files)
  - image files
  - etc
  to show clearly when they are created/modified. For the case of
  this package, keeping timestamps of header files in -devel package
  is preferable.
  For this case, the usual method 'make INSTALL="install -c -p"
  install cannot be used. Try like:
     sed -i -e 's|install|install -p|g' Makefile
  or so.

2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
   = Nothing.

3. Other things I have noticed:
* For bogl-bterm
  - Well, Japanese people (including me) always complain about bterm
    as this software (bterm) is not useful for non-root users because
    *it seems* bterm requires device access right to /dev/tty0 .
   (I have not checked the whole source code of bterm and perhaps it is
    impossible for me).
    What do you think of this?

    + kon2 (which was in Core till FC5, however it was removed for FC6)
      used to setuid on kon binary.
    + For jfbterm, I decided to use console.perms method. (through long 
      discussion with the original maintainer) This method
      allows the FIRST user to use CUI frame buffer to gain device access 
      right.
    + or should we leave this as it was?

+++++++++++++++++++++++++++++++++++++++++++++++++++++++
In my opinion, the following are okay.

= %find_lang
  There is no gettext mo files so %find_lang macro is not
  needed, this is correct.

= /usr/share/bogl
  This is correctly owned by bogl-bterm

= /usr/include/bogl
  This is correctly owned by bogl-bterm

= You are using: "Requires: bogl = %{epoch}:%{version}-%{release}"
  This is correct when using epoch.

= I can't see any resaon why you need to use the epoch tag
  For this package, epoch is needed as Epoch was already used
  when this package was in Fedora Core.

= source files license issue:
  Well, surely some of the source files are not explicitly
  licensed, however, for now I trust that these are licensed 
  under GPL accroding to debian/copyright.

= bogl does not use autoconf/automake
  In my opinion, autoconf/automake should not be used unless
  it is unavoidable and there is no problem for this package.

= There is a lot of compile warnings
  Well, compilation warnings should be avoided as well as
  possible, however my opinion is this is not a blocker 
  as long as the warnings are not _crucial_ .
  I maintain xscreensaver, of which the compilation warning canNOT
  be avoided because of gtk2 "bug".

  Jules, any comments?

Thanks for the comments!

(In reply to comment #1)
> * rpmlint is not silent. 
[SNIP]
> I can see from the comment in the specthat there really aren't any URL
> presently, but I think that one should be provided/created.
packages.debian.org URL added.

> * There is no use of the %find_lang macro in the spec. There are no locale files
> so maybe this is not needed anyway?
Exactly.

> * /usr/share/bogl is not owned by any package (use %dir)
AFAICS this directory is owned by bogl-bterm.

> * /usr/include/bogl is not owned by any package (use %dir)
... and this one by bogl-devel.

> * You are using: "Requires: bogl = %{epoch}:%{version}-%{release}".
>   Why not: "Requires: %{name} = %{version}-%{release}" ? Se also my comment
> about the epoch tag below.
Because the package does have Epoch: 0.  Even if 0 behaves exactly the same as
"not present" (which I'm not sure is true), it seems better not to rely on this.
 
> From http://fedoraproject.org/wiki/Packaging/Guidelines:
> * Timestamps: Consider using "install -p" and "cp -p". You could use
> INSTALL="install -c -p" in your make install command
Changed for the header files, the other files are created during the build.

> Other issues:
> * I can't see any resaon why you need to use the epoch tag. Version numbers like
> 0.1.18 are not hard for RPM to parse at all. See e.g. here:
>
http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-tags.html#S3-RPM-INSIDE-REQUIRES-TAG
The epoch is already in the old Fedora / RHEL packages, and
http://fedoraproject.org/wiki/Tools/RPM/VersionComparison seems to say removing
the epoch could break upgrades.

> * There is no COPYING file in the top-level source directory. There should be.
Too bad.

I have added the debian/copyright files, although I don't think they can really
replace the licence.

> * The *.bdf files are not under the GPL, but they appear to be free enough

> * bogl does not use autoconf/automake. I really do find that the old Makefile
> way is to inflexible.
This should be fixed upstream, not in Fedora packaging.

> * Please use "Release: 12%{?dist}" not "Release: 12"
AFAIK the dist tag is not mandatory, and I'd rather not use it for the main
branch if possible.

> * There is a lot of compile warnings. These warning should be reviewd for
> seriousness. I know this is just me being overly strict, but I would prefer the
> Werror compile flag to be mandatory for all F[C,E] packages.
I have reviewed them about a year ago, and IIRC all the remaining warnings are
harmless.

> - I recommend using %_datadir instead of /usr/share.
Both /usr/share/terminfo and /usr/share/bogl are hardcoded in the bogl
installation scripts and the source, respectively, so the spec file should use
/usr/share as well.

> - fedora specific compilation flags are not passed.
Fixed.

> - Well, Japanese people (including me) always complain about bterm
>   as this software (bterm) is not useful for non-root users because
>   *it seems* bterm requires device access right to /dev/tty0 .
>  (I have not checked the whole source code of bterm and perhaps it is
>   impossible for me).
It fails on opening /dev/tty0, but even if that were removed, bogl needs root
privileges anyway to drive the VGA hardware.  I have looked at the kernel code a
bit and I couldn't find any alternative, although I'm not very familiar with the
framebuffer API.

I'd prefer leaving the program as is, I don't think it was audited for running
by hostile users.


http://people.redhat.com/mitr/extras/bogl-0.1.18-13.src.rpm :
* Fri Nov 10 2006 Miloslav Trmac <mitr> - 0:0.1.18-13
- Add URL:
- Preserve modification date of header files
- Ship debian/copyright
- Compile all files with $RPM_OPT_FLAGS

Well, I have not yet checked your new srpm, however:

(In reply to comment #4)
> Thanks for the comments!
> > * Please use "Release: 12%{?dist}" not "Release: 12"
> AFAIK the dist tag is not mandatory, and I'd rather not use it for the main
> branch if possible.

For using FE CVS system, this seems mandatory as we have to make 
a DIFFERENT tag for different branch. Without %?dist tag, you have
to change release number manually for each branch (-devel, 6, 5)
even there is no difference for srpm, which seems annoying for me
Note that you have to rebuild this against -devel branch first,
so you have to 'decrease' release number.
(however if you have a different idea, this is not a blocker).

Anyway I will check for the other things later.

I checked again and everything is okay.
I recommend adding ?dist tag, however, 
http://fedoraproject.org/wiki/Packaging/DistTag
says it is not mandatory.

--------------------------------------------------------------
  This package (bogl) is ACCEPTED by me.

(In reply to comment #3)
> 
>   Jules, any comments?

Sure. I'll jump straight to your comments to my comments. This is, after all, a
learning experience for me.


> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> In my opinion, the following are okay.
> 
> = /usr/share/bogl
>   This is correctly owned by bogl-bterm
>
> = /usr/include/bogl
>   This is correctly owned by bogl-bterm

Hmm.. I was under the false impression that the right way to claim directory
ownership was to state the directory with the %dir macro first and later list
all files within that directory.

 
> = You are using: "Requires: bogl = %{epoch}:%{version}-%{release}"
>   This is correct when using epoch.
> 
> = I can't see any resaon why you need to use the epoch tag
>   For this package, epoch is needed as Epoch was already used
>   when this package was in Fedora Core.

OK, but epoch is generally frowned upon, right?


> = source files license issue:
>   Well, surely some of the source files are not explicitly
>   licensed, however, for now I trust that these are licensed 
>   under GPL accroding to debian/copyright.

OK, but was I correct in bringing the "issue" to light? I remember reading
somewhere on gnu.org that each and every file should explicitly state the
license terms as well as a copyright notice.


> = bogl does not use autoconf/automake
>   In my opinion, autoconf/automake should not be used unless
>   it is unavoidable and there is no problem for this package.

OK. It is just me beeing overly pedantic here...


> = There is a lot of compile warnings
>   Well, compilation warnings should be avoided as well as
>   possible, however my opinion is this is not a blocker 
>   as long as the warnings are not _crucial_ .
>   I maintain xscreensaver, of which the compilation warning canNOT
>   be avoided because of gtk2 "bug".

No, surely not a blocker. Miloslav also states above that he did review all of
those warning about a year ago, so they should be harmles.

Now of to pre-review some other unfortunate package :-)

-- 
  jules

(In reply to comment #7)
> > = /usr/share/bogl
> > = /usr/include/bogl
> >   This is correctly owned by bogl-bterm
> Hmm.. I was under the false impression that the right way to claim directory
> ownership was to state the directory with the %dir macro first and later list
> all files within that directory.
Your way is polite and recommended, however, not a few people
(including Miloslav and me) just write the directory name,
which means the directory itself and all the files under the directory.

> > = You are using: "Requires: bogl = %{epoch}:%{version}-%{release}"
> >   This is correct when using epoch.
> > 
> > = I can't see any resaon why you need to use the epoch tag
> >   For this package, epoch is needed as Epoch was already used
> >   when this package was in Fedora Core.
> 
> OK, but epoch is generally frowned upon, right?
Yes, generally epoch should be avoided, however, *ONCE* it is used
it becomes inevitable......

> > = source files license issue:
> >   Well, surely some of the source files are not explicitly
> >   licensed, however, for now I trust that these are licensed 
> >   under GPL accroding to debian/copyright.
> 
> OK, but was I correct in bringing the "issue" to light? I remember reading
> somewhere on gnu.org that each and every file should explicitly state the
> license terms as well as a copyright notice.
I think this should be left to the discussion with upstream.