Bug 201178 - Review Request: pinball - Emilia Pinball game
Summary: Review Request: pinball - Emilia Pinball game
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul F. Johnson
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-08-03 13:16 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-08-10 20:41:51 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Patch to allow ltdl to load from an so instead of an la file (1023 bytes, patch)
2006-08-10 00:41 UTC, Toshio Kuratomi
no flags Details | Diff
My spec file. (3.73 KB, application/x-extension-spec)
2006-08-10 00:43 UTC, Toshio Kuratomi
no flags Details

Description Hans de Goede 2006-08-03 13:16:41 UTC
Spec URL: http://people.atrpms.net/~hdegoede/pinball.spec
SRPM URL: http://people.atrpms.net/~hdegoede/pinball-0.3.1-3.src.rpm
Description:
The Emilia Pinball project is an open source pinball simulator for linux
and other unix systems. The current release is a stable and mature alpha.
There are only two pinball machines to play with, it is however very addictive.

Comment 1 Paul F. Johnson 2006-08-03 13:47:22 UTC
Requires:       %{name} = %{version}-%{release}

Add pkgconfig to this line

%files devel

Isn't there an .so file for this directory?



Comment 2 Hans de Goede 2006-08-03 13:51:23 UTC
(In reply to comment #1)
> Requires:       %{name} = %{version}-%{release}
> 
> Add pkgconfig to this line
> 

Erm, it doesn't install any pc files, so pkgconfig isn't needed.

> %files devel
> 
> Isn't there an .so file for this directory?
> 

Notice that it uses %{_libdir}/%{name} not just %{_libdir}, %{_libdir}/%{name}
contains plugins which get loaded by there full soname by ltdl, which finds this
name by looking at the .la files, so in essence the .la files replace the .so
files for the purpose of plugin loading.

The .so files also are not needed to link against, since this are not libs but
plugins. The -devel package is to develop new plugins, not to develop programs
using the existing ones, hence no .so files.



Comment 3 Paul F. Johnson 2006-08-03 13:54:21 UTC
Sorry, didn't spot those!

I'll get on reviewing the package as soon as my buildsys stops playing silly
buggers.

Comment 4 Paul F. Johnson 2006-08-03 23:02:49 UTC
Fails to build : you need to remove the smp_flags as it causes a race condition
                 LoaderModule.cpp has a lot of errors in - the majority of them
are the likes of lt_dlinit not declared in this scope.

I'm actually starting to thing this is gcc being overly strict as amaya has this
problem as well and I'm unable to see why it should be by examining the source

Comment 5 Hans de Goede 2006-08-04 06:43:39 UTC
(In reply to comment #4)
> Fails to build : you need to remove the smp_flags as it causes a race condition

Done, sorry I used to have it set to -j3 in my .rpmrc but I've changed it to -j1
because lately I've been packaging a few huge c++ apps and my machine gut real
sluggish with -j3 (I don't have a seperate builder like you do).

>                  LoaderModule.cpp has a lot of errors in - the majority of them
> are the likes of lt_dlinit not declared in this scope.
> 
> I'm actually starting to thing this is gcc being overly strict as amaya has this
> problem as well and I'm unable to see why it should be by examining the source

Are you sure it doesn't start with a missing header error before those, as |i
forgot to add a BR for libtool-ltdl-devel, which IMHO is the likely cause of this.

Here is a new version with smp_flags removed and the BR added:
Spec URL: http://people.atrpms.net/~hdegoede/pinball.spec
SRPM URL: http://people.atrpms.net/~hdegoede/pinball-0.3.1-4.src.rpm


Comment 6 Paul F. Johnson 2006-08-05 21:18:40 UTC
rpmlint warns that there is no documentation in the devel package

mock builds cleanly and rpm -qa --requires is not showing anything not caught in
the mock build

---
SPEC FILE
---

Good

Upstream version is the same, md5sums both check out
No dupes in the BR
No ownership problems
Software actually runs

Bad

%{_libdir}/%{name}/lib*.la

Why? There should be no .la or .a files in a package

Should /sbin/ldconfig not be also executed?

Comment 7 Hans de Goede 2006-08-09 12:10:28 UTC
(In reply to comment #6)
> Bad
> 
> %{_libdir}/%{name}/lib*.la
> 
> Why? There should be no .la or .a files in a package
> 

Quoting from the specfile:
"# .la files are needed for ltdl"

What is not clear about that?

> Should /sbin/ldconfig not be also executed?

Quoting myself from comment #3:

"The .so files also are not needed to link against, since this are not libs but
plugins. The -devel package is to develop new plugins, not to develop programs
using the existing ones, hence no .so files."

Since they are not used to link against directly calling ldconfig is not needed,
actually since they are not in a dir searched by ldconfig, calling ldconfig is
futile.


Comment 8 Paul F. Johnson 2006-08-09 21:41:04 UTC
Okay. I'm happy with them.

The software works fine

Good
Licence fine
Includes update-icon-cache
No permission problems
No directory ownership problems
Has documentation (devel doesn't, can be ignored)
Software works and installs
No missing R's when installed
No missing BRs or Rs when compiling

Bad
Missing rm -rf %{buildroot} at the start of %install

Fix the one bad and it's good to go.


Comment 9 Paul F. Johnson 2006-08-09 21:56:16 UTC
Can you also check that if you use the fedora libtool if the .la files are still
needed?

Please just upload the new spec file (unless something happens in the src.rpm)

Comment 10 Paul F. Johnson 2006-08-09 22:16:31 UTC
It may be worth actually patching the app to use the fedora libtool and seems
pretty trivial to do...

in LoaderModule.cpp, Line 86 change

lt_dlopen() to lt_dlopenext() and remove the extension on the passed in filename

There may be a couple more of those, so you'll need to grep the source.

Comment 11 Toshio Kuratomi 2006-08-10 00:41:37 UTC
Created attachment 133896 [details]
Patch to allow ltdl to load from an so instead of an la file

Hey Hans, Here's a patch to load the modules from the .so file if the .la file
is not present.  This patch would be a good starting point with upstream but
may need a little work (I'm not sure whether the fallback lt_dlopen() is
necessary and my C++ dates from before the STL so you can tell how rusty I am.)
 I'll submit my spec file as well so you can diff for the changes.

Comment 12 Toshio Kuratomi 2006-08-10 00:43:29 UTC
Created attachment 133897 [details]
My spec file.

Patch the source.
Include the .so file and rm the .la file.

P.S. I suck at pinball :-)

Comment 13 Hans de Goede 2006-08-10 09:40:01 UTC
(In reply to comment #8)
> Bad
> Missing rm -rf %{buildroot} at the start of %install
> 

Good catch! New version with this fixed (only specfile changed):
Spec URL: http://people.atrpms.net/~hdegoede/pinball.spec
SRPM URL: http://people.atrpms.net/~hdegoede/pinball-0.3.1-5.src.rpm


(In reply to comment #9)
> Can you also check that if you use the fedora libtool if the .la files are
> still needed?
Yes they are still needed.


(In reply to comment #10)
> It may be worth actually patching the app to use the fedora libtool and seems
> pretty trivial to do...
> 

It already is using the Fedora/system libtool, quoting from %setup:
"rm -fr libltdl", so there is no way its using its own version :)


> in LoaderModule.cpp, Line 86 change
> 
> lt_dlopen() to lt_dlopenext() and remove the extension on the passed in filename
> 
> There may be a couple more of those, so you'll need to grep the source.

That is not using the Fedora libtool, that is patching arounds libltdl's
behaviour / default use to open the .la files. All kde apps with plugings (as in
Fedora) do this, and I see no reason why pinball shouldn't/couldn't there is no
reason to deviate from upstream here / use ltdl in a non standard way here.

.la files are evil / must be removed for normal libraries which are intended to
link against, because when an application gets build using libtool and those .la
files are there libtool will prefer the .la files above the .so files and will
use the often broken dependencies in those .la files instead of the soname deps
in the .so files (which get tracked by rpm). This is known as libtool dependency
hell, but this only happens with libraries which are intented to link against by
other apps and which thus are usually installed directly into %_libdir and not
into a subdir of it as the *plugins* are. So there is no reason to start
patching and deviating from upstream here, the .la files are harmless because
they are for plugins and beside harmless also needed as this is the default mode
of operation of ltdl. If you don't like this file a bug against ltdl and the
zillion of kde packages doing the same.

(In reply to comment #11)
> Created an attachment (id=133896) [edit]
> Patch to allow ltdl to load from an so instead of an la file
> 
> Hey Hans, Here's a patch to load the modules from the .so file if the .la file
> is not present.  This patch would be a good starting point with upstream but
> may need a little work (I'm not sure whether the fallback lt_dlopen() is
> necessary and my C++ dates from before the STL so you can tell how rusty I am.)
>  I'll submit my spec file as well so you can diff for the changes.

Thanks,  but no thanks see above for the reasoning.


Comment 14 Rex Dieter 2006-08-10 20:00:40 UTC
the .la files here should be ok as-is.

Comment 15 Paul F. Johnson 2006-08-10 20:01:07 UTC
With the change to the spec and the explanations here (and on IRC), I'm happy.

APPROVED

Comment 16 Rex Dieter 2006-08-10 20:02:22 UTC
Not sure why you're removing lib*.so, but I'm sure you have a good reason. (:

Comment 17 Hans de Goede 2006-08-10 20:05:58 UTC
(In reply to comment #15)
> With the change to the spec and the explanations here (and on IRC), I'm happy.
> 
> APPROVED

Thanks!

(In reply to comment #16)
> Not sure why you're removing lib*.so, but I'm sure you have a good reason. (:

The .la files which are used / openened by pinball refer to the .so.x files, not
to the .so ones, so for loading the plugins they are not nescesarry, since these
 .so files are plugins and libs they are not meant to be linked against either,
so the .so files aren't nescesarry for that either. Thus they are completly
unused, hence I remove them. I must say I didn't look what other ltdl using
packages do with the .so files.


Comment 18 Hans de Goede 2006-08-10 20:41:51 UTC
Imported and build, closing.



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