Bug 749753 - Review Request: clxclient - a C++ X windows library
Summary: Review Request: clxclient - a C++ X windows library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 16
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jerry James
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 751466
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-10-28 08:53 UTC by Brendan Jones
Modified: 2011-12-24 20:51 UTC (History)
3 users (show)

Fixed In Version: clxclient-3.6.1-5.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-11-23 23:31:46 UTC
Type: ---
Embargoed:
loganjerry: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Brendan Jones 2011-10-28 08:53:45 UTC
clxclient is a C++ X windows library which currently resides in the CCRMA repositories.

I would like to move it into Fedora as it a requirement for two new audio packages zita-rev1 and zita-at1

SPEC: http://bsjones.fedorapeople.org/clxclient.spec
SRPM: http://bsjones.fedorapeople.org/clxclient-3.6.1-1.fc16.src.rpm

rpmlint ~/rpmbuild/SRPMS/clxclient-3.6.1-1.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint ~/rpmbuild/RPMS/x86_64/clxclient-*
clxclient.x86_64: W: shared-lib-calls-exit /usr/lib64/libclxclient.so.3.6.1 exit.5
clxclient-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

I think the shared-lib-calls-exit is intended in this case

Comment 1 Brendan Jones 2011-11-04 06:21:04 UTC
A few minor modifications.

SPEC: http://bsjones.fedorapeople.org/clxclient.spec
SRPM: http://bsjones.fedorapeople.org/clxclient-3.6.1-2.fc16.src.rpm

Comment 2 Jerry James 2011-11-04 19:12:36 UTC
I will take this review.

Comment 3 Jerry James 2011-11-04 20:55:50 UTC
The BuildRoot tag at the top of the spec file is unnecessary in all current versions of Fedora.  Also, the -devel subpackage should be declared "BuildArch: noarch", which also means removing the "%{?_isa}" from that subpackage's Requires of the main package.

Note that the rpmlint output below is from the installed packages, not the binary RPMs.  Some checks are disabled for the latter.

+: OK
-: must be fixed
=: should be fixed (at your discretion)
N: not applicable

MUST:
[+] rpmlint output:
clxclient.x86_64: W: undefined-non-weak-symbol /usr/lib64/libclxclient.so.3.6.1 P_thread::thr_start(int, int, unsigned long)
clxclient.x86_64: W: undefined-non-weak-symbol /usr/lib64/libclxclient.so.3.6.1 typeinfo for P_thread
clxclient.x86_64: W: undefined-non-weak-symbol /usr/lib64/libclxclient.so.3.6.1 P_thread::~P_thread()
clxclient.x86_64: W: undefined-non-weak-symbol /usr/lib64/libclxclient.so.3.6.1 ITC_ip1q::get_event(unsigned int)
clxclient.x86_64: W: undefined-non-weak-symbol /usr/lib64/libclxclient.so.3.6.1 P_thread::thr_start(int, int, unsigned long)
clxclient.x86_64: W: undefined-non-weak-symbol /usr/lib64/libclxclient.so.3.6.1 P_thread::P_thread()
clxclient.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libclxclient.so.3.6.1 /usr/lib64/libfreetype.so.6
clxclient.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libclxclient.so.3.6.1 /lib64/libm.so.6
clxclient.x86_64: W: shared-lib-calls-exit /usr/lib64/libclxclient.so.3.6.1 exit.5
clxclient-devel.x86_64: W: no-documentation
2 packages and 1 specfiles checked; 0 errors, 10 warnings.

This means that libclxclient.so.3.6.1 is NOT linked with -lclthreads but should be (hence the undefined non-weak symbols), and is linked with -lfreetype and -lm, but should not be.  This alteration of your sed command nearly fixes the issues:

# Force Fedora's flags and fix the link line
sed -e 's|-O2|%{optflags}|' \
    -e 's|/sbin/ldconfig|# /sbin/ldconfig|' \
    -e 's|`freetype-config --libs`||' \
    -e 's|-lpthread -lXft -lX11|-lclthreads -lXft -lX11|' \
    -i Makefile

This doesn't completely fix the problem, though, due to bug 751466.

[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license
[-] license field matches the actual license: actual license is LGPLv2+
[+] license file is included in %doc
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum is bd47f80a855d3203fcf10365e79d85e4 for both
[+] package builds on at least one primary arch (tried x86_64)
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[+] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[+] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[+] header files in -devel
[N] static libraries in -static
[+] .so in -devel
[+] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[N] query upstream for license text
[N] description and summary contain available translations
[+] package builds in mock: tried fedora-rawhide-i386
[+] package builds on all supported arches: tried i386 and x86_64
[+] package functions as described: no simple means of testing
[+] sane scriptlets
[+] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

Comment 4 Brendan Jones 2011-11-04 22:13:36 UTC
Thanks for the review!

I have made the changes you've suggested, and rebuilt clthreads in accordance with bug 751466.

SPEC: http://bsjones.fedorapeople.org/clxclient.spec
SRPM: http://bsjones.fedorapeople.org/clxclient-3.6.1-4.fc16.src.rpm

Comment 5 Jerry James 2011-11-04 22:26:39 UTC
Oh heck, I'm sorry, I made a mistake.  The -devel package should NOT be noarch.  It almost is, except for those .so files.  The embarrassing part is that I've made this mistake before.... (*blush*)  Take the noarch out and put the %{?_isa} back on -devel's Requires, like you had it in the first place, and this package is APPROVED.

Comment 6 Brendan Jones 2011-11-04 22:45:28 UTC
No problem (I missed it too ;) Thanks again for the review.

Comment 7 Brendan Jones 2011-11-04 22:47:54 UTC
New Package SCM Request
=======================
Package Name: clxclient
Short Description: a C++ X windows library
Owners: bsjones
Branches: rawhide f16 f15 f14
InitialCC:

Comment 8 Gwyn Ciesla 2011-11-06 20:39:37 UTC
Git done (by process-git-requests).

rawhide==f17==devel, and is automatically created, don't explicitly request
it.  Thanks!

Comment 9 Fedora Update System 2011-11-12 18:10:22 UTC
zita-rev1-0.2.1-4.fc16,clxclient-3.6.1-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/zita-rev1-0.2.1-4.fc16,clxclient-3.6.1-5.fc16

Comment 10 Fedora Update System 2011-11-13 05:37:46 UTC
zita-rev1-0.2.1-4.fc16, clxclient-3.6.1-5.fc16 has been pushed to the Fedora 16 testing repository.

Comment 11 Fedora Update System 2011-11-23 23:31:46 UTC
zita-rev1-0.2.1-4.fc16, clxclient-3.6.1-5.fc16 has been pushed to the Fedora 16 stable repository.

Comment 12 Fedora Update System 2011-12-13 22:48:47 UTC
clxclient-3.6.1-5.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/clxclient-3.6.1-5.fc15

Comment 13 Fedora Update System 2011-12-24 20:51:59 UTC
clxclient-3.6.1-5.fc15 has been pushed to the Fedora 15 stable repository.


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