Bug 756046

You should probably refer to 'udisks2' in the %description of the main package, as well as the subpackage descriptions. Or maybe just add 'This package contains version 2 of udisks.' for the main package.

%defattr(-,root,root,-) is no longer needed

There have been requests before to move the bash completion to /etc/bash-completion.d (https://bugzilla.redhat.com/show_bug.cgi?id=584569). Something to consider, maybe.

Other than that, the spec file looks fine at first glance. Will do a formal review soon.

Building in mock reveals a missing 

BuildRequires: gobject-introspection-devel

After adding that line, it builds ok.

rpmlint output:

 rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/udisks2-*.rpm
udisks2.src: W: spelling-error %description -l en_US udisks -> disks, u disks, Saudis
udisks2.src:98: E: hardcoded-library-path in /lib/udisks2/udisksd
udisks2.src: W: file-size-mismatch udisks-1.90.0.git20111122.tar.bz2 = 584775, http://people.freedesktop.org/~david/udisks-1.90.0.git20111122.tar.bz2 = 640107
udisks2.x86_64: W: unexpanded-macro dependency dbus >= %{dbus_version} %{dbus_version}
udisks2.x86_64: W: spelling-error %description -l en_US udisks -> disks, u disks, Saudis
udisks2.x86_64: E: executable-sourced-script /etc/profile.d/udisksctl-bash-completion.sh 0755L
udisks2.x86_64: E: non-standard-dir-perm /var/lib/udisks2 0700L
udisks2.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/org.freedesktop.UDisks2.conf
udisks2.x86_64: W: no-manual-page-for-binary umount.udisks2
3 packages and 0 specfiles checked; 3 errors, 6 warnings.

Some things to fix here:

- The hardcoded library path is fine, but you need to add a 
  %dir /lib/udisks2 to own the directory.

- The file size mismatch should be investigated and fixed.

- You need to define dbus_version

- Should make /etc/profile.d/udisksctl-bash-completion.sh non-executable
  (and, as already mentioned, consider moving it)

- You should probably add a comment in the file list explaining
  the permissions of /var/lib/udisks2

Review checklist:

package name: ok
spec file name: ok
packaging guidelines: ok, apart from whats mentioned in the previous comment
license: ok
license field: ok
license file: ok
spec language: ok
spec readable: ok
upstream sources: see the rpmlint complaint above
buildable: ok, module missing BR
excludearch: ok
buildrequires: see initial comment, needs gobject-introspection-devel
locale handling: ok
ldconfig: ok
system libs: ok
relocatable: ok
dir ownership: see above, must own /lib/udisks2
duplicate files: ok
permissions: ok. note that defattr() is no longer needed
macro use: ok
permissible content: ok
large docs: ok
%doc content: ok
headers: ok
static libs: ok
shared libs: ok
devel deps: ok
libtool archives: ok
gui apps: ok
file ownership: ok
utf8 filenames: ok

Hey, thanks for the review. I've uploaded new files

 http://people.freedesktop.org/~david/udisks2.spec
 http://people.freedesktop.org/~david/udisks2-1.90.0-0.git20111128.fc16.src.rpm

Here's a unified diff of the spec file changes made:

 http://people.freedesktop.org/~david/udisks2.spec.diff

Inline comments: 

(In reply to comment #1)
> You should probably refer to 'udisks2' in the %description of the main package,
> as well as the subpackage descriptions. Or maybe just add 'This package
> contains version 2 of udisks.' for the main package.

I've added "This package is for the udisks 2.x series" to all package descriptions.

> %defattr(-,root,root,-) is no longer needed

Nuked.

> There have been requests before to move the bash completion to
> /etc/bash-completion.d (https://bugzilla.redhat.com/show_bug.cgi?id=584569).
> Something to consider, maybe.

OK, don't really agree but not worth fighting. Changed upstream.

(In reply to comment #2)
> Building in mock reveals a missing 
> 
> BuildRequires: gobject-introspection-devel
> 
> After adding that line, it builds ok.

Fixed.

> rpmlint output:
> 
>  rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/udisks2-*.rpm
> udisks2.src: W: spelling-error %description -l en_US udisks -> disks, u disks,
> Saudis
> udisks2.src:98: E: hardcoded-library-path in /lib/udisks2/udisksd
> udisks2.src: W: file-size-mismatch udisks-1.90.0.git20111122.tar.bz2 = 584775,
> http://people.freedesktop.org/~david/udisks-1.90.0.git20111122.tar.bz2 = 640107
> udisks2.x86_64: W: unexpanded-macro dependency dbus >= %{dbus_version}
> %{dbus_version}
> udisks2.x86_64: W: spelling-error %description -l en_US udisks -> disks, u
> disks, Saudis
> udisks2.x86_64: E: executable-sourced-script
> /etc/profile.d/udisksctl-bash-completion.sh 0755L
> udisks2.x86_64: E: non-standard-dir-perm /var/lib/udisks2 0700L
> udisks2.x86_64: W: non-conffile-in-etc
> /etc/dbus-1/system.d/org.freedesktop.UDisks2.conf
> udisks2.x86_64: W: no-manual-page-for-binary umount.udisks2
> 3 packages and 0 specfiles checked; 3 errors, 6 warnings.
> 
> Some things to fix here:
> 
> - The hardcoded library path is fine, but you need to add a 
>   %dir /lib/udisks2 to own the directory.

Done.

> - The file size mismatch should be investigated and fixed.

I did a new tarball with the same name but forgot to put it online.

> - You need to define dbus_version

Done.

> - Should make /etc/profile.d/udisksctl-bash-completion.sh non-executable
>   (and, as already mentioned, consider moving it)

Done upstream, see above.

> - You should probably add a comment in the file list explaining
>   the permissions of /var/lib/udisks2

Added.

(In reply to comment #3)
> Review checklist:
> 
> package name: ok
> spec file name: ok
> packaging guidelines: ok, apart from whats mentioned in the previous comment
> license: ok
> license field: ok
> license file: ok
> spec language: ok
> spec readable: ok
> upstream sources: see the rpmlint complaint above

Should be fixed.

> buildable: ok, module missing BR

Fixed.

> excludearch: ok
> buildrequires: see initial comment, needs gobject-introspection-devel

Fixed.

> locale handling: ok
> ldconfig: ok
> system libs: ok
> relocatable: ok
> dir ownership: see above, must own /lib/udisks2

Fixed.

> duplicate files: ok
> permissions: ok. note that defattr() is no longer needed

Fixed.

> macro use: ok
> permissible content: ok
> large docs: ok
> %doc content: ok
> headers: ok
> static libs: ok
> shared libs: ok
> devel deps: ok
> libtool archives: ok
> gui apps: ok
> file ownership: ok
> utf8 filenames: ok

Thanks for the review.

Looks all good now, approved.

New Package SCM Request
=======================
Package Name: udisks2
Short Description: Disk Manager
Owners: davidz
Branches: f17
InitialCC:

Git done (by process-git-requests).

No need to request f17, ==devel.

Thanks, packages built: http://koji.fedoraproject.org/koji/taskinfo?taskID=3548581