Bug 225856 - Merge Review: gpm
Merge Review: gpm
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Robert Scheck
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:57 EST by Nobody's working on this, feel free to take it
Modified: 2009-02-23 17:39 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-23 17:39:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
redhat: fedora‑review+


Attachments (Terms of Use)
Patch to solve some issues in gpm spec file (2.92 KB, patch)
2009-01-17 10:08 EST, Robert Scheck
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:57:56 EST
Fedora Merge Review: gpm

http://cvs.fedora.redhat.com/viewcvs/devel/gpm/
Initial Owner: tjanouse@redhat.com
Comment 1 Patrice Dumas 2008-01-03 04:28:04 EST
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.
Comment 2 Patrice Dumas 2008-01-03 05:00:20 EST
Also there is a strange cross dependency on ncurses, see
Bug 226188#13
Comment 3 Patrice Dumas 2008-01-03 05:01:28 EST
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. 
Comment 4 Patrice Dumas 2008-01-03 13:18:59 EST
(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...
Comment 5 Patrice Dumas 2008-01-03 13:55:36 EST
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).
Comment 6 Robert Scheck 2009-01-17 09:54:15 EST
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@GLIBC_2.2.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@GLIBC_2.2.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.
Comment 7 Robert Scheck 2009-01-17 10:08:07 EST
Created attachment 329283 [details]
Patch to solve some issues in gpm spec file
Comment 8 Robert Scheck 2009-01-17 10:17:28 EST
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.
Comment 9 Zdenek Prikryl 2009-01-20 03:58:54 EST
(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.
Comment 10 Zdenek Prikryl 2009-02-03 05:58:50 EST
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.
Comment 11 Zdenek Prikryl 2009-02-04 02:37:09 EST
I did a few little changes. So now, (hopefully) the correct version of the spec file is in the cvs :-)
Comment 12 Robert Scheck 2009-02-04 16:42:16 EST
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@GLIBC_2.2.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?
Comment 13 Zdenek Prikryl 2009-02-05 04:14:30 EST
(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@GLIBC_2.2.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.
Comment 14 Robert Scheck 2009-02-05 05:55:30 EST
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.
Comment 15 Zdenek Prikryl 2009-02-05 08:35:51 EST
Ok, it seems reasonable. Committed.
Comment 16 Robert Scheck 2009-02-05 08:45:39 EST
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.
Comment 17 Robert Scheck 2009-02-23 17:39:37 EST
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.

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