[Bug 214124] New: 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.
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@fedoraproject.org ReportedBy: mitr@redhat.com QAContact: fedora-package-review@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.
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@omesc.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |colding@omesc.com
------- Additional Comments From colding@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...
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.
* 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-RE... * 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
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@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |mtasaka@ioa.s.u-tokyo.ac.jp
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@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163776 |163778 nThis| |
------- Additional Comments From mtasaka@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.
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@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?
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@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-RE... 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
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@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.
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@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163778 |163779 nThis| |
------- Additional Comments From mtasaka@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.
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@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 :-)
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@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.
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@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla