[Bug 467958] Review Request: barry - BlackBerry(tm) Desktop for Linux

[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=467958





--- Comment #5 from Hans de Goede <hdegoede at redhat.com>  2008-11-05 05:23:29 EDT ---
Ok, it builds this time, so full review this time:

MUST FIX
--------

* Do not uses tabs, only use spaces, rpmlint has a good reason to complain,
here is how the specfile looks in my editor (with standard 8 space tabs):
Source0:        http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar
Source1:                 %{name}.desktop
BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

* Put either the utils or the libs in a separate sub-package, with multilib (64
bit systems with 32 bit compatibility) both a 32 bit and a 64 bit version of
the sub-package containing the libs will get added to the repository, we don't
want to have both 32 and 64 bit utils in the 64 bit repository

* The license tag should be GPLv2+ not GPLv2, I so no license headers without
  the "or any later version" language

* "barry.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libbarry.so",
that file should really go the the -devel package, and no the utils don't need
it, the trick is to put the libbarry.so.* files in the libs package and
libbarry.so in the devel package

* Only documentation, not doc build scripts should be shipped as %doc:
barry-devel.x86_64: W: spurious-executable-perm
/usr/share/doc/barry-devel-0.14/www/static.sh
barry-devel.x86_64: W: spurious-executable-perm
/usr/share/doc/barry-devel-0.14/www/clean.sh
barry-devel.x86_64: W: doc-file-dependency
/usr/share/doc/barry-devel-0.14/www/clean.sh R
barry-devel.x86_64: W: doc-file-dependency
/usr/share/doc/barry-devel-0.14/www/static.sh R
So you must not include static.sh and clean.sh here, there are 2 ways todo
this:
1) rm the scripts in %setup
2) make a copy of the www dir in %build and remove the files from the copy
   (use this when the scripts are needed during build / make intall)
   and then use the copy in %doc

* barry seems to out rpaths in its binaries when build on x86_64:
barry.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/bidentify
['/usr/lib64']
<and lots more>
Put the following 3 lines between each %{configure} and %{__make} (so put them
in 3 times):
# Disable rpath
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

* barry.spec has various directory ownership issues, all non standard dirs must
be owned either by barry itself, or by a package required by barry:

%config(noreplace) %{_sysconfdir}/udev/rules.d/*
You should add a "Requires: udev" as that ownes the %{_sysconfdir}/udev/rules.d
dir

%config(noreplace) %{_sysconfdir}/security/console.perms.d/*
Add a "Requires: pam"

%config(noreplace) %{_sysconfdir}/modprobe.d/blacklist-berry_charge
Add a "Requires: module-init-tools"

%config(noreplace) %{_sysconfdir}/ppp/peers/barry-rogers
Add a "Requires initscripts"

%config(noreplace) %{_sysconfdir}/chatscripts/barry-rogers.chat
Hmm, no such directory exists on my system, better own it in the package, add
a:
%dir %{_sysconfdir}/chatscripts
To the %files list

%{_includedir}/barry/*
barry-devel should own %{_includedir}/barry, so instead write:
%{_includedir}/barry/
That will get you to own the dir and gets you all the files below it

%{_libdir}/pkgconfig/*.pc
Add a "Requires: pkgconfig" to the devel package

%{_datadir}/barry/glade/*.glade
barry should own %{_datadir}/barry and everything below it, so write:
%{_datadir}/barry/

%{_libdir}/opensync
You should add a Requires: for what ever package owns %{_libdir}/opensync


* Do not package .la files, instead remove them from the buildroot in %install
after the make install


Thats it, with the next iteration please increase Release one and add a
changelog entry what you changed, we always track changes even during 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.