Bug 498873 - Review Request: thrift - A multi-language RPC and serialization framework
Review Request: thrift - A multi-language RPC and serialization framework
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2009-05-04 04:02 EDT by Silas Sewell
Modified: 2011-03-24 15:55 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-12-29 23:10:16 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Silas Sewell 2009-05-04 04:02:23 EDT
Spec Url:
http://silassewell.googlecode.com/svn/trunk/projects/packages/rpms/thrift/thrift.spec

SRPM Url:
http://silassewell.googlecode.com/files/thrift-0.0-0.fc10.20090501svn770888.src.rpm

Description:
Thrift is a software framework for scalable cross-language services
development. It combines a powerful software stack with a code generation
engine to build services that work efficiently and seamlessly between C++,
Java, C#, Python, Ruby, Perl, PHP, Objective C/Cocoa, Smalltalk, Erlang,
Objective Caml, and Haskell.

rpmlint

[silas@silas rpmbuild]$ rpmlint /var/lib/mock/fedora-10-i386/result/*.rpm
thrift-erlang.i386: W: only-non-binary-in-usr-lib
thrift-ghc-prof.i386: W: devel-file-in-non-devel-package /usr/lib/ghc-6.10.1/Thrift-0.1.0/libHSThrift-0.1.0_p.a
16 packages and 0 specfiles checked; 0 errors, 2 warnings.

These warnings are OK according to:

thrift-erlang.i386: W: only-non-binary-in-usr-lib
 - https://bugzilla.redhat.com/show_bug.cgi?id=455187#c6

thrift-ghc-prof.i386: W: devel-file-in-non-devel-package /usr/lib/ghc-6.10.1/Thrift-0.1.0/libHSThrift-0.1.0_p.a
 - https://bugzilla.redhat.com/show_bug.cgi?id=470756#c8

NOTE: The summary and description were taken from a previous package by Devan Goodwin (446993).
Comment 1 Silas Sewell 2009-05-04 04:11:16 EDT
NOTE the csharp modules is commented about because upstream doesn't provide a "strong name". I'll contact them at some point this week and get a bug filed, but otherwise it builds fine.

Output from failed build:

DEBUG: + gacutil -i lib/csharp/Thrift.dll -f -package Thrift -root /builddir/build/BUILDROOT/thrift-0.0-0.fc10.20090501svn770888.i386/usr/lib
DEBUG: Failure adding assembly lib/csharp/Thrift.dll to the cache: Attempt to install an assembly without a strong name
Comment 3 Silas Sewell 2009-05-16 21:36:16 EDT
C# bug reported: https://issues.apache.org/jira/browse/THRIFT-509
Comment 4 arthurguru 2009-05-19 01:37:21 EDT
Hi Silas,

Whilst doing an informal review for another package, I questioned the %{?dist} location used for the Release: field in the spec file when intermixed with a snapshot string.

It appears that the correct syntax in your case should be:
Release:          0.20090505svn%{snapshot}%{?dist}

Recommend you check this out further and correct if necessary

Best regards,
Arthur Gouros.
Comment 7 Jason Tibbitts 2009-11-12 21:26:50 EST
Just looking over some old tickets; this one fails to build for me.  Here's a scratch build in koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1804331

Please clear the whiteboard if you provide a package that builds.
Comment 8 Silas Sewell 2009-11-13 02:31:25 EST
The build errors should be fixed and I've updated to the latest snapshot.

Unfortunately thrift now only build against devel because of the introduction of slf4j into the Java library.

Also note the spec file url has changed.

Spec:
http://github.com/silas/rpms/raw/master/thrift/thrift.spec

SRPM:
http://cloud.github.com/downloads/silas/rpms/thrift-0.2-0.20091112svn835538.fc12.src.rpm

Diff:
http://github.com/silas/rpms/commit/3d6a44f91b8532e88452f6fa9cf358eb332ba81c
Comment 9 Jason Tibbitts 2010-01-06 21:59:41 EST
Sorry for not being able to take another look at this sooner, but it still doesn't seem to build for me:

+ pushd lib/hs
+ /usr/bin/runghc Setup configure --prefix=/usr --libdir=/usr/lib64 --docdir=/usr/share/doc/thrift-0.1.0 --htmldir=/usr/share/doc/ghc/libraries/Thrift-0.1.0 '--libsubdir=$compiler/$pkgid' --ghc -p
Configuring Thrift-0.1.0...
Setup: At least the following dependencies are missing:
network -any

This is against current rawhide.  Unfortunately I've no idea at all about Haskell so I'm not even sure what to do to debug it.  A scratch build is at http://koji.fedoraproject.org/koji/taskinfo?taskID=1906387 if you'd like to see the full logs.
Comment 10 Silas Sewell 2010-01-07 03:26:35 EST
I fixed the dependency issue, unfortunately it looks like Haskell is pretty broken in rawhide.

Looking at http://fedoraproject.org/wiki/SIGs/Haskell and some of other Haskell related bugs it looks like they're rebuilding the specs with a new tool (or something...).

Either way the package below builds on F-12.

SRPM: http://cloud.github.com/downloads/silas/rpms/thrift-0.2-0.5.20091112svn835538.fc12.src.rpm

If they don't get it fixed soon I'll remove the haskell libraries and rebuild.

P.S. I'm slowing adding EPEL support, so thats the reason for the extra conditionals.
Comment 11 Jason Tibbitts 2010-01-25 18:51:54 EST
This is still marked as not building.  Could someone rectify that?
Comment 12 Silas Sewell 2010-01-25 21:58:00 EST
I disabled Haskell, the following package will now build on rawhide.

http://cloud.github.com/downloads/silas/rpms/thrift-0.2-0.6.20091112svn835538.fc13.src.rpm

rpmlint

[silas@hub rpmbuild]$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
thrift.src: E: specfile-error sh: ruby: command not found
thrift.src: E: specfile-error sh: ruby: command not found
thrift.src: E: specfile-error sh: ruby: command not found
thrift.src: E: specfile-error sh: ruby: command not found
thrift.src: E: specfile-error sh: ruby: command not found
thrift-erlang.x86_64: W: only-non-binary-in-usr-lib
12 packages and 0 specfiles checked; 5 errors, 1 warnings.

I have no idea why rpmlint is complaining about ruby, the related issues I've found have to do with ruby no being included in the buildrequires which is not the case for this package.

Otherwise the erlang warning is explained in the original description.
Comment 13 Silas Sewell 2010-03-02 14:21:06 EST
Update to release build.

Diff: http://github.com/silas/rpms/commit/8cc59c146ac7c09f4ca538ef2ee662841d6f6303

SRPM: http://cloud.github.com/downloads/silas/rpms/thrift-0.2.0-1.fc12.src.rpm

I did test builds for fedora-rawhide-x86_64, fedora-rawhide-i386, fedora-12-x86_64 and epel-5-x86_64.

rpmlint

thrift.src: E: specfile-error sh: ruby: command not found
 - still don't know how to fix this
thrift.x86_64: W: spelling-error Summary(en_US) multi -> mulch, mufti
thrift.x86_64: W: spelling-error %description -l en_US scalable -> salable, scalawag, scalar
thrift.x86_64: W: spelling-error %description -l en_US Smalltalk -> Small talk, Small-talk, Smallness
thrift.x86_64: W: spelling-error %description -l en_US Erlang -> Overland, Tamerlane, Melange
thrift.x86_64: W: spelling-error %description -l en_US Caml -> Cal, Camel, Carl
thrift.x86_64: W: spelling-error %description -l en_US Haskell -> Harrell, Rathskeller, Hastily
 - fine
thrift-erlang.x86_64: W: only-non-binary-in-usr-lib
 - see above
Comment 14 Jason Tibbitts 2010-11-02 09:17:38 EDT
I'm looking over old review tickets.  This one builds OK, but the built packages do not install.  It looks like thrift-perl has a dependency on perl(Thrift) but nothing provides it:

Error: Package: thrift-perl-0.2.0-1.fc14.x86_64 (/thrift-perl-0.2.0-1.fc14.x86_64)
           Requires: perl(Thrift)
Comment 15 Jason Tibbitts 2010-11-05 15:08:10 EDT
I'm going to go ahead and mark this as not being ready for review; please clear the whiteboard if the above issue is fixed.
Comment 16 Silas Sewell 2010-11-19 16:47:38 EST
Updated to Thrift 0.5.0 and fixed Perl issue.

Spec:
http://github.com/silas/rpms/raw/master/thrift/thrift.spec

SRPM:
https://github.com/downloads/silas/rpms/thrift-0.5.0-1.fc13.src.rpm
Comment 17 Jason Tibbitts 2010-11-19 18:44:25 EST
OK, this builds and installs fine.  This is going to be somewhat arduous as this touches most of the packaging guidelines we have.  I think I'll take it piece by piece and I won't try to do it all at once, because I know my browser will crash or my power will go out and I'll lose the whole thing.

First let's look at rpmlint:

  thrift.x86_64: W: no-manual-page-for-binary thrift
It's nice to have manpages for things when possible but it's not essential.

  thrift-erlang.x86_64: W: only-non-binary-in-usr-lib
This is pretty normal for erlang packages.

  thrift-cpp.x86_64: W: shared-lib-calls-exit
   /usr/lib64/libthriftnb.so.0.0.0 exit@GLIBC_2.2.5
Generally it's a bad idea for libraries to call exit themselves, but this is something to be reported upstream, not fixed in Fedora packages.

  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 _ZN6apache6thrift12GlobalOutputE
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 inflateEnd
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 deflate
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 deflateInit_
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 inflate
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 deflateEnd
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftz.so.0.0.0 inflateInit_
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 _ZN6apache6thrift12GlobalOutputE
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 _ZTVN6apache6thrift9transport13TMemoryBufferE
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 _ZN6apache6thrift7TOutput6printfEPKcz
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_get_version
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_set
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_base_free
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0
   _ZN6apache6thrift9transport13TMemoryBuffer10wroteBytesEj
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_get_method
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_del
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_add
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_init
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_base_set
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 event_base_loop
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0 _ZN6apache6thrift7TOutput6perrorEPKci
  thrift-cpp.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libthriftnb.so.0.0.0
   _ZN6apache6thrift9transport13TMemoryBuffer14ensureCanWriteEj
I know nothing of how these libraries are supposed to be used, but this forces whatever uses these libraries to also link with whatever provides these symbols even if the app doesn't call anything in that library.

  thrift-cpp.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libthriftz.so.0.0.0 /lib64/librt.so.1
  thrift-cpp.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libthriftz.so.0.0.0 /lib64/libm.so.6
  thrift-cpp.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libthrift.so.0.0.0 /lib64/libm.so.6
  thrift-cpp.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libthriftnb.so.0.0.0 /lib64/librt.so.1
  thrift-cpp.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libthriftnb.so.0.0.0 /lib64/libm.so.6
This is just things being linked against libraries that aren't actually called.  Not a big problem as long as those libraries would be in memory anyway.

So, nothing there that really must be fixed, although the undefined-non-weak-symbol stuff could probably be patched without too much effort.

I'll run through my checklist in a bit.
Comment 18 Jason Tibbitts 2010-11-22 12:48:55 EST
Well, I decided to take the weekend off.  Next thing to do here is look at the licensing situation.  And it looks, according to the LICENSE file, rather unpleasant.  (I hope you do realize that it's your job to look at this licensing stuff and comment about it when you submit your package, instead of expecting me to do all the work.)

The bulk is, obviously, ASL 2.0.  But there's a bunch of differently-licensed stuff included in the source tarball, and someone needs to check to see if any of that makes it into the final package.  Fortunately the LICENSE file indicates everything that's differently-licensed, and as far as I can tell it's correct.

The erlang automake file lib/erl/src/Makefile.am is supposedly MIT but is build infrastructure and I'd expect that it wouldn't be in the final package.  However, for whatever reason, your package includes it, along with the source code for the module.  Can you explain why it would do that?  I looked through some other erlang packages and they don't seem to do that.  You'd certainly need to list the license of all included files if you actually want to include them.

The stuff that was licensed from the old license to ASL 2.0 shouldn't be of any concern.

The md5.[ch] stuff is a license I haven't seen before.  I verified that this is built into the final thrift binary, so the license will need to be evaluated for ASL 2.0 compatibility and perhaps accounted for in the License: tag.  For legal, here's the license in question:

-----
  Copyright (C) 1999, 2000, 2002 Aladdin Enterprises.  All rights reserved.

  This software is provided 'as-is', without any express or implied
  warranty.  In no event will the authors be held liable for any damages
  arising from the use of this software.

  Permission is granted to anyone to use this software for any purpose,
  including commercial applications, and to alter it and redistribute it
  freely, subject to the following restrictions:

  1. The origin of this software must not be misrepresented; you must not
     claim that you wrote the original software. If you use this software
     in a product, an acknowledgment in the product documentation would be
     appreciated but is not required.
  2. Altered source versions must be plainly marked as such, and must not be
     misrepresented as being the original software.
  3. This notice may not be removed or altered from any source distribution.

  L. Peter Deutsch
  ghost@aladdin.com
----

lib/rb/setup.rb is LGPLv2+, but is build infrastructure and is not included in the final package.

lib/ocaml/OCamlMakefile doesn't appear to have a license (the Thrift project says LGPLv2+) but it's build infrastructure and is not included in the final package.

I went to check bug 446993 to see if any of this had been covered before, but that seems to be a completely unrelated review ticket (for liblicense).
Comment 19 Silas Sewell 2010-11-22 13:26:30 EST
Sorry, this was one the first couple of packages I created and I was a bit overwhelmed by its scope (I did it for a company I no longer work for).

I'll do a second take on all the language-specific/LICENSE stuff and see what I can fix.

Here is the original review request, it didn't cover most of the subpackages and didn't get very far:
https://bugzilla.redhat.com/show_bug.cgi?id=456941
Comment 20 Jason Tibbitts 2010-11-22 13:55:40 EST
Yeah, that old ticket didn't get anywhere at all.  I just wanted to see if there was any coverage of the license issue there; obviously there wasn't.

There are really only two licensing issues I saw: the erlang stuff is probably including too much and pulling in differently-licensed build infrastructure unnecessarily, and the md5.[ch] thing which is going to have to wait for someone with the legal group to look.
Comment 21 Silas Sewell 2010-12-29 23:10:16 EST
I guess I don't have the time or motivation to get this working.

Thanks for looking at it anyway Jason.
Comment 22 Steve Traylen 2011-03-24 15:29:48 EDT
I was just contemplating looking at this again though it's probably harder
than the need I have to justify it.

Some extra comments in case someone else chooses to tackle it.

The md5.[ch] files look to be a bundled

http://sourceforge.net/projects/libmd5-rfc/files/libmd5-rfc/2002-04-13/

moreover md5 looks to be already include in cups as well though I've
not checked if it is used.

cups-1.4.6/cups/md5.[ch]
Comment 23 Jason Tibbitts 2011-03-24 15:55:41 EDT
Do note that FPC has accepted two md5 variants as copylibs.  Of course, most packages that bundle them haven't been discovered and nobody's done a full audit, but bundling per-se isn't really a blocker.

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