Bug 245492 (ndesk-dbus-glib) - Review Request: ndesk-dbus-glib - glib mainloop integration for ndesk-dbus
Summary: Review Request: ndesk-dbus-glib - glib mainloop integration for ndesk-dbus
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: ndesk-dbus-glib
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: ndesk-dbus
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-06-24 08:44 UTC by David Nielsen
Modified: 2007-11-30 22:12 UTC (History)
3 users (show)

Fixed In Version: ndesk-dbus-glib-0.4.1-1.fc9
Clone Of:
Environment:
Last Closed: 2007-11-17 08:58:27 UTC
Type: ---
Embargoed:
peter: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description David Nielsen 2007-06-24 08:44:10 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib-0.3-1.fc8.src.rpm
Description: ndesk-dbus-glib is glib mainloop integration for the C# implementation of D-Bus ndesk-dbus. 

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 08:45:00 UTC
block on 245491 since this depends on ndesk-dbus which is also under review.

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

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

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

Significantly less hacky. Also let's not attempt to build that debug stuff.

Comment 4 David Nielsen 2007-07-04 09:56:00 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib-0.3-4.fc8.src.rpm

Fix a few problems found during the ndesk-dbus review which also applied to this
one.

Comment 5 David Nielsen 2007-07-05 18:18:40 UTC
Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib-0.3-5.fc8.src.rpm

And let's not build this on ppc64, it lacks the proper dependencies.

Comment 6 Peter Gordon 2007-07-06 21:10:03 UTC
I'll review this shortly.

Comment 7 David Nielsen 2007-07-10 16:12:34 UTC
I'm dropping this as part of orphaning my packages. I'll leave the package and
spec on the server if someone else wants to pick it up at least this might save
them some time.

Comment 8 David Nielsen 2007-10-21 06:40:43 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-glib 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.

Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib-0.3-6.fc8.src.rpm


Comment 9 David Nielsen 2007-10-21 21:07:04 UTC
Revert noarch change to confort to the guidelines, Upstream 0 : Peter Gordon 1

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



Comment 10 Peter Gordon 2007-11-02 03:18:02 UTC
Okey dokey....sorry about the lateness of this. Here we go!

== GOOD ==
+ Naming and version/release are appropriate.
+ Source tarball matches that of upstream:
	319d423fa3dfe8bfddba69e2b7c58df8  dbus-sharp-glib-0.3-upstream.tar.gz
	319d423fa3dfe8bfddba69e2b7c58df8  dbus-sharp-glib-0.3-srpm.tar.gz
+ License (MIT) is Fedora-acceptable and matches the licenses of the source files.
+ Spec file is written in American English, and is legible. 
+ Builds fine in mock (F7 and Development, x86_64); all BuildRequires are listed
without duplicates or repeats via dependencies.
+ ExcludeArch bug filed as appropriate (PPC64)
+ Final file/directory ownership is good, and does not conflict with system
packages. No duplicate files are listed.
+ Files listed properly with an appropriate %defattr(...) line.
+ A %clean section exists and includes only "rm -rf $RPM_BUILD_ROOT" as required.
+ Macro usage is consistent.
+ Package contains code.
+ pkgconfig data (.pc file) is in a devel subpackage as required; and the devel
subpackage properly hardcodes a dependency on pkgconfig.
+ devel subpackage properly hardcodes a dependency on its parent
(ndesk-dbus-glib) of the same Version/Release.
+ The buildroot is properly cleaned at the beginning of %install (via "rm -rf
$RPM_BUILD_ROOT").
+ All filenames are valid UTF-8.

== MINOR ==
(1) The "Url" tag in your spec file should be all-uppercase: "URL: ..."

(2) rpmlint complains about the source RPM, though the binary  
> ndesk-dbus-glib.src: W: mixed-use-of-spaces-and-tabs (spaces: line 17, tab:
line 1)

Just an inconsistency between using spaces and tabs for indenting. Choose one or
the other and stick to it, please. :)


(3) Your spec contains:
> Source1:		include.mk
> Patch0:		build-fix.patch

Please prefix any extra files with %{name}, so that file conflicts are kept
to a minimum when installing multiple source RPMs to the same build tree. 


(4) You're using %{?buildsubdir} in the various MONO_SHARED_DIR settings. Is
that really needed? There seems to be no definition for it in the standard rpm
macros. (Then again, this was also in the ndesk-dbus package, so if you remove
it from here, please remove it in that package as well to keep consistent.)


(5) While understandable, the ndesk-dbus dependency is not needed, as RPM
Mono-handling is sane enough that it registers the library as a mono() provides,
and knows that this is linked to it so uses that for it's dependency. During
their builds, the following are output:

ndesk-dbus:
> Provides: mono(NDesk.DBus) = 1.0.0.0

ndesk-dbus-glib:
> Provides: mono(NDesk.DBus.GLib) = 1.0.0.0
> Requires: mono(NDesk.DBus) = 1.0.0.0 mono(mscorlib) = 2.0.0.0 ndesk-dbus

Unless another alternate package also provides this, you should not need to
manually specify the ndesk-dbus dep.

 

=== BLOCKER ===
- Since the upstream source tarball includes a copy of the license text
(COPYING), please add to the %doc of the package. 



== NOT APPLICABLE ==
* Includes no i18n, so %find_lang is not needed.
* Does not install shared libs; /sbin/ldconfig invocations are not needed.
* Package is not relocatable.
* Package does not need a doc subpackage, since no documentation is included.
* No %doc files are listed; thus nothing in runtime breaks if they are not present.
* No headers or static libraries; and no native-code shared libraries.
* Package contains no .la (libtool) or GUI stuff.



Also, would you please bump the package to 0.4.1 (available from their website)?
Once we can filter through these small issues, it should be golden. :)

Comment 11 David Nielsen 2007-11-08 09:22:45 UTC
I fixed the release blockers.. and I upgraded to 0.4.1, as discussed on jabber
this meant switching to a much cleaner autofoo build process. I added a dirty
hack to both this and the base package to correctly set the libdir, I'm sure
there's a cleaner way to do it but this one seemed good (ifarch really needs a
ifarch 64bit thingy).

* updated to 0.4.1
* Patches are gone
* COPYING is in %doc
* Dependency removed 
* Url -> URL
* spaces -> tabs (all hail the mighty tab)

Spec URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib.spec
SRPM URL: http://www.lovesunix.net/fedora/ndesk-dbus-glib-0.4.1-1.fc8.src.rpm

Comment 12 David Nielsen 2007-11-08 10:48:18 UTC
It's likely you'll need ndesk-dbus 0.6.0 for this, I have updated that spec and
it builds locally on x86_64 but throw an error on koji. I'm trying to figure out
where in the code the /usr/lib/mono/2.0/mscorlib.dll reference comes from, but
I'd naturally love your expertise Senor Gordon.

http://koji.fedoraproject.org/koji/taskinfo?taskID=230519

Comment 13 Peter Gordon 2007-11-10 19:59:26 UTC
The Mono/multlib stuff should be resolved with recent updates. I'll try to build
this locally later today and finish the review from that. Thanks.

Comment 14 David Nielsen 2007-11-12 05:24:04 UTC
And it hit the build system so here's a package:
http://koji.fedoraproject.org/koji/buildinfo?buildID=23677

Comment 15 Peter Gordon 2007-11-12 07:24:52 UTC
Mock build is good (testThe spec looks properly cleaned up as requested, except
for one tiny detail: Is there a reason for the %ifarch conditional in the update
spec's %install section? It looks to me that you're only using it to get the
proper lib or lib64 directory; in which case you can simply use %{_libdir}
instead of hardcoding /usr/lib or /usr/lib64. The reason I ask this is that
other 64-bit arches (secondary, AFAIK) exist that you'd probably want to add to
it if you choose to leave it as a conditional: ppc64, sparc64, alpha, et al.
Keeping it as a simple %{_libdir} macro would preclude the need for any such
conditional.

Other than that, ndesk-dbus-glib-0.4.1-1 is APPROVED. Please fix that up before
you import it. :)

Comment 16 David Nielsen 2007-11-12 07:45:52 UTC
Thank you Peter.

New Package CVS Request
=======================
Package Name: ndesk-dbus-glib
Short Description: glib mainloop integration for ndesk-dbus
Owners: dnielsen
Branches: devel F-8 F-7
InitialCC: (none)
Cvsextras Commits: Yes

Comment 17 Kevin Fenzi 2007-11-12 17:30:43 UTC
cvs done.

Comment 18 David Nielsen 2007-11-17 08:58:27 UTC
This is, finally, in devel - closing

http://koji.fedoraproject.org/koji/taskinfo?taskID=237147


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