This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 543608 - Review Request: udisks - Storage Management Service
Review Request: udisks - Storage Management Service
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-02 13:04 EST by David Zeuthen
Modified: 2010-02-13 22:53 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-02-13 22:53:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mclasen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description David Zeuthen 2009-12-02 13:04:26 EST
Spec URL: http://people.freedesktop.org/~david/udisks-pkg-review/20091202/udisks.spec
SRPM URL: http://people.freedesktop.org/~david/udisks-pkg-review/20091202/udisks-1.0.0-0.git20091202.fc13.src.rpm

Description:

DeviceKit-disks recently got renamed to udisks, see

 http://lists.freedesktop.org/archives/devkit-devel/2009-December/000567.html

so it is only proper to change the Fedora package name too. This package will obsolete (and provide - for transitioning) DeviceKit-disks. 

It is important to note that DeviceKit-disks has never claimed to provide a stable ABI or API, see

 $ head -9 /usr/share/doc/DeviceKit-disks-009/NEWS
 -------------------
 DeviceKit-disks 009
 -------------------

 DeviceKit-disks is a daemon that provide interfaces to obtain
 information and perform operations on storage devices.

 NOTE NOTE NOTE: This is an unstable release of DeviceKit-disks, all
                 API is subject to change.
 $ rpm -qf /usr/share/doc/DeviceKit-disks-009/NEWS
 DeviceKit-disks-009-3.fc12.x86_64

so changing the name is no different than uploading a new DeviceKit-disks package. 

Note also that the new udisks packages provide slightly stronger ABI and API guarantees (see the mail linked to above) so packages using it should be able to do

 Requires:  udisks >= 1.0.<something>
 Requires:  udisks < 1.1.0

instead of the mess we have today.

The main user, gnome-disk-utility, will use udisks in the next version.

The only other user of DeviceKit-disks (according to 'repoquery --whatrequires DeviceKit-disks') is the emelfm2 package. It should be easy for that package to transition (the gnome-disk-utility patch is ~200 lines - mostly just changing the D-Bus bus names and interfaces) and emelfm2 would have to _anyway_ because DeviceKit-disks never claimed to support any stable ABI or API - e.g. the next DeviceKit-disks version could have used a completely different ABI.

The spec file is based on the existing DeviceKit-disks one with a few cleanups.

 $ rpmlint udisks.spec 
 0 packages and 1 specfiles checked; 0 errors, 0 warnings.

 $ rpmlint ../SRPMS/udisks-1.0.0-0.git20091202.fc13.src.rpm 
 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

 $ rpmlint ../RPMS/x86_64/udisks-devel-1.0.0-0.git20091202.fc13.x86_64.rpm 
 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

 $ rpmlint ../RPMS/x86_64/udisks-1.0.0-0.git20091202.fc13.x86_64.rpm 
 udisks.x86_64: W: non-conffile-in-etc /etc/profile.d/udisks-bash-completion.sh
 udisks.x86_64: E: non-standard-dir-perm /var/run/udisks 0700
 udisks.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/org.freedesktop.UDisks.conf
 udisks.x86_64: E: non-standard-dir-perm /var/lib/udisks 0700
 1 packages and 0 specfiles checked; 2 errors, 2 warnings.

Both warnings and errors should be waived - reasons

 o /etc/profile.d/udisks-bash-completion.sh isn't a config-file at
   all - it is a shell script for bash completion

 o /etc/dbus-1/system.d/org.freedesktop.UDisks.conf - if users/admins
   wants to override D-Bus config directives (they have no reason to
   do this though) they can drop files in /etc/dbus-1/system.d. Note
   that we have talked in the D-Bus project about using $datadir or
   $libdir for these kinds of files. It might happen.

   This is also the case for every other package using the D-Bus
   system bus.

 o /var/run/udisks and /var/lib/udisks needs these permissions because
   we don't want to disclose this data to other users (might be an
   information leak to let everyone know that a user has mounted a disk)

Thanks for reviewing this.
Comment 1 David Zeuthen 2009-12-02 14:23:13 EST
For good measure, the original DeviceKit-disks review was bug 456033.
Comment 2 David Zeuthen 2009-12-02 15:12:02 EST
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.
Comment 3 Matthias Clasen 2009-12-03 16:14:43 EST
Builds fine in mock, I get the same rpmlint output.
Comment 4 Matthias Clasen 2009-12-03 16:22:51 EST
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 ?
Comment 5 Matthias Clasen 2009-12-03 20:31:20 EST
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
Comment 6 Matthias Clasen 2009-12-03 21:50:05 EST
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/
Comment 7 David Zeuthen 2009-12-04 08:23:41 EST
(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.
Comment 8 David Zeuthen 2009-12-04 08:44:46 EST
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
Comment 10 Matthias Clasen 2009-12-04 08:59:03 EST
Looks fine now. Approved.
Comment 11 David Zeuthen 2009-12-04 09:18:51 EST
Thanks for the review. Requesting cvs.
Comment 12 Kevin Fenzi 2009-12-06 18:15:46 EST
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.
Comment 13 David Zeuthen 2009-12-07 10:35:25 EST
New Package CVS Request
=======================
Package Name: udisks
Short Description: Storage Management Server
Owners: davidz
Branches:
InitialCC:
Comment 14 David Zeuthen 2009-12-07 11:15:52 EST
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:
Comment 15 Kevin Fenzi 2009-12-07 11:35:27 EST
cvs done.

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