Bug 209214 - Review Request: libprelude - Prelude library collection
Review Request: libprelude - Prelude library collection
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 209215 209217 209219 209220 209222 209224
  Show dependency treegraph
 
Reported: 2006-10-03 17:27 EDT by Thorsten Scherf
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-30 19:24:15 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
perlbindings failing (140.83 KB, text/plain)
2006-12-19 17:37 EST, Ruben Kerkhof
no flags Details
Libprelude without perl and python bindings. (872 bytes, patch)
2006-12-20 13:21 EST, Ruben Kerkhof
no flags Details | Diff

  None (edit)
Description Thorsten Scherf 2006-10-03 17:27:54 EDT
Spec URL: http://people.redhat.com/tscherf/fedora-extra/
SRPM URL: http://people.redhat.com/tscherf/fedora-extra/
Description: 
The Prelude Library is a collection of generic functions providing
communication between the Prelude Hybrid IDS suite components. It
provides a convenient interface for sending alerts to Prelude
Manager with transparent SSL, failover and replication support,
asynchronous events and timer interfaces, an abstracted
configuration API (hooking at the commandline, the configuration
line, or wide configuration, available from the Manager), and a
generic plugin API. It allows you to easily turn your favorite
security program into a Prelude sensor.

This is my first package and I need a sponsor.
Comment 2 Ralf Corsepius 2006-10-05 05:14:47 EDT
Thorsten, would please make yourself familiar with the Fedora Packaging
Guidelines and modify your spec to its conventions.

Just to point out a some issues:
* Source: should be an absolute URL
* Remove "Prefix:"
* Adopt FPC's conventions on "Release:" and "BuildRoot:"
* Use %configure instead of ./configure
* %{_libdir}/*.so's belong into *-devel
* Shipping static libs
* *devel contains files which should go to *-debuginfo-* only.
Comment 3 Ralf Corsepius 2006-10-19 01:09:53 EDT
Ping? 

Thorsten, shouldn't you show up again before Nov 3rd (One month after
submission), I'll close this request, then.
Comment 5 Thorsten Scherf 2006-11-01 10:58:04 EST
ping?!
Comment 6 Ruben Kerkhof 2006-12-19 17:37:05 EST
Created attachment 144049 [details]
perlbindings failing
Comment 7 Ruben Kerkhof 2006-12-19 17:39:41 EST
Hi Thorsten,

I tried building the srpm on FC-6, but it failed on the perl and python bindings. Building the tarball 
worked though, so I'm not sure what's wrong. Buildlog is attached.
Comment 8 Thorsten Scherf 2006-12-20 05:33:22 EST
ok, it must have something to do with the perl-bindings. installing the SRPM in
mock works without any problems, but I guess you have perl-devel installed which
forces the configure script to do perl-bindings. I disabled them in a new packeg
which is available here:

https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=144049&action=view

could you please check of this one works without the binding problem?

Thanks,
Thorsten
Comment 9 Thorsten Scherf 2006-12-20 05:34:46 EST
sorry, correct link for the SRPM is:

http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.11-3.src.rpmv
Comment 10 Thorsten Scherf 2006-12-20 05:35:17 EST
arrg, next try:

http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.11-3.src.rpm
Comment 11 Ruben Kerkhof 2006-12-20 13:21:31 EST
Created attachment 144124 [details]
Libprelude without perl and python bindings.
Comment 12 Ruben Kerkhof 2006-12-20 13:26:23 EST
I didn't have perl-devel installed, but nevermind, now it fails on the python bindings.
--disable-perl doesn't work, it's --enable-perl=no. I disabled perl and python and now it builds.
Comment 13 Thorsten Scherf 2006-12-20 16:35:56 EST
info from upstream was to remove the smp compiler flag which often leads to
problem with perl- and python bindings.

could you please test the new release, if this one builds without problems (in
my mock buildsystem it actually builds).

new link is:
http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.11-4.src.rpm

 
Comment 14 Ruben Kerkhof 2006-12-21 13:30:33 EST
Ah, much better, now it builds in my mock as well.

I'm not a sponsor, but I do have a few notes:

Review for release 4:
* RPM name is OK
* Source libprelude-0.9.11.tar.gz is the same as upstream
* Builds fine in mock
* rpmlint of libprelude looks OK

Needs work:
* Use of buildroot is not consistant
  (wiki: PackagingGuidelines#UsingBuildRootOptFlags)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* rpmlint of libprelude-devel: Please fix the errors and warnings
* The package should contain the text of the license
  Please add COPYING to %doc, and ChangeLog
  (wiki: Packaging/ReviewGuidelines)
* The package owns /usr/share/doc, which is a standard directory
  (wiki: Packaging/ReviewGuidelines)
* Config files of libprelude: Is there a reason the config files are in /etc/prelude/prelude and not in /
etc/prelude?

Minor:
* Duplicate BuildRequires: gnutls (by gnutls-devel)
* The latest upstream version is 0.9.12.

rpmlint output for libprelude-devel-0.9.11-3.i386.rpm
W: libprelude-devel summary-ended-with-dot Header files and libraries for libprelude development.
W: libprelude-devel non-standard-group Environment/Libraries
E: libprelude-devel standard-dir-owned-by-package /usr/share/doc

Thanks,

Ruben
Comment 15 Thorsten Scherf 2006-12-21 19:16:35 EST
* now used %buildroot all over the spec file
* removed smp-flags/-j flag because the package won't build with it
* added doc files
* moved config to /etc/prelude instead of /etc/prelude/prelude
* removed duplicate BuildReqs
* upgraded to new upstream verson 0.9.12

no more output from rpmlint on the SRPM.

any sponsor available?

new package is online:
http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.12-1.src.rpm
http://people.redhat.com/tscherf/fedora-extra/libprelude.spec
Comment 16 Ralf Corsepius 2006-12-22 01:27:44 EST
(In reply to comment #15)

> * moved config to /etc/prelude instead of /etc/prelude/prelude
Well, did you?
# rpm -qlp libprelude-0.9.12-1.i386.rpm | grep /etc/prelude
/etc/prelude
/etc/prelude/prelude
/etc/prelude/prelude/default
/etc/prelude/prelude/default/client.conf
/etc/prelude/prelude/default/global.conf
/etc/prelude/prelude/default/idmef-client.conf
/etc/prelude/prelude/default/tls.conf
/etc/prelude/prelude/profile

Further issues:

- The perl modules are being installed into site_perl
# rpm -qlp libprelude-devel-0.9.12-1.i386.rpm | grep perl5
/usr/lib/perl5/5.8.8/i386-linux-thread-multi/perllocal.pod
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/Prelude.pm
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto/Prelude
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto/Prelude/.packlist
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto/Prelude/Prelude.bs
/usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi/auto/Prelude/Prelude.so

Fedora supplied perl-modules MUST go into vendor_perl (or outside of the perl5/
hierarchy). Installing into site_perl is a no-no.

- The perl-module contains files supposed not to be shipped (.packlist, etc.)
You might want to check how perl modules are suppose to be packaged in FE.

- Package must not own /usr/share/aclocal
# rpm -qlp libprelude-devel-0.9.12-1.i386.rpm | grep aclocal
/usr/share/aclocal
/usr/share/aclocal/libprelude.m4

The FPC has not decided upon yet, but so far the recommendation is to requires
packages providing aclocal macros to 
Requires: automake

- I don't understand why the perl-module is part of *-devel.
Perl modules normally all are run-time packages.

- I'd split the language bindings into separate packages (esp. move the perl
module into a separate perl-Prelude package). This would avoid pulling in
unnecessary deps in cases users are only interested into one of the language
bindings.

I am not sufficiently familiar with python, but I presume similar consideration
as to the perl bindings also apply to it (Toshio, Ville, f13?)
Comment 17 Thorsten Scherf 2006-12-22 05:47:26 EST
ok, here we go:

* config is _now_ in /etc/prelude
* perl-modules are now in vendor_perl instead of site_perl
* unnecessary files were deleted 
* package does not own /usr/share/aclocal anymore
* automake is a new Requirement for libprelude-devel
* language bindings are in separated packages now

sponsors?

new package is here:
http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.12-2.src.rpm
http://people.redhat.com/tscherf/fedora-extra/libprelude.spec


   
Comment 18 Mamoru TASAKA 2006-12-25 05:03:39 EST
Well,

Compared to http://fedoraproject.org/wiki/Packaging/Guidelines :

* Requires
  - Check the requirement for -devel package.
--------------------------------------------------------------
[tasaka1@dhcp158 bin]$ ./libprelude-config --libs
-L/usr/lib -lprelude -lgnutls -lgcrypt -lgpg-error -lrt -ldl
--------------------------------------------------------------
    This usually means that -devel package should require
    gnutls-devel.

* Beware of Rpath
  - This package fails on rpath checking.
-----------------------------------------------------------
+ /usr/lib/rpm/find-debuginfo.sh /home/tasaka1/rpmbuild/BUILD/libprelude-0.9.12
extracting debug info from
/var/tmp/libprelude-0.9.12-2-root-tasaka1/usr/bin/prelude-adduser
extracting debug info from
/var/tmp/libprelude-0.9.12-2-root-tasaka1/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/Prelude/Prelude.so
extracting debug info from
/var/tmp/libprelude-0.9.12-2-root-tasaka1/usr/lib/libprelude.so.2.9.0
extracting debug info from
/var/tmp/libprelude-0.9.12-2-root-tasaka1/usr/lib/python2.5/site-packages/_prelude.so
10681 blocks
+ /usr/lib/rpm/check-rpaths /usr/lib/rpm/check-buildroot
*******************************************************************************
*
* WARNING: 'check-rpaths' detected a broken RPATH and will cause 'rpmbuild'
*          to fail. To ignore these errors, you can set the '$QA_RPATHS'
*          environment variable which is a bitmask allowing the values
*          below. The current value of QA_RPATHS is 0x0000.
*
*    0x0001 ... standard RPATHs (e.g. /usr/lib); such RPATHs are a minor
*               issue but are introducing redundant searchpaths without
*               providing a benefit. They can also cause errors in multilib
*               environments.
*    0x0002 ... invalid RPATHs; these are RPATHs which are neither absolute
*               nor relative filenames and can therefore be a SECURITY risk
*    0x0004 ... insecure RPATHs; these are relative RPATHs which are a
*               SECURITY risk
*    0x0008 ... the special '$ORIGIN' RPATHs are appearing after other
*               RPATHs; this is just a minor issue but usually unwanted
*    0x0010 ... the RPATH is empty; there is no reason for such RPATHs
*               and they cause unneeded work while loading libraries
*    0x0020 ... an RPATH references '..' of an absolute path; this will break
*               the functionality when the path before '..' is a symlink
*          
*
* Examples:
* - to ignore standard and empty RPATHs, execute 'rpmbuild' like
*   $ QA_RPATHS=$[ 0x0001|0x0010 ] rpmbuild my-package.src.rpm
* - to check existing files, set $RPM_BUILD_ROOT and execute check-rpaths like
*   $ RPM_BUILD_ROOT=<top-dir> /usr/lib/rpm/check-rpaths
*  
* 'check-rpaths' is part of 'rpmdevtools'.
*
*******************************************************************************
ERROR   0010: file
'/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/Prelude/Prelude.so'
contains an empty rpath in []
error: Bad exit status from /var/tmp/rpm-tmp.71892 (%install)

-----------------------------------------------------------
    Prelude.so contains empty rpath and this should be removed.

* Timestamps
  - Please consider to keep timestamps on installed files,
    especially on text files because
    * timestamps can show when the files are created and
      if vendor (like you) have modified the files.
    This package contain many HTML documents and header files,
    so keeping timestamps on those files are preferable.

    For this package, this can be done by
------------------------------------------------------------
make install DESTDIR=%{buildroot} INSTALL="%{__install} -c -p"
cp -p AUTHORS ChangeLog README INSTALL LICENSE.README HACKING.README \
------------------------------------------------------------
Comment 19 Mamoru TASAKA 2006-12-25 05:12:51 EST
Oops. I forgot to add the following comments...

* File and Directory Ownership
  - Please own all directories which are not owned by other
    packages needed by this package.
    From a quick view, the following directories are not owned
    by any package.
-------------------------------------------------------------
/usr/share/doc/libprelude-0.9.12/
/usr/include/libprelude/
-------------------------------------------------------------
  - And.. please don't own directories which are owned by other
    packages needed by this package. This package owns the
    following directories unnecessarily.
--------------------------------------------------------------
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto
--------------------------------------------------------------
Comment 20 Mamoru TASAKA 2006-12-25 07:41:35 EST
More comments:

Please check the permissions of files:
-------------------------------------------------------------
E: libprelude-perl non-standard-executable-perm
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/Prelude/Prelude.so
0555
-------------------------------------------------------------
Also, check if 0444 permission of
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/Prelude.pm
is correct.
Comment 21 Thorsten Scherf 2006-12-27 10:13:34 EST
ok, rpath and permission problems are fixed now. timestamp issue is fixed as well.

new package is available here:
http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.12-3.src.rpm
http://people.redhat.com/tscherf/fedora-extra/libprelude.spec
Comment 22 Mamoru TASAKA 2006-12-28 13:57:40 EST
Well, I will check -3 package today (in Japan: EST+14h)...
Comment 23 Mamoru TASAKA 2006-12-29 00:52:26 EST
Well,

A. Genenal packaging issues
* Please add the following documentation(s)
----------------------------------------
NEWS
COPYING
----------------------------------------
* Please remove the following documentation(s)
----------------------------------------
INSTALL - only need by manual installation and
          not needed by rpm installation
----------------------------------------

B. For debuginfo rpm issue
! It seems that some of the source files are
  borrowed from libgpg-error.
  (borrowed means "copied with some modifications for the
   usage of libprelude")
---------------------------------------
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/code-from-errno.c
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/code-from-errno.h
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/code-to-errno.c
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/code-to-errno.h
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/err-codes.h
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/err-sources.h
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/strerror.c
/usr/src/debug/libprelude-0.9.12/src/libprelude-error/strsource.c
---------------------------------------
  Usually local copies of other libraries are
  forbidden, however, as long as I checked how 
  these source codes are used, these codes can be
  allowed because it seems that the part of codes
  borrowed from libgpg-error seems very trivial.

  However, would you check if this is proper?
  (IMO this is not a blocker for this package).

Then:
C: Related to http://fedoraproject.org/wiki/Packaging/Guidelines
* Use rpmlint
------------------------------------------------
E: libprelude-perl script-without-shebang
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/Prelude.pm
------------------------------------------------
   permission should be 0644 for this file.

* BuildRequires
  - Mockbuild fails.
  BuildRequires: gnutls should be BuildRequires: gnutls-devel

* Parallel make
  - Does this package fail on parallel make?
    If not, please use make %{?_smp_mflags}

D. Related to http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
  = This is okay, except for the issues on A-C.
Comment 24 Thorsten Scherf 2006-12-29 04:21:57 EST
* new docs (NEWS, COPYING) added to the package, removed INSTALL

* corrected permissions on
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/Prelude.pm 
to 0644

* gnutls-devel is added to BuildRequires, MockBuild works now again

* when using parallel make, package building fails. thats the reason why I avoid
using parallel make with this package.

* no other issues found

new package is available here:
http://people.redhat.com/tscherf/fedora-extra/libprelude-0.9.12-4.src.rpm
http://people.redhat.com/tscherf/fedora-extra/libprelude.spec


Comment 25 Mamoru TASAKA 2006-12-29 11:51:34 EST
( Some comments mentioned on bug 209215 )
Comment 26 Mamoru TASAKA 2006-12-30 00:02:27 EST
Well,
now this package meets
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
------------------------------------------------------------
  This package (libprelude) is APPROVED by me.
------------------------------------------------------------

Please step forward according to
http://fedoraproject.org/wiki/Extras/Contributors
Comment 27 Mamoru TASAKA 2006-12-30 00:08:29 EST
Umm...
It seems that Paul Johnson has already sponsored you
(I don't know why, however, it is okay..)

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