[Bug 474768] Review Request: jpilot-backup - An enhanced backup plugin for J-Pilot

bugzilla at redhat.com bugzilla at redhat.com
Tue Dec 9 23:07:53 UTC 2008


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=474768





--- Comment #4 from Patrick C. F. Ernzer <pcfe at redhat.com>  2008-12-09 18:07:52 EDT ---
Thanks for the feedback.

(In reply to comment #1)
> - '%define version 0.53' is not necessary.  The number in the 'Version:' tag
> can be reused with %{version}
fixed

> - Add %{?_smp_mflags} to make in the %build section 
done

> - 'BuildRoot: %{_tmppath}/%{name}_%{version}' don't match the recommandations.
fixed

> - Remove the *.la files.  They should not be included.
as you can see in the -4 spec, I manually remove it but have provisions in
place to build a -static subpackage. I think just removing is better in this
case but would like comments.

> - '%defattr(-,root,root)' should be '%defattr(-,root,root,-)' for the default
> directory permissions.
fixed

(In reply to comment #2)
> - 'Release:  3' should be 'Release:  3%{dist}'
fixed

(In reply to comment #3)
> Some apps require *.la files for plugins. Is that maybe the case here?
Does not seem so, the plugin still backs up just fine when libbackup.la is not
present on my system.
OTOH, the main jpilot RPM does drop .la files in addition to the .so files

[...]
> - summary should not start with "A" or "An"
fixed

> - I don't like the "enhanced" in the summary much, as that information is not
> helpful at all for new users

jpilot itself includes backup functionality. The included version requires the
user to explicitly click a 'backup' button whereas the jpilot-backup package
allows setting up rules like 'backup every sync' or 'backup daily'. Hence the
'enhanced'. Would you still like me to drop the word?

> - repeating the summary in the %description IMHO is wasting space 
true, fixed.

> - just wondering: "make install DESTDIR=$RPM_BUILD_ROOT" doesn't work instead
> of  "make prefix=$RPM_BUILD_ROOT%{_prefix}"?
does not seem to work (make install tried to write under / instead of
$RPM_BUILD_ROOT), so I left it as it was.

> - would be nice to split the {build,}requires in multiple lines (one per line)
done

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




More information about the Fedora-package-review mailing list