Fedora Merge Review: gpm http://cvs.fedora.redhat.com/viewcvs/devel/gpm/ Initial Owner: tjanouse
This spec file should be brought up to date, there are many easy items, it would be nice to do this before the review not to waste anyone time. But I have a concern about circular Build dependencies. Indeed, it seems to me that there is a cirular build loop texinfo -> ncurses-devel -> gpm-devel -> gpm -> install-info in texinfo. One way to avoid this loop would be to have a gpm-lib package. This only makes sense if the library are really independent from the executables. It may be, for example, that the library only works if the server is started. Another way could be to have a subpackage for the info files. Or don't care about it.
Also there is a strange cross dependency on ncurses, see Bug 226188#13
On http://invisible-island.net/ncurses/ncurses.faq.html#using_gpm_lib there is With ncurses 5.5, the recommendation is still the same: build the GPM library without the Gpm_Wgetch interface. ncurses 5.5 can dynamically load the GPM library on Linux, and that eliminates any reason to have the ncurses library built with an explicit dependency upon GPM.
(In reply to comment #3) > On http://invisible-island.net/ncurses/ncurses.faq.html#using_gpm_lib > there is > > With ncurses 5.5, the recommendation is still the same: build the GPM library > without the Gpm_Wgetch interface. ncurses 5.5 can dynamically load the GPM > library on Linux, and that eliminates any reason to have the ncurses library > built with an explicit dependency upon GPM. This seems wrong, however since at least libaa and w3m need the Gpm_Wgetch symbol...
After thinking and looking abit more at it, I don't have an idea on how the Build cross dependency could be avoided since gpm needs the ncurses headers, and ncurses needs the gpm headers (and libs).
Adding Zdenek Prikryl (zprikryl), seems currently to be the maintainer. So lets go for beginning the review: [ DONE ] MUST: rpmlint must be run on every package. The output should be posted in the review. $ rpmlint /var/lib/mock/fedora-10-x86_64/result/gpm-* gpm.src:17: E: prereq-use /sbin/chkconfig /sbin/ldconfig /sbin/install-info gpm.src:459: W: macro-in-%changelog config gpm.src:518: W: macro-in-%changelog preun gpm.src: W: summary-ended-with-dot A mouse server for the Linux console. gpm.src: W: strange-permission gpm.init 0755 gpm.x86_64: W: file-not-utf8 /usr/share/doc/gpm-1.20.5/TODO gpm.x86_64: W: summary-ended-with-dot A mouse server for the Linux console. gpm.x86_64: W: unstripped-binary-or-object /usr/lib64/libgpm.so.2.1.0 gpm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgpm.so.2.1.0 exit.5 gpm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgpm.so.2.1.0 exit@@GLIBC_2.2.5 gpm.x86_64: W: service-default-enabled /etc/rc.d/init.d/gpm gpm.x86_64: W: service-default-enabled /etc/rc.d/init.d/gpm gpm-devel.x86_64: W: no-documentation gpm-devel.x86_64: W: summary-ended-with-dot A mouse server for the Linux console. 4 packages and 0 specfiles checked; 1 errors, 13 warnings. $ [ OK ] MUST: The package must be named according to the Package Naming Guidelines [ OK ] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption [FAILED] MUST: The package must meet the Packaging Guidelines -> See below for details [ OK ] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines [ OK ] MUST: The License field in the package spec file must match the actual license. [ OK ] MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc [ OK ] MUST: The spec file must be written in American English. [ OK ] MUST: The spec file for the package MUST be legible. [ OK ] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. -> 3915bdd6bf947ef867752a30b4be2387 gpm-1.20.5.tar.gz -> 3915bdd6bf947ef867752a30b4be2387 gpm-1.20.5.tar.gz.1 -> 71ee9125414e5a4c3916c5f5f35ee76ca1397f9d gpm-1.20.5.tar.gz -> 71ee9125414e5a4c3916c5f5f35ee76ca1397f9d gpm-1.20.5.tar.gz.1 [ OK ] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [ N/A ] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line [ OK ] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. [ N/A ] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden. [ OK ] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [ N/A ] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker [ OK ] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory [ OK ] MUST: A package must not contain any duplicate files in the %files listing. [ OK ] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. -> We're suggesting %defattr(-,root,root,-) as a should rather current [ OK ] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [ OK ] MUST: Each package must consistently use macros. [ OK ] MUST: The package must contain code, or permissable content [ N/A ] MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [ OK ] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present -> No %doc for -devel? [ OK ] MUST: Header files must be in a -devel package [FAILED] MUST: Static libraries must be in a -static package -> Are the static libraries needed at all? [ N/A ] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [21] [ OK ] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package [ OK ] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [ OK ] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. [ N/A ] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [ OK ] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time [ OK ] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [ OK ] MUST: All filenames in rpm packages must be valid UTF-8. I don't know how to handle the following - ideas? W: shared-lib-calls-exit /usr/lib64/libgpm.so.2.1.0 exit.5 W: shared-lib-calls-exit /usr/lib64/libgpm.so.2.1.0 exit@@GLIBC_2.2.5 There are more things not mentioned here yet, wait for the next comment.
Created attachment 329283 [details] Patch to solve some issues in gpm spec file
You're currently adding "Requires: bash >= 2.0" to gpm package. Is this really needed? Bash < 2.0 existed before 1998 in Red Hat Linux - that was just before Red Hat Linux 5.2. If we still need it, please explain the need for it. Do we really need the static library? If yes, we need a -static subpackage. But personally, I don't see a need for a *.a file - can we remove it? I think, we can ignore macro-in-%changelog warnings, there's nothing which gets expanded here. Do we really need to package the TODO file as %doc? That seems to be needed for upstream, not for downstream, yes? If we need it, we have to convert it to UTF8 using e.g. the following: iconv -f iso-8859-1 -t utf-8 -o TODO.utf8 TODO touch -c -r TODO TODO.utf8 mv -f TODO.utf8 TODO We can't fix W: strange-permission gpm.init 0755 as CVS won't let us do this, AFAIK. Please have a look in the future, that you're importing/adding files with the correct permissions, please (0644) - thanks. BTW, somebody an idea, what causes W: unstripped-binary-or-object /usr/lib64/ libgpm.so.2.1.0 and how to solve it? Zdenek - please take action, thank you.
(In reply to comment #8) > You're currently adding "Requires: bash >= 2.0" to gpm package. Is this really > needed? Bash < 2.0 existed before 1998 in Red Hat Linux - that was just before > Red Hat Linux 5.2. If we still need it, please explain the need for it. It seems that this requires isn't needed any more. > Do we really need the static library? If yes, we need a -static subpackage. But > personally, I don't see a need for a *.a file - can we remove it? We need the static library. So, I added -static subpackage. > I think, we can ignore macro-in-%changelog warnings, there's nothing which gets > expanded here. I changed % to %% so this warnings disappear. > Do we really need to package the TODO file as %doc? That seems to be needed for > upstream, not for downstream, yes? If we need it, we have to convert it to UTF8 > using e.g. the following: > > iconv -f iso-8859-1 -t utf-8 -o TODO.utf8 TODO > touch -c -r TODO TODO.utf8 > mv -f TODO.utf8 TODO > I my opinion this file is needed. If anyone wants to start writing patches, then he'll look into this file and start writing. So, I added the conversion to the spec file. > We can't fix W: strange-permission gpm.init 0755 as CVS won't let us do this, > AFAIK. Please have a look in the future, that you're importing/adding files > with the correct permissions, please (0644) - thanks. Ok. > BTW, somebody an idea, what causes W: unstripped-binary-or-object /usr/lib64/ > libgpm.so.2.1.0 and how to solve it? I'll take a look on this.
Ok, I committed the new spec file into the cvs. The library is now stripped. Please, review it, so I can bump a new release. Also, I did minor clean up in %prep and %bild section.
I did a few little changes. So now, (hopefully) the correct version of the spec file is in the cvs :-)
Thank you for going on, rpmlint against latest CVS build (from http:// cvs.fedoraproject.org/viewvc/devel/gpm/gpm.spec?revision=1.69) > gpm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgpm.so.2.1.0 exit.5 Any ideas for this? That really looks strange to me - and I do not really have a clue what causes this. See also below at the bottom of this comment. > gpm.src: W: strange-permission gpm.init 0755 > gpm.x86_64: W: service-default-enabled /etc/rc.d/init.d/gpm > gpm.x86_64: W: service-default-enabled /etc/rc.d/init.d/gpm > gpm-devel.x86_64: W: no-documentation > gpm-static.x86_64: W: no-documentation Ignore so far. GPM should be enabled per default, otherwise the service does not make so much sense to me. Docs are not available and permission can't be solved after wrong import (as eplained above). > %__cc %{?_smp_mflags} -o inputattach %{SOURCE2} Well, we've lost $RPM_OPT_FLAGS. See build logs: %{?_smp_mflags} only causes -jX, not the rest of the flags $RPM_OPT_FLAGS would bring. So please re-add. Could you perform real integer comparisons rather half string comparisions? -if [ "$1" = "0" ]; then +if [ $1 -eq 0 ]; then -if [ "$1" -ge "1" ]; then +if [ $1 -ne 0 ]; then Following is suggested to not break rpm transaction if something goes wrong: -/sbin/install-info %{_infodir}/gpm.info.gz --delete %{_infodir}/dir +/sbin/install-info %{_infodir}/gpm.info.gz --delete %{_infodir}/dir || : Except of things raised above, I would say we're fine. Most hard seems to me shared-lib-calls-exit - can we avoid it or can we just ignore the warning; I had a look to bug #450011 and if I see correct, it depends on how it is done; sometimes it can't be avoided. You know code better than me...suggestions?
(In reply to comment #12) > Thank you for going on, rpmlint against latest CVS build (from http:// > cvs.fedoraproject.org/viewvc/devel/gpm/gpm.spec?revision=1.69) > > > gpm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgpm.so.2.1.0 exit.5 > > Any ideas for this? That really looks strange to me - and I do not really > have a clue what causes this. See also below at the bottom of this comment. This is caused by exit() function. If something goes wrong, libgpm calls exit(). This isn't standard library behaviour. Even in the source code there is a comment that mentions this fact. But anyway, right now there isn't a easy way how to get rid of exit() calls. I mean, if we want to remove calls, we'll have to change an architecture of library's reporting stuff. So, IMO we can ignore this warning. > > > %__cc %{?_smp_mflags} -o inputattach %{SOURCE2} > > Well, we've lost $RPM_OPT_FLAGS. See build logs: %{?_smp_mflags} only causes > -jX, not the rest of the flags $RPM_OPT_FLAGS would bring. So please re-add. Right, re-added. > Could you perform real integer comparisons rather half string comparisions? > > -if [ "$1" = "0" ]; then > +if [ $1 -eq 0 ]; then > > -if [ "$1" -ge "1" ]; then > +if [ $1 -ne 0 ]; then I had a conversation about this and string comparison is safer than integer comparison. So that's why I prefer this. > > Following is suggested to not break rpm transaction if something goes wrong: > > -/sbin/install-info %{_infodir}/gpm.info.gz --delete %{_infodir}/dir > +/sbin/install-info %{_infodir}/gpm.info.gz --delete %{_infodir}/dir || : IMO, if something goes wrong, user has to be notified. I committed spec with corrections.
If "/sbin/install-info %{_infodir}/gpm.info.gz --delete %{_infodir}/dir" fails, this will make rpm and yum breaking the transaction and thus not updating or deleting. the package and leave stuff in a not-very-well-state. Printing the error will be independent of that, but updating/uninstalling will work then; a thing which we should avoid (otherwise user needs rpm --noscripts etc.). Rest of your explanations is accepted and/or verified in CVS.
Ok, it seems reasonable. Committed.
http://cvs.fedoraproject.org/viewvc/devel/gpm/gpm.spec?revision=1.71 seems now fine to me, thus APPROVED. Please close this bug report, once you've built the changes in Rawhide. Thanks.
Well, gpm-1.20.6-1.fc11 has been built in Rawhide which is newer rather the reviewed package, so closing now. Thank you, Zdenek.