Bug 214124
Summary: | Review Request: bogl - a graphics library and an Unicode terminal emulator | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Miloslav Trmač <mitr> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | colding, eng-i18n-bugs, mtasaka |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-11-10 16:34:02 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Miloslav Trmač
2006-11-05 21:41:52 UTC
----------------------------------------------------------- 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. |