Bug 543608

For good measure, the original DeviceKit-disks review was bug 456033.

FYI, the gnome-disk-utility package cvs has been updated to use udisks - I can't build the package until udisks is available in the build roots though.

Anyway, once you install udisks (which will remove DeviceKit-disks in the process) you want the new gnome-disk-utility packages in order for GVfs/Nautilus to keep working. Normal users won't see this problem unless they specifically install udisks. Once the new gnome-disk-utilities packages are built users will transition without any problems.

Builds fine in mock, I get the same rpmlint output.

ln -s udisks $RPM_BUILD_ROOT%{_bindir}/devkit-disks
ln -s udisks.1 $RPM_BUILD_ROOT%{_datadir}/man/man1/devkit-disks.1

Should these be relative ?

Nevermind, they are relative.

Formal review:

Package name: ok
Spec file name: ok
Packaging guidelines: the guidelines nowadays contain language that forbids
  explicit dependencies for stuff thats already pulled in via library
  dependencies. So the Requires for dbus, dbus-glib, glib2, polkit, parted, 
  udev, libatasmart should be dropped unless you need the specific minimal 
  versions.
  It might be nice to have a /usr/libexec/udisks/ instead of dumping all the
  helpers into /usr/libexec, but thats not a must-fix.
License: ok
License field: ok
License file: ok
Spec file language: ok
Spec file readable: ok
Upstream sources: need upstream location
Buildable: ok
ExcludeArch: ok, none
BuildRequires: ok
Locale handling: ok
ldconfig: ok, no libs
system libraries: ok, no libs
relocatable: no
directory ownership: ok
duplicate files: ok
permissions: ok
%clean: ok
macro use: ok
permissible content: ok
large docs: ok
%doc content: ok
header files: ok, none
static libs: ok, none
pc files: ok, according to https://fedoraproject.org/wiki/PackagingDrafts/PkgconfigAutoRequires, which I believe was recently accepted
shared libs: ok, none
devel deps: ok
libtool archives: ok, none
gui apps: ok, no gui app
file ownership: ok
%install: ok
utf8 filenames: ok


# TODO: should be fixed upstream

Is it ?


In summary: 
- remove explicit library dependencies, then the package is acceptable.
- additionally, consider using a subdir in /usr/libexec and fixing the TODO upstream

Wrt to the bash completion script, see https://bugzilla.redhat.com/show_bug.cgi?id=543989 which asks for it to be moved to /etc/bash_completion.d/

(In reply to comment #6)
> Wrt to the bash completion script, see
> https://bugzilla.redhat.com/show_bug.cgi?id=543989 which asks for it to be
> moved to /etc/bash_completion.d/  

I disagree it should be in /etc/bash_completion.d - and this very discussion has been had a number of times with the old PolicyKit version and I haven't changed my mind - /etc/profile.d is a fine location for the bash completion scripts.

Hey,

thanks for reviewing this

(In reply to comment #5)
> Packaging guidelines: the guidelines nowadays contain language that forbids
>   explicit dependencies for stuff thats already pulled in via library
>   dependencies. So the Requires for dbus, 

I'm going to keep this one because we really need the bus daemon running - and it's not impossible (in fact it would be desirable) that libdbus.so could be packaged separately from the bus daemon.

> dbus-glib, glib2, polkit, parted, 

Removed.

>   udev, 

We need the udev daemon - libudev and libgudev are packaged separately.

> libatasmart 

We need the specified minimal version for certain bug-fixes.

>   It might be nice to have a /usr/libexec/udisks/ instead of dumping all the
>   helpers into /usr/libexec, but thats not a must-fix.

All files are prefixed with udisks- anyway. We might want to just use $libdir/udisks instead to avoid this though. I'll consider this upstream.

(FWIW, I'd rather see Fedora just pass --libexecdir=$libdir/$package_name just like Debian and SUSE does.)

> Upstream sources: need upstream location

Fixed (see TODO)

> # TODO: should be fixed upstream
> 
> Is it ?

Not yet. I believe this is a bug with install(1) since the file has mode 0644 in the git repo - I need to investigate further.

Here are the changes I've made:

--- udisks.spec.prev	2009-12-04 08:26:12.000000000 -0500
+++ udisks.spec	2009-12-04 08:41:43.000000000 -0500
@@ -17,7 +17,8 @@
 License: GPLv2+
 Group: System Environment/Libraries
 URL: http://www.freedesktop.org/wiki/Software/udisks
-Source0: %{name}-%{version}.git20091202.tar.gz
+# TODO: when fd.o #22578 is resolved, update URL to proper location
+Source0: http://hal.freedesktop.org/releases/%{name}-%{version}.git20091202.tar.gz
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root
 BuildRequires: glib2-devel >= %{glib2_version}
 BuildRequires: dbus-devel  >= %{dbus_version}
@@ -30,14 +31,13 @@
 BuildRequires: libgudev1-devel >= %{udev_version}
 BuildRequires: libudev-devel >= %{udev_version}
 BuildRequires: sg3_utils-devel >= %{sg3_utils_version}
+# needed to pull in the system bus daemon
 Requires: dbus >= %{dbus_version}
-Requires: dbus-glib >= %{dbus_glib_version}
-Requires: glib2 >= %{glib2_version}
-Requires: polkit >= %{polkit_version}
-Requires: parted >= %{parted_version}
+# needed to pull in the udev daemon
 Requires: udev >= %{udev_version}
-Requires: mdadm >= %{mdadm_version}
+# we need at least this version for bugfixes / features etc.
 Requires: libatasmart >= %{libatasmart_version}
+Requires: mdadm >= %{mdadm_version}
 # for smp_rep_manufacturer
 Requires: smp_utils >= %{smp_utils_version}
 # for mount, umount, mkswap
@@ -50,8 +50,7 @@
 Requires: dosfstools
 # for mlabel
 Requires: mtools
-# for mkntfs
-# no ntfsprogs on ppc, though
+# for mkntfs - no ntfsprogs on ppc, though
 %ifnarch ppc ppc64
 Requires: ntfsprogs
 %endif

Updated SPEC and SRPM files here

Spec URL: http://people.freedesktop.org/~david/udisks-pkg-review/20091204/udisks.spec
SRPM URL: http://people.freedesktop.org/~david/udisks-pkg-review/20091204/udisks-1.0.0-0.git20091202.1.fc13.src.rpm

Thanks.

Looks fine now. Approved.

Thanks for the review. Requesting cvs.

Please put a cvs template here so we know what branches you need, etc. 

https://fedoraproject.org/wiki/CVS_admin_requests

Reset the flag when ready.

New Package CVS Request
=======================
Package Name: udisks
Short Description: Storage Management Server
Owners: davidz
Branches:
InitialCC:

Actually I got it wrong - here's the right request (s/Server/Service). Sorry about that.

New Package CVS Request
=======================
Package Name: udisks
Short Description: Storage Management Service
Owners: davidz
Branches:
InitialCC:

cvs done.