[Bug 756046] Review Request: udisks2 - Disk Manager

[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.


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

--- Comment #4 from David Zeuthen <davidz at redhat.com> 2011-11-28 10:56:43 EST ---
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.

-- 
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.