3:41 p.m.
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=214124
Summary: Review Request: bogl - a graphics library and an Unicode
terminal emulator
Product: Fedora Extras
Version: devel
Platform: All
OS/Version: Linux
Status: NEW
Severity: normal
Priority: normal
Component: Package Review
AssignedTo: nobody(a)fedoraproject.org
ReportedBy: mitr(a)redhat.com
QAContact: fedora-package-review(a)redhat.com
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.
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
9:01 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
colding(a)omesc.com changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |colding(a)omesc.com
------- Additional Comments From colding(a)omesc.com 2006-11-09 10:01 EST -------
-----------------------------------------------------------
I'm not a member of sponsors so I can only do a pre-review.
-----------------------------------------------------------
With that out of the way...
* 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.
*
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...
* 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
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
9:44 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
mtasaka(a)ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
AssignedTo|nobody(a)fedoraproject.org |mtasaka(a)ioa.s.u-tokyo.ac.jp
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
9:48 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
mtasaka(a)ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added
----------------------------------------------------------------------------
OtherBugsDependingO|163776 |163778
nThis| |
------- Additional Comments From mtasaka(a)ioa.s.u-tokyo.ac.jp 2006-11-09 10:48 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.
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
11:56 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
------- Additional Comments From mtasaka(a)ioa.s.u-tokyo.ac.jp 2006-11-09 12:55 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?
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
5:23 p.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
------- Additional Comments From mitr(a)redhat.com 2006-11-09 18:23 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...
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(a)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
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
midnight
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
------- Additional Comments From mtasaka(a)ioa.s.u-tokyo.ac.jp 2006-11-10 01:00 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.
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
3:28 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
mtasaka(a)ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added
----------------------------------------------------------------------------
OtherBugsDependingO|163778 |163779
nThis| |
------- Additional Comments From mtasaka(a)ioa.s.u-tokyo.ac.jp 2006-11-10 04:28 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.
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
3:45 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
------- Additional Comments From colding(a)omesc.com 2006-11-10 04:44 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
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
3:54 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
------- Additional Comments From mtasaka(a)ioa.s.u-tokyo.ac.jp 2006-11-10 04:54 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.
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
10:34 a.m.
New subject: [Bug 214124] Review Request: bogl - a graphics library and an Unicode terminal emulator
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: bogl - a graphics library and an Unicode terminal emulator
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214124
mitr(a)redhat.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |CLOSED
Resolution| |NEXTRELEASE
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.
6438
days inactive
6443
days old
package-review@lists.fedoraproject.org
10 comments
1 participants
participants (1)
-
Red Hat Bugzilla