Bug 495692 - Review Request: tslib - Touchscreen Access Library
Review Request: tslib - Touchscreen Access Library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-14 08:47 EDT by Nicolas Chauvet (kwizart)
Modified: 2009-06-11 09:51 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-11 09:51:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nicolas Chauvet (kwizart) 2009-04-14 08:47:27 EDT
Spec URL:
http://kwizart.fedorapeople.org/SPECS/tslib.spec
SRPM URL:
http://kwizart.fedorapeople.org/SRPMS/tslib-1.0-1.fc11.src.rpm
Description: Touchscreen Access Library

rpmlint, no doc in -devel subpackage. (none are provided wrt -devel).
rpmlint on installed package is clean

build tested with mock for rawhide x86_64.
Comment 1 Michael Schwendt 2009-05-05 11:22:19 EDT
Since Fedora 11 one can require an arch-specific pkgconfig with:

%if 0%{?fedora} > 10
Requires:       pkgconfig%{_isa}
%else
Requires:       pkgconfig
%endif

This is more accurate on multi-arch platforms.
Comment 2 Nicolas Chauvet (kwizart) 2009-05-07 07:00:50 EDT
This is true, depsite It could have been handled with the rpm dependency extractor. (along with the pkgconfig provides). In Theory, when a .pc file is provided in /usr/lib64/pkgconfig it should already requires pkgconfig.x86_64 which is the only one to own such directory. 
At least this is what I would expect.

Do we have corner case where this is known to produce issue?
Comment 3 Michael Schwendt 2009-05-07 09:28:23 EDT
There is no automatic dependency on pkgconfig. The pkgconfigdeps.sh script only evaluates any .pc file's internal dependencies and adds corresponding "pkgconfig(foo)" RPM Requires/Provides.
Comment 4 Nicolas Chauvet (kwizart) 2009-05-07 09:51:33 EDT
This is right, and there is no isa dependency either.

But after all, Fedora isn't a multiarch distro ( it is a multilibs), one could say that pkg-config.i386 shouldn't be provided within the x86_64 repository. And /usr/lib{64}/pkgconfig should be provided by the filesystem instead.
Comment 5 Michael Schwendt 2009-05-07 10:55:50 EDT
In F11 Rawhide I see automatic dependencies on /usr/bin/pkg-config - ouch! [One can only hope that createrepo has moved the Provides into the primary metadata, too.]

pkgconfig.i386 is not published in the x86_64 repo. At least not in the F10 final tree and not in F11 Rawhide either. Without looking into it in detail, I can't tell what provides /usr/lib/pkgconfig for the 64-bit targets. If nothing did, it would be a broken dependency.

By default, the old "Requires: pkgconfig" is fine. It's just 1) that some packagers start adding "Requires: %{_libdir}/pkgconfig" instead (see e.g. bug 484849), and 2) you need the arch-specific binary for multi-arch development, since pkgconfig looks in $libdir/pkgconfig ... so where $libdir=/usr/lib64, it doesn't look in /usr/lib/pkgconfig, does it? And if one would set $PKG_CONFIG_PATH, would it handle multilib $libdir correctly?
Comment 6 Michael Schwendt 2009-05-07 11:02:44 EDT
At least openchange-devel includes %{_libdir}/pkgconfig -- going to file a bug now.
Comment 7 Nicolas Chauvet (kwizart) 2009-05-12 20:04:59 EDT
Any others thoughts about this package ?
Comment 8 Michael Schwendt 2009-06-08 07:52:57 EDT
APPROVED

[...]

Some observations, though:

* Version 1.0 of tslib uses version 0.0 in its SONAME and in the pkg-config file (libts-0.0.pc). Especially the latter is a questionable decision, because the pkg-config file doesn't do anything special and only returns the non-versioned -lts via --libs. There's no reason why upstream could not have named the file "libts.pc" then. Pkg-config can evaluate the internal "Version" field to requires specific versions or version-ranges.

* The modules/plugins pollute the automatic RPM Provides (and hence the metadata) with their *.so names:

$ rpm -qp --provides tslib-1.0-1.fc10.i386.rpm|grep -e so[^.]
arctic2.so  
collie.so  
corgi.so  
dejitter.so  
h3600.so  
input.so  
linear.so  
linear_h2200.so  
mk712.so  
pthres.so  
ucb1x00.so  
variance.so  

Harmless, since nothing ought to depend on such symbols, but it's pollution nevertheless.

* Asking upstream to run autogen.sh prior to creating the source tarball would be helpful.
Comment 9 Nicolas Chauvet (kwizart) 2009-06-10 05:46:38 EDT
(In reply to comment #8)
> APPROVED
> 
> [...]
> 
> Some observations, though:
And again, some very good comments.

> * Version 1.0 of tslib uses version 0.0 in its SONAME and in the pkg-config
Indeed, but until they change the API without bumping the release field, we can still assure they are using it right despite it could have been done better.
 
> * The modules/plugins pollute the automatic RPM Provides (and hence the
> metadata) with their *.so names:
> 
> $ rpm -qp --provides tslib-1.0-1.fc10.i386.rpm|grep -e so[^.]
It should be possible for the rpm dependency extractor to filter them as soon as it discovers that they are module to be used with dlopen ?
It would be wondefull.

> * Asking upstream to run autogen.sh prior to creating the source tarball would
> be helpful.  
Yep, despite it could lead to problem once the sources are generated with a version of libtool patched in an incompatible way. (experienced with vlc in F-11: a binary failed to link with a just built library unless rpath as used at build time which I've explicitely forbidden).
Comment 10 Nicolas Chauvet (kwizart) 2009-06-10 05:47:35 EDT
New Package CVS Request
=======================
Package Name: tslib
Short Description: Touchscreen Access Library
Owners: kwizart
Branches: devel F-11 F-10 EL-5
Cvsextras Commits: yes
Comment 11 Jason Tibbitts 2009-06-10 16:54:15 EDT
CVS done.

BTW, we pay no attention to "Cvsextras Commits" these days.
Comment 12 Nicolas Chauvet (kwizart) 2009-06-11 09:51:32 EDT
Thx for the review

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