Bug 199491

Summary: Review Request: dbus-sharp
Product: [Fedora] Fedora Reporter: John (J5) Palmieri <johnp>
Component: Package ReviewAssignee: Paul F. Johnson <paul>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, jkeck, jpmahowald, wtogami
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-07-21 06:05:39 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 188268    

Description John (J5) Palmieri 2006-07-19 16:42:16 EDT
Spec URL: http://people.redhat.com/johnp/files/dbus/dbus-sharp.spec
SRPM URL: http://people.redhat.com/johnp/files/dbus/dbus-sharp-0.63-1.src.rpm
Description: Mono bindings for dbus.  The Source is not a full URL because the mono bindings are unmaintained and I just made a quick build enviornment in the git repo.  Unfortunatly if we ship mono we need to include this or all the interesting apps won't build or install.  I specificly did not do a release because I want the mono devs to fix the situation in the future.
Comment 1 Paul F. Johnson 2006-07-19 16:57:26 EDT
Okay, couple of comments on the install section of the spec file

# dbus installs mono files in libdir, not in prefix/lib, fixup:
perl -pi -e 's,/gacdir \$\(libdir\),/gacdir /usr/lib,g' mono/Makefile
perl -pi -e "s,/root \\\$\\(DESTDIR\\)\\\$\(libdir\\),/root
$RPM_BUILD_ROOT/usr/lib,g" mono/Makefile
perl -pi -e "s,/usr/lib64,/usr/lib,g" dbus-sharp.pc
mkdir -p $RPM_BUILD_ROOT%{_prefix}/lib/mono


I'm not sure here. If the files are being installed in %_libdir rather than
%{_prefix}/lib, why have you got the mkdir -p line? When you run the make
install, the directories are created for you by the makefile (unless in this
case, they're not of course!)

The other problem is that the .pc file is going into %_libdir/pkgconfig. I've
seen this on 64 bit architecture failing miserably, but not if it's in
%_prefix/lib (yes, I am aware of the packaging guidelines!)

The .pc file should be in a devel package by itself. Should the Source0 not
point to a URL rather than on a local machine?

You're missing the %{?dist} tag on the Release line

Finally, is the define arch required?
Comment 2 John (J5) Palmieri 2006-07-19 17:15:41 EDT

On the first point - this is just something I copied from the old dbus package.
 I assume on 64bit archs everything is still installed into /usr/lib but the pc
file needs to be installed into /usr/lib64.  I would ask Alex who packaged Mono
but he is on vacation right now.

On the second point %_libdir/pkgconfig is the correct place to put pc files.

Third point (.pc in devel) I'll fix that.

On the fourth point as I said in the initial comment, there is no URL because
this package has not been released.  It is a streight build from the git repo. 
If it were up to me wouldn't even be packaging this but mono apps need it for now.

Fifth point (dist tag) I'll add it

On the sixth point I just picked that up from the old spec file.  Does that
define all of our arch's?  Not sure why it was still there.

Fouth point (.pc in devel) I'll fix that.
Comment 3 John (J5) Palmieri 2006-07-19 17:18:36 EDT
Oh wait, ppc64 isn't listed.  There is most likely problems building mono on
that arch.
Comment 4 Paul F. Johnson 2006-07-19 17:24:22 EDT
On the forth point, could the tarball not have been uploaded to your redhat
space and Source0 become Source1 with Source0 becoming a link to your webspace
(or some comment made saying it's a grab from git). As it stands, I'm not sure
it complies with the packaging guidelines.

Second point : I know %_libdir/pkgconfig is the correct place for .pc files.
However, from my experience of building monodevelop (and as a general rule for
mono, any package relying on another mono lib), .pc files on 64 bit
architectures tend not to be picked up when you run the configure file. There is
an easy fix, just add %libdir/pkgconfig to the pkgconfig PATH variable.

Sixth point : Again, when I've created packages for FE on mono, the buildarchs
are not usually included in the spec file, so I'm not sure as to their value
Comment 5 Jesse Keating 2006-07-19 17:28:01 EDT
Source doesn't always have to be a URL.  Especially if its pre-release
snapshotting.  Also for RH projects, our "preferred" method of distribution is
directly in srpms so there are many projects that don't have tarball releases,
so the srpm referrs to itself.

(I'll step out of the way of the rest of this review unless needed)
Comment 6 Paul F. Johnson 2006-07-19 17:29:23 EDT
Thanks for that clarification :-)
Comment 7 Peter Gordon 2006-07-19 17:33:11 EDT
*** Bug 199490 has been marked as a duplicate of this bug. ***
Comment 8 John (J5) Palmieri 2006-07-19 17:39:25 EDT
new package and spec uploaded

Spec URL: http://people.redhat.com/johnp/files/dbus/dbus-sharp.spec
SRPM URL: http://people.redhat.com/johnp/files/dbus/dbus-sharp-0.63-2.src.rpm

I kept the libdir the same just because this is how it has worked in the past
and when alex gets back he can fix it up if need be.  I need to drop an intern
off at home.  I'll check back later and build if it is approved.
Comment 9 Paul F. Johnson 2006-07-19 17:56:41 EDT
That's the old spec file and the src rpm is giving a 404
Comment 10 Paul F. Johnson 2006-07-20 07:18:32 EDT
I've noticed that one of todays rawhide updates is relying on dbus-sharp. How
many applications currently in core also rely on dbus-sharp?
Comment 11 John (J5) Palmieri 2006-07-20 11:45:56 EDT
Woops, I uploaded them but forgot to move them to the webdir, moved now. 
Currently f-spot and tomboy use it.
Comment 12 Warren Togami 2006-07-20 13:31:57 EDT
Name: dbus-sharp
Version: 0.63
Obsoletes: dbus-sharp < 0.63

I think this Obsoletes is unnecessary, am I wrong?

Otherwise everything else looks OK.  APPROVED
Comment 13 Warren Togami 2006-07-20 13:37:10 EDT
Note, things are not perfect here but good enough.  I am pushing this through in
order to fix rawhide a day sooner.  Please submit more spec improvements here if
you think it should be improved further.
Comment 14 John (J5) Palmieri 2006-07-20 13:37:48 EDT
The Obsoletes is a precaution since we are moving from a subpackage to an
actuall package.
Comment 15 Warren Togami 2006-07-20 15:07:48 EDT
I'm pretty sure it doesn't work this way, and there is no actual problem here.
Comment 16 Paul F. Johnson 2006-07-20 15:49:26 EDT

Requires: %name = %{version}-%{release}

Should this not be %{name}?

Can the spec unify how it uses $RPM_BUILD_ROOT? It should be either entirely
%{buildroot} or $RPM_BUILD_ROOT but not a mix of the two.

Also, why is the mkdir -p %{_prefix}/lib/mono in the install? The make install
step creates it.

Comment 17 Ralf Corsepius 2006-07-21 05:06:34 EDT
(In reply to comment #16)
> Requires: %name = %{version}-%{release}
> Should this not be %{name}?
Not necessarily. It should not make a different.

%{name} is the quoted version of %name. 
This is similar to ${var} vs. $var in /bin/sh.
Comment 18 Paul F. Johnson 2006-07-21 06:05:39 EDT
Okay, thanks for that clarification. I was just making sure after the problem I
had with NAnt recently.