Bug 214124 - Review Request: bogl - a graphics library and an Unicode terminal emulator
Review Request: bogl - a graphics library and an Unicode terminal emulator
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-11-05 16:41 EST by Miloslav Trmač
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-10 11:34:02 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Miloslav Trmač 2006-11-05 16:41:52 EST
Spec URL: http://people.redhat.com/mitr/extras/bogl.spec
SRPM URL: http://people.redhat.com/mitr/extras/bogl-0.1.18-12.src.rpm
Description:
BOGL stands for Ben's Own Graphics Library.  It is a small graphics
library for Linux kernel frame buffers.  It supports only very simple
graphics.

The bterm application is a terminal emulator that displays to a Linux
frame buffer.  It is able to display Unicode text on the console.


This package used to be in Fedora Core and was dropped after anaconda removed
support for it.  This version drops the modifications for anaconda support,
to be included as a standalone package in Extras.
Comment 1 Jules Colding 2006-11-09 10:01:18 EST
-----------------------------------------------------------
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
Comment 2 Mamoru TASAKA 2006-11-09 10:48:24 EST
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.
Comment 3 Mamoru TASAKA 2006-11-09 12:55:50 EST
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?
Comment 4 Miloslav Trmač 2006-11-09 18:23:08 EST
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@redhat.com> - 0:0.1.18-13
- Add URL:
- Preserve modification date of header files
- Ship debian/copyright
- Compile all files with $RPM_OPT_FLAGS
Comment 5 Mamoru TASAKA 2006-11-10 01:00:39 EST
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.
Comment 6 Mamoru TASAKA 2006-11-10 04:28:11 EST
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.
Comment 7 Jules Colding 2006-11-10 04:44:48 EST
(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
Comment 8 Mamoru TASAKA 2006-11-10 04:54:10 EST
(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.

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