Bug 245917 - Review Request: libtextcat - language guessing library
Review Request: libtextcat - language guessing library
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-27 08:47 EDT by Caolan McNamara
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-06 14:44:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Caolan McNamara 2007-06-27 08:47:14 EDT
Spec URL: http://people.redhat.com/caolanm/libtextcat/libtextcat.spec
SRPM URL: http://people.redhat.com/caolanm/libtextcat/libtextcat-2.2-1.fc8.src.rpm
Description: language guessing library, it will be used by the next version of OOo, so I'd like to get it ready for F-8 so if OOo is released on time we're ready to roll immediately.
Comment 1 Caolan McNamara 2007-06-27 08:48:22 EDT
rpmlint has nothing to say about this package
Comment 2 Ralf Corsepius 2007-06-27 09:51:32 EDT
Some remarks:

* The tarball is mis-packaged:

# rpmbuild -ba libtextcat.spec
...
.../rpms/BUILD/libtextcat-2.2/missing: Unknown `--run' option
...

You might want to patch it (Better: Upstream should use "make dist" to build the
tarball)

* The *devel package doesn't contain headers:
# rpm -qlp libtextcat-devel-2.2-1.fc7.i386.rpm
/usr/bin/createfp
/usr/lib/libtextcat.so -> libtextcat.so.0.0.0

I don't know how this package is supposed to be used, but IMO, a devel package
without headers doesn't make much sense.

* The configuration plays nasty tricks with CFLAGS (-g -O3 -funroll-loops).
Fortunately, they don't have much impact in this case.

Otherwise, this package looks good.
Comment 3 Caolan McNamara 2007-06-27 10:15:50 EDT
oops, k how about ...
Spec URL: http://people.redhat.com/caolanm/libtextcat/libtextcat.spec
SRPM URL: http://people.redhat.com/caolanm/libtextcat/libtextcat-2.2-2.fc8.src.rpm
adds the headers and autoreconf updates the "missing"
Comment 4 Ralf Corsepius 2007-06-27 10:56:00 EDT
(In reply to comment #3)
> adds the headers and autoreconf updates the "missing"

Well, as you probably know, I am not going to approve any package which runs
autoreconf during builds ;)


Apart of this, there now is a showstopper issue:

libtextcat-devel-2.2-2.fc7.i386.rpm's config.h is an (autoheader-generated)
config-header (autoheader).

Installing autoheaders renders a package unusable to other autoconf-based
packages, because the defines inside clash with those defines of other autoconf
based packages (autoheader are supposed to be private).
You will have to find a way to avoid these clashes 

A brute-force way would be to "sed-out" potentially conflicting defines from
${RPM_BUILD_ROOT}%{_includedir}/libtextcat/*.h. A cleaner way would be to add a
second, manually written autoheader. Finally you could resort to patch common.h
to hard-code the linux specific values ;)
Comment 5 Caolan McNamara 2007-06-27 13:07:09 EDT
Spec URL: http://people.redhat.com/caolanm/libtextcat/libtextcat.spec
SRPM URL: http://people.redhat.com/caolanm/libtextcat/libtextcat-2.2-3.fc8.src.rpm

exports the api that we'll need in OOo and tweaks things around to be
includeable from other autoconfed thingies.
Comment 6 Ralf Corsepius 2007-06-27 19:20:41 EDT
- The headers aren't C/C++ safe. They lack 'extern "C" { }' guards.

- You are running the autotools without BuildRequire'ing them.



Comment 7 Caolan McNamara 2007-06-28 04:31:48 EDT
Spec URL: http://people.redhat.com/caolanm/libtextcat/libtextcat.spec
SRPM URL: http://people.redhat.com/caolanm/libtextcat/libtextcat-2.2-4.fc8.src.rpm

sticks a few extern "C" around the headers and adds a BuildRequire for automake
Comment 8 Jason Tibbitts 2007-07-06 11:32:48 EDT
This failed to build for me:

+ autoreconf -f -i
configure.ac:11: error: possibly undefined macro: AC_PROG_LIBTOOL
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
autoreconf: /usr/bin/autoconf failed with exit status: 1
error: Bad exit status from /var/tmp/rpm-tmp.90064 (%build)

I added a build dependency on libtool and it built OK.
Comment 9 Jason Tibbitts 2007-07-06 12:16:59 EDT
I'll assume that the libtool build dependency is there for the purposes of this
review.

rpmlint says:
   W: libtextcat-devel no-documentation
which is OK.

This package screws with the compiler flags as noted above.  Frankly I don't
know what gcc does when it sees -O3 -O2 on the command line, but I'll trust
Ralf's assessment above that it doesn't cause problems. Since you're running the
autotools anyway, it should be easy to patch them out but I don't think it's
mandatory.

So I'll approve this, but of course you'll have to add the libtool dependency so
that you can build.

Review:
* source files match upstream:
   5677badffc48a8d332e345ea4fe225e3577f53fc95deeec8306000b256829655  
   libtextcat-2.2.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper (assuming libtool is there)
? compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint output is OK.
* final provides and requires are sane:
  libtextcat-2.2-4.fc8.x86_64.rpm
   libtextcat.so.0()(64bit)
   libtextcat = 2.2-4.fc8
  =
   /sbin/ldconfig
   libtextcat.so.0()(64bit)

  libtextcat-devel-2.2-4.fc8.x86_64.rpm
   libtextcat-devel = 2.2-4.fc8
  =
   libtextcat = 2.2-4.fc8
   libtextcat.so.0()(64bit)

* %check is not present; no test suite upstream.  I have no way to test this
   package.
* shared libraries installed; ldconfig is called properly and unversioned .so 
   files are in the -devel package.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets OK (ldconfig).
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

APPROVED; just add that libtool dependency.
Comment 10 Caolan McNamara 2007-07-06 12:31:00 EDT
k,

Spec URL: http://people.redhat.com/caolanm/libtextcat/libtextcat.spec
SRPM URL: http://people.redhat.com/caolanm/libtextcat/libtextcat-2.2-5.fc8.src.rpm

New Package CVS Request
=======================
Package Name: libtextcat
Short Description: Text Categorization Library
Owners: caolanm@redhat.com
Branches: devel
Comment 11 Kevin Fenzi 2007-07-06 14:06:01 EDT
cvs done.
Comment 12 Ralf Corsepius 2007-07-07 01:38:47 EDT
(In reply to comment #9)
>
> This package screws with the compiler flags as noted above.  Frankly I don't
> know what gcc does when it sees -O3 -O2 on the command line, but I'll trust
> Ralf's assessment above that it doesn't cause problems.
Normally, in GCC's option processing, later options are supposed to override
earlier options, so the -O2 should win.

Unfortunately, a corner-case in some versions of GCC had been found recently,
where this behavior doesn't apply. I don't know off-head if this issue/bug hits
with Fedora's GCCs nor if the flags/options being used inside of this package
are affected.

> Since you're running the
> autotools anyway, it should be easy to patch them out but I don't think it's
> mandatory.
To be safe, this stuff should be patched out.

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