Bug 245491 (ndesk-dbus) - Review Request: ndesk-dbus - Managed DBus implementation
Summary: Review Request: ndesk-dbus - Managed DBus implementation
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: ndesk-dbus
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Peter Gordon
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: ndesk-dbus-glib
TreeView+ depends on / blocked
 
Reported: 2007-06-24 08:42 UTC by David Nielsen
Modified: 2007-11-30 22:12 UTC (History)
3 users (show)

Fixed In Version: ndesk-dbus-0.5.2-12.fc9
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-10-31 16:57:47 UTC
Type: ---
Embargoed:
peter: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description David Nielsen 2007-06-24 08:42:05 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-1.fc8.src.rpm
Description: ndesk-dbus is a C# implementation of D-Bus. It's often referred to as
"managed D-Bus" to avoid confusion with existing bindings (which wrap
libdbus).

This is required to make DBus communication work for many Mono apps, such as Banshee where the outdated dbus-sharp currently prevents correct functioning.

*Note* this does not currently build in Mock, please consider pointing out my mistake to me.

Comment 1 David Nielsen 2007-06-24 23:01:59 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-2.fc8.src.rpm

Okay small thinko, .mdb files aren't MonoDevelop project files and thus don't go
in -devel.

Comment 2 David Nielsen 2007-06-25 01:23:18 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-3.fc8.src.rpm

Significantly less hacky, now builds in a F-7 x86_64 mock here (but strangely
still complains about gmcs being AWOL in a Fedora Development x86_64 mock
build). No longer attempts to do a debug package. 

Comment 3 Peter Gordon 2007-06-27 03:43:59 UTC
Review in progress; will be posted shortly.

Comment 4 Peter Gordon 2007-06-29 03:12:05 UTC
Here we go.... a formal review of ndesk-dbus 0.5.2-3:

OK: Source matches that of upstream:
     $ md5sum dbus-sharp-0.5.2*.tar.gz
86312f99721a97dc8028343d477fd4be  dbus-sharp-0.5.2-srpm.tar.gz
86312f99721a97dc8028343d477fd4be  dbus-sharp-0.5.2-upstream.tar.gz.
OK: Package naming is good; and the BuildRoot is sane.
OK: Package license (MIT) is acceptable for Fedora and matches that of upstream;
and a copy of this is included in the package as %doc (COPYING).
OK: -devel subpackage properly requires pkgconfig since it installs a .pc file.
OK: Package builds fine in mock (F-7, x86_64); though it fails in Devel (see below).
OK: Spec file is legible and in American English.
OK: Final file and directory ownership looks good; and the %files listing
contains no duplicates. 
OK: The buildroot is cleaned before anything else is done in %install; and a
proper %clean section is used.
OK: Macros are consistent; though the choice was made to use $RPM_foo variables
instead of their respective %foo definitions (entirely aesthetic).
OK: Package contains permissible code.
OK: Files marked as documentation (%doc) do not affect runtime capabilites.
OK: Package contains no libtool files (*.la).	
OK: All files and their names are proper UTF-8.



N/A: This package does not install locale data; so %find_lang handling is not
needed.
N/A: This package does not install shared libraries; so ldconfig invocations in
%post/%postun are not necessary.
N/A: A separate -doc subpackage is not required since this installs very little
documentation.
N/A: Package is not a GUI application; so no .desktop magic is needed.
N/A: Scriptlets are not used.

 
-- BAD ---

  (1) rpmlint output:
> W: ndesk-dbus summary-not-capitalized ndesk-dbus is a C# implementation of D-Bus.
> W: ndesk-dbus summary-ended-with-dot ndesk-dbus is a C# implementation of D-Bus.
> W: ndesk-dbus-devel summary-not-capitalized ndesk-dbus is a C# implementation
of D-Bus.
> W: ndesk-dbus-devel summary-ended-with-dot ndesk-dbus is a C# implementation
of D-Bus.
These are aesthetic; but your Summary should not end in a period ("."); and it
also should not need the "ndesk-dbus is..." prefixing phrase. 


> W: ndesk-dbus obsolete-not-provided dbus-sharp

If it is a true replacement for the old libdbus wrapper, please add a "Provides:
dbus-sharp" along with the Obsoletes tag.

> E: ndesk-dbus only-non-binary-in-usr-lib
Since it installs files to %_libdir, it should not be a noarch package. (Then
again, I'm not too familiar with how Mono handles this. Input from someone who
is would be greatly appreciated. ^_^)

> W: ndesk-dbus-devel no-documentation
It might be wise to add COPYING as the sole %doc file in this subpackage; but
this is otherwise harmless.
 

(2) Development subpackages (*-devel) should have a full EVR for the base
dependency; E.g., change the dependency to Requires: %{name} =
%{version}-%{release}. 

(3) This fails to build in Mock in Development. Adding mono-core to the
BuildRequires appears to fix it.

	make -C src
	make[1]: Entering directory `/builddir/build/BUILD/dbus-sharp-0.5.2/src'
	gmcs -debug -unsafe -d:STRONG_NAME -out:NDesk.DBus.dll -t:library 
-r:Mono.Posix   -keyfile:../ndesk.snk Address.cs AssemblyInfo.cs Bus.cs
BusObject.cs Connection.cs ExportObject.cs Authentication.cs Protocol.cs
Mapper.cs MatchRule.cs Message.cs MessageFilter.cs MessageReader.cs
MessageWriter.cs PendingCall.cs SocketTransport.cs Transport.cs
TypeImplementer.cs Wrapper.cs UnixTransport.cs UnixNativeTransport.cs DBus.cs
Introspection.cs DProxy.cs Signature.cs
	make[1]: gmcs: Command not found
	make[1]: *** [NDesk.DBus.dll] Error 127
	make[1]: Leaving directory `/builddir/build/BUILD/dbus-sharp-0.5.2/src'
	make: *** [all] Error 2

	$ rpm -qf `which gmcs`
	mono-core-1.2.4-1.fc8


(4) Your %setup invocation should be quieter: Use the "-q" option.

Comment 5 David Nielsen 2007-06-29 09:49:52 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-4.fc8.src.rpm

I added the BuildRequires, fixed the Requires, hacked away at the summary, made
setup quiet and I made the package a noarch no more (I'm unsure why I did that
in the first place, this seems more correct to me as well).

Adding the BR did not fix building in Mock here, I've attempted that before as
well. I have no idea what's going on but it seems like the correct thing to do
regardless. Might Development be experiencing problems in this regard, it
clearly works on F7.

Thank you for the review.

Comment 6 Peter Gordon 2007-06-30 21:37:22 UTC
David:

Thanks for fixing the issues I brought up. The only thing left is that it does
not build in Mock (Development). However, this appears to be an error with the
mono packages' dependency tree. My attempt created the following in the
build.log file:

[...]
--> Finished Dependency Resolution
Error: Missing Dependency: libglib-2.0.so.0 is needed by package mono-devel
Error: Missing Dependency: libz.so.1 is needed by package mono-core
Error: Missing Dependency: libgcc_s.so.1(GCC_3.3.1) is needed by package mono-core
Error: Missing Dependency: libgcc_s.so.1(GCC_3.0) is needed by package mono-devel
Error: Missing Dependency: libgcc_s.so.1 is needed by package mono-core
Error: Missing Dependency: libglib-2.0.so.0 is needed by package mono-core
Error: Missing Dependency: libgcc_s.so.1(GCC_3.0) is needed by package mono-core
Error: Missing Dependency: libgthread-2.0.so.0 is needed by package mono-core
Error: Missing Dependency: libgcc_s.so.1 is needed by package mono-devel
Error: Missing Dependency: libgthread-2.0.so.0 is needed by package mono-devel
Error: Missing Dependency: libgmodule-2.0.so.0 is needed by package mono-core
Error: Missing Dependency: libgcc_s.so.1(GCC_3.3.1) is needed by package mono-devel
build
DEBUG: Executing timeout(0): /usr/sbin/mock-helper chroot
/var/lib/mock/fedora-development-x86_64/root /sbin/runuser - root -c "cd
/;/sbin/runuser -c 'rpmbuild --rebuild  --target x86_64 --nodeps
/builddir/build/SRPMS/ndesk-dbus-0.5.2-4.fc8.src.rpm' mockbuild"
Installing /builddir/build/SRPMS/ndesk-dbus-0.5.2-4.fc8.src.rpm
Building target platforms: x86_64
Building for target x86_64
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.59257
+ umask 022
[...]

Thus, these two BR packages do not get installed into the buildroot. However, I
just tested this and it does indeed build fine in an F-7 mock run. Therefore, I
won't consider that a blocker. (Alas, I had not realized at first that mono-core
is a runtime dependency of mono-devel, so mono-core should not be an explicit BR.)

However, there is still work needed to be done on this package before it can be
accepted:

> W: ndesk-dbus summary-not-capitalized ndesk-dbus is a C# implementation of D-Bus.
> W: ndesk-dbus summary-ended-with-dot ndesk-dbus is a C# implementation of D-Bus.
> W: ndesk-dbus mixed-use-of-spaces-and-tabs (spaces: line 15, tab: line 1)

(1) Please fix your Summary and your inconsistency of tabs/space usage.


> W: ndesk-dbus unversioned-explicit-obsoletes dbus-sharp
> W: ndesk-dbus unversioned-explicit-provides dbus-sharp
You need to give an E:V-R to these tags, making it explicitly larger than that
of dbus-sharp to provide a proper upgrade path for users. Please see:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#PackageRename for more
information.

> W: ndesk-dbus macro-in-%changelog setup

This is really bad. Unfortunately, macros in the %changelog do also expand and
cause unexpected troubles later. Remove the macro or escape it by using an extra
percent sign (e.g., "%%setup" and "%%macro_here").

> E: ndesk-dbus no-binary
> E: ndesk-dbus only-non-binary-in-usr-lib
These two say that the package has no binaries in it, but this is a Mono library
which RPM still has troubles identifying, it seems. Thus, these seem harmless.


Comment 7 David Nielsen 2007-07-01 05:06:08 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-5.fc8.src.rpm

Okay, take 5.

Fixed the summaries, the nasty use of spaces instead of tabs and hopefully I
understood how obsoletes and provides work now. Fixed up macro use in changelog.

Now all that remains is to make mock love us.. I hope. This seems to be a mock
bug, maybe opening a bug against mock and letting it block this one would be
acceptable? I would really not like to release ndesk-dbus on the world without
being able to have it at least on all supported releases at the same time since
depend specs are likely to need alterations to not use bundled copies and that
would create ugly unless done at the same time (keeping if nesting to weed out
devel would be the pinnacle of ugly).

Upstream is intending on renaming future tarball releases to ndesk-dbus so a lot
of the ugly should go away in the future, they do however request that packagers
put up with ugly for now.

Comment 8 Peter Gordon 2007-07-04 02:12:09 UTC
Ok, there are only two showstopper issues to resolve before this is accepted.

Firstly, the EVR of your Provides, when evaluated by RPM, actually is _less_
than that of your Provides for dbus-sharp. Please ensure that your Provides EVR
is larger. Also, the current EVR of dbus-sharp is 0.63-6, so I'd probably do
something such as the following:

Provides:  dbus-sharp = 0.64
Obsoletes: dbus-sharp < 0.64

Also, your %defattr lines should contain all four fields: E.g.,
"%defattr(-,root,root,-)".

Those two issues are the only remaining blockers that I see in this packaging. :)

Comment 9 David Nielsen 2007-07-04 09:45:48 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-6.fc8.src.rpm

Fixed those two.. this spec will scar me for life.

Comment 10 Peter Gordon 2007-07-04 20:11:33 UTC
The changes you made look good now. ndesk-dbus 0.5.2-6 is APPROVED. :)

Please close this bug as NEXTRELEASE once you've pushed it to CVS and through
the Fedora build system.

Comment 11 David Nielsen 2007-07-04 21:07:56 UTC
New Package CVS Request
=======================
Package Name: ndesk-dbus
Short Description: Managed C# implementation of DBus
Owners: dnielsen
Branches: FC-6 F-7 devel
InitialCC: 

Comment 12 Kevin Fenzi 2007-07-05 16:14:48 UTC
cvs done.

Comment 14 Peter Gordon 2007-07-06 20:46:00 UTC
Err; thanks for the warning, Michael.

David, you'll need to add a mono(dbus-sharp) Provides similar to what you did
for the plain dbus-sharp package to ensure a proper upgrade path and whatnot.
Sorry for not catching that earlier...

Thanks.

Comment 15 David Nielsen 2007-07-06 21:37:38 UTC
True except banshee should not depend on dbus-sharp since it uses a bundled
version of ndesk-dbus. I've filed a bug for this some time ago but callion has
not yet found time to update the spec I guess. Once both ndesk-dbus and
ndesk-dbus-glib is in the repos the bundled copy should be replaced with a
dependency on ndesk-dbus-devel and ndesk-dbus-glib-devel (banshee has a compile
time option for this). The same should go for other packages, no package I know
of uses the old obsoleted bindings anymore and instead opt for using a bundled
ndesk-dbus. This also explains why dbus use in mono apps currently is broken on
Fedora.

Basically the dependency is bogus but I will provide an updated package with the
next update.

Comment 16 Peter Gordon 2007-07-06 21:51:32 UTC
Great; thanks for the explanation, David!

I'll take care of the ndesk glib package after some food. 8)

Comment 17 David Nielsen 2007-07-06 21:59:56 UTC
and wow now that I even corrected the EVR.. thanks Peter, the package should be
hitting those build boxes soon..

I'm not entirely sure about f-spot but banshee at least should not depend on
dbus-sharp - that's the app I used to test ndesk-dbus with.

Sorry for the breakage, hopefully nobody got hurt in the process - I was hoping
this would have been less... painful.



Comment 18 David Nielsen 2007-07-10 16:03:43 UTC
I'm orphaning this one, the documentation does not say to whom I have to assign
this but here is the CVS change request.

Package Change Request
======================
Package Name: ndesk-dbus
[Updated Fedora Owners: ]


Comment 19 Kevin Fenzi 2007-07-10 23:18:04 UTC
cvs done. 

Comment 20 David Nielsen 2007-10-21 06:39:23 UTC
Okay, I got the personal issues that caused me to abandon Fedora contribution
worked out, as nobody has expressed an interest in this package in my regretable
absence I'm picking it up again. ndesk-dbus will be required to build the
dependencies for Banshee in the near future so we'll probably need to put this
in the repos anyways soon. According to the rules since 3 months has passed it
has to undergo a new review.

Changes:

* Build this as noarch since that upstream recommended this.
* No longer orphan dbus-sharp, they should be able to peacefully coexist and
this part was causing considerable problems, including but not limited to
breaking Blam!

Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-7.fc8.src.rpm

Comment 21 Peter Gordon 2007-10-21 07:02:08 UTC
Yay! I'll let the mock builds go overnight and post another review for this and
the glib package first thing tomorrow morning. :)

Thanks.

Comment 22 Peter Gordon 2007-10-21 20:30:05 UTC
Mock builds are successful (F7 and Devel, x86_64); and the spec looks clean. 
rpmlint is mostly silent, with a false only-non-binary-in-usr-lib (see below).

Most of the package looks good (see above for full review), but Mono packages
should not be noarch. Please revert that change, and this package is tentatively
approved. Same thing for the glib package, too; though a full review of that is
in progress.

Comment 23 David Nielsen 2007-10-21 20:41:59 UTC
I beg to differ as upstream filed a bug against the previous package:

https://bugzilla.redhat.com/show_bug.cgi?id=247629

Comment 24 David Nielsen 2007-10-21 21:03:57 UTC
After a bit of debating on Jabber, Peter convinced me that upstream is wrong and
the packaging guidelines are right. This is once again ExcludeArch ppc64.

I'm closing #247629 once the package gets into the repos.

Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-0.5.2-8.fc8.src.rpm

Comment 25 David Nielsen 2007-10-21 21:33:59 UTC
Okay let's request a F-8 branch and get rocking again

Package Change Request
=======================
Package Name: ndesk-dbus
Short Description: Managed C# implementation of DBus
Owners: dnielsen
Branches: F-7 F-8 devel
InitialCC:

Comment 26 Kevin Fenzi 2007-10-22 19:36:49 UTC
cvs done. 
Note that we are in final freeze for F8, so you will need to contact
releng to get this package added to F8, or wait until after
release and use bodhi to push out the package as an update. 

Comment 27 David Nielsen 2007-10-31 16:57:47 UTC
Okay, this is successfully built for F9, closing


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