Bug 177117 - Review Request: libtlen - Tlen.pl client library
Review Request: libtlen - Tlen.pl client library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-06 09:25 EST by Dominik 'Rathann' Mierzejewski
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-08-09 15:07:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
patch to spec file (2.87 KB, patch)
2006-01-06 18:05 EST, Dawid Gajownik
no flags Details | Diff

  None (edit)
Description Dominik 'Rathann' Mierzejewski 2006-01-06 09:25:07 EST
Spec: http://rpm.greysector.net/extras/libtlen.spec
SRPM: http://rpm.greysector.net/extras/libtlen-0-0.1.20041113.src.rpm
Description:
libtlen is a library providing an API for client programs which want
to use Tlen.pl, an Instant Messanging protocol based on Jabber, but
with some modifications.
Comment 1 Dawid Gajownik 2006-01-06 18:05:20 EST
Created attachment 122897 [details]
patch to spec file

It seems to be your first package in FE, so only your sponsor can make an
official review of this RPM. I can only point out some problems with the spec
file.

- wrong “Group” tag. Should be “System Environment/Libraries”
http://fedoraproject.org/wiki/RPMGroups
- you can use “%{?dist}” in “Release” tag. It make life much easier :)
http://fedoraproject.org/wiki/DistTag#head-6853fe5b797b146350e0d6b4caf57d517504449f

- remove “Epoch” tag - this package was never available in FC or FE
- superflous BR. You can delete:

BuildRequires:	autoconf
BuildRequires:	automake
BuildRequires:	libstdc++-devel

libstdc++-devel is a dependency of gcc-c++
http://fedoraproject.org/wiki/PackagingGuidelines#Exceptions
- wrong “BuildRoot”
http://fedoraproject.org/wiki/PackagingGuidelines#BuildRoot
- in “devel” subpackage summary:
s/developping/developing/
- use parallel make:
http://fedoraproject.org/wiki/PackagingGuidelines#parallelmake
- kill static libs. They are evil from the security point of view ;]
http://fedoraproject.org/wiki/PackagingGuidelines#StaticLibraries
http://people.redhat.com/drepper/no_static_linking.html
- please provide css file in devel subpackage
- change encoding of %doc files to UTF-8 (like in man-pages-pl or vim SRPMs)

Proposal changes are in attached patch. I have also modified order of tags so
now they are like in /usr/share/fedora/spectemplate-minimal.spec from
fedora-rpmdevtools package. I also changed $RPM_BUILD_ROOT to %{buildroot}
because its shorter. If you don't like these to changes, you can revert them ;)

http://fedoraproject.org/wiki/PackagingGuidelines#UsingBuildRootOptFlags

rpmlint warnings which should be corrected:

[rpm-build@X i386]$ rpmlint libtlen-0-0.1.20041113.i386.rpm
W: libtlen no-version-in-last-changelog
[rpm-build@X i386]$ rpmlint libtlen-devel-0-0.1.20041113.i386.rpm
W: libtlen-devel no-version-in-last-changelog
[rpm-build@X i386]$

It would be nice if you could contact with upstream and force them to include
license in tarball ;)

Package compiles with lots of warnings. Developers screwed the job in
lib/libtlen.h:452 and  lib/libtlen.h:457. They check some macros but they do
not set them in config.h via configure script :D
Comment 2 Dominik 'Rathann' Mierzejewski 2006-01-07 11:21:16 EST
Fixed, but what's the point of adding %{dist} to Release?
Comment 3 Dawid Gajownik 2006-01-07 12:41:12 EST
(In reply to comment #2)
> Fixed

It does not build now :/ rpmbuild does not recognize “%doc(pl)” macro on my
machine (fully updated FC4 and Rawhide box):

Processing files: libtlen-0-0.1.20041113
error: File must begin with "/": %doc(pl)
error: File must begin with "/": docs/AUTHORS
error: File must begin with "/": docs/TODO
error: File must begin with "/": ChangeLog
Processing files: libtlen-devel-0-0.1.20041113
error: File must begin with "/": %doc(pl)
error: File must begin with "/": docs/*.{html,css}
Processing files: libtlen-debuginfo-0-0.1.20041113
Provides: libtlen.so.1.5.debug
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1


RPM build errors:
    user dominik does not exist - using root
    user dominik does not exist - using root
    File must begin with "/": %doc(pl)
    File must begin with "/": docs/AUTHORS
    File must begin with "/": docs/TODO
    File must begin with "/": ChangeLog
    File must begin with "/": %doc(pl)
    File must begin with "/": docs/*.{html,css}
[rpm-build@X SPECS]$

Aghh, I forgot about ChangeLog file. It has wrong encoding. It should be also fixed.

> but what's the point of adding %{dist} to Release?

It's not mandatory but it makes life of developer much easier (at least for me).
You can have the same spec file for FC-3, FC-4 and devel branch and it makes
very clear which distribution package was built for. It can also help a bit in
upgrade path from FC(n) to FC(n+1).
You can take a look at this thread →
http://www.redhat.com/archives/fedora-extras-list/2006-January/msg00334.html
Comment 4 Dominik 'Rathann' Mierzejewski 2006-01-07 13:08:11 EST
Doh! It should've been %lang(pl) %doc, fixed.
Comment 5 Michael Schwendt 2006-05-29 07:23:06 EDT
Dropping FE-NEEDSPONSOR. Tom Callaway offered sponsorship in bug 177235
(and bugzilla change-several-bugs-at-once feature requires me to add/edit
something).
Comment 6 Michael Schwendt 2006-05-29 07:28:30 EDT
uhm, bugzilla is broken :(
Comment 7 Jason Tibbitts 2006-06-15 21:47:42 EDT
I know this is an old ticket; is there still interest in getting it in?  I'll be
happy to review it if so.
Comment 8 Jason Tibbitts 2006-07-26 15:44:22 EDT
I will close this bug if there is no response soon.
Comment 9 Dominik 'Rathann' Mierzejewski 2006-07-28 12:48:57 EDT
Is there anything wrong with the current submission?
Comment 10 Jason Tibbitts 2006-07-28 12:55:50 EDT
I haven't reviewed it yet.  I just wanted to make sure that my time wouldn't be
wasted, given that it had been six weeks since my original review offer with no
response.

I'll go ahead and review it now.
Comment 11 Jason Tibbitts 2006-07-28 14:06:17 EDT
You've said on IRC that you'll add the dist tag, so no worries there.

The major issue I see is with the license.  The only file I see that actually
has a license statement is lib/asciitab.h which is GPL/MPL.  The only thing that
says LGPL is the sourceforge page, which unfortunately really isn't sufficient.
 Perhaps I'm just not looking in the proper place, though; am I missing a
license statement somewhere?

* source files match upstream:
   b77c0a3234a21d1b79df8a8b9a9b9534  libtlen-20041113.tar.gz
* package meets naming and packaging guidelines.
* Prerelease snapshot naming is correct: 0-0.1.20041113
* specfile is properly named, is cleanly written and uses macros consistently.
X dist tag is not present.
* build root is correct.
? license field matches the actual license.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
  libtlen-0-0.1.20041113.x86_64.rpm
   libtlen.so.1()(64bit)
   libtlen = 0-0.1.20041113
  =
   /sbin/ldconfig
   libtlen.so.1()(64bit)

  libtlen-devel-0-0.1.20041113.x86_64.rpm
   libtlen-devel = 0-0.1.20041113
  =
   libtlen = 0-0.1.20041113
   libtlen.so.1()(64bit)

* %check is not present; no test suite upstream.
* shared libraries are present; ldconfig is called as necessary and unversioned
.so links are in the -devel package.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* 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 libtool .la droppings.
Comment 12 Dominik 'Rathann' Mierzejewski 2006-07-28 14:09:49 EDT
http://rpm.greysector.net/extras/libtlen.spec
http://rpm.greysector.net/extras/libtlen-0-0.2.20041113.src.rpm

added dist tag and a patch to fix 64bit builds
Comment 13 Dominik 'Rathann' Mierzejewski 2006-07-28 14:10:48 EDT
or was it to fix 32bit build on 64bit fedora...
Comment 14 Dominik 'Rathann' Mierzejewski 2006-07-28 14:56:16 EDT
Yes, that was it. In the meantime I've found a newer release on
http://tleenx.sf.net/ and updated once again. ;)
Also, I sent an e-mail to the developers about the license.

http://rpm.greysector.net/extras/libtlen.spec
http://rpm.greysector.net/extras/libtlen-0-0.3.20060309.src.rpm
Comment 15 Dominik 'Rathann' Mierzejewski 2006-08-07 15:03:48 EDT
Got a response from upstream. They've included the license text (LGPL) inside.

http://rpm.greysector.net/extras/libtlen.spec
http://rpm.greysector.net/extras/libtlen-0-0.4.20060309.src.rpm
Comment 16 Jason Tibbitts 2006-08-08 23:22:43 EDT
OK, the updated package builds fine and still looks OK.  The dist tag is there,
as is the license file.

APPROVED
Comment 17 Dominik 'Rathann' Mierzejewski 2006-08-09 06:43:28 EDT
Package imported, devel build successful, FC-4 and FC-5 branches requested.
Comment 18 Dominik 'Rathann' Mierzejewski 2006-08-09 17:48:50 EDT
Branches built successfully.

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