[Bug 245492] Review Request: ndesk-dbus-glib - glib mainloop integration for ndesk-dbus

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: ndesk-dbus-glib - glib mainloop integration for ndesk-dbus
Alias: ndesk-dbus-glib

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


peter at thecodergeek.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEEDINFO
               Flag|                            |needinfo?(david at lovesunix.ne
                   |                            |t)




------- Additional Comments From peter at thecodergeek.com  2007-11-01 23:18 EST -------
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. :)

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.