Bug 474768 - Review Request: jpilot-backup - An enhanced backup plugin for J-Pilot
Summary: Review Request: jpilot-backup - An enhanced backup plugin for J-Pilot
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thorsten Leemhuis
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-12-05 10:31 UTC by Patrick C. F. Ernzer
Modified: 2009-02-07 22:19 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-17 14:21:32 UTC
Type: ---
fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Patrick C. F. Ernzer 2008-12-05 10:31:19 UTC
Spec URL: http://pcfe.fedorapeople.org/jpilot-backup.spec
SRPM URL: http://pcfe.fedorapeople.org/jpilot-backup-0.53-3.src.rpm
Description: Hello,
this is my first package submitted for review. The spec file is built on the one included in the tarball. Notable changes to the spec are in the %changelog (basically cleanups), patch to the package added so that it compiles on x86_64. I'd definitely like feedback on that patch as it feels botched to me.

Comment 1 Fabian Affolter 2008-12-08 14:18:50 UTC
Just some comments on your spec file

- '%define version 0.53' is not necessary.  The number in the 'Version:' tag can be reused with %{version}

- Add %{?_smp_mflags} to make in the %build section 
  
- 'BuildRoot: %{_tmppath}/%{name}_%{version}' don't match the recommandations.
  https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

- Remove the *.la files.  They should not be included.
  https://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries

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

Comment 2 Fabian Affolter 2008-12-08 14:22:06 UTC
- 'Release:  3' should be 'Release:  3%{dist}'

Comment 3 Thorsten Leemhuis 2008-12-08 14:43:23 UTC
(In reply to comment #1)
> Just some comments on your spec file

Agreed to most of them (and thanks for your help), with one note:

> - Remove the *.la files.  They should not be included.

Some apps require *.la files for plugins. Is that maybe the case here?

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

Not required, but makes things a whole lot easier


In addition (just some things from a quick look): 

- summary should not start with "A" or "An"

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

- repeating the summary in the %description IMHO is wasting space 

- just wondering: "make install DESTDIR=$RPM_BUILD_ROOT" doesn't work instead of  "make prefix=$RPM_BUILD_ROOT%{_prefix}"?

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

Comment 4 Patrick C. F. Ernzer 2008-12-09 23:07:52 UTC
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

Comment 5 Thorsten Leemhuis 2008-12-13 14:27:31 UTC
(In reply to comment #4)

*
>> - summary should not start with "A" or "An"
> fixed

Really? I still see "Summary: A enhanced backup plugin for J-Pilot" ;-)

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

Removing definitely is better. Static subpackages are hightly discouraged in Fedora; see packaging guidelines for details.


(quoting from the spec or rpmlint)

* 
> License: GPLv2
Looks more like GPLv2+ to me (or did I miss something?). 

*
> Requires: gdbm
Why is that needed? RPM automatically adds a requires for "libgdbm.so.2()", hence please remove it if there there are no good reason for it.


*
>$ rpmlint rpmbuild/RPMS/x86_64/jpilot-backup-* jpilot-backup-0.53-4.fc10.src.rpm 
>jpilot-backup.src: W: strange-permission jpilot-backup.spec 0600
>3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Please fix

* 
looks good apart from the above

Comment 6 Patrick C. F. Ernzer 2008-12-16 13:52:32 UTC
(In reply to comment #5)

[...]
> I still see "Summary: A enhanced backup plugin for J-Pilot" ;-)
doh! my bad, misread originally. Fixed now.

> Removing definitely is better. Static subpackages are hightly discouraged in
> Fedora; see packaging guidelines for details.

OK, removed the provisions to make static package from spec file completely to keep is clean.

> > License: GPLv2
> Looks more like GPLv2+ to me (or did I miss something?). 
No, you are right, I had missed the 'or later' in my first and second look. Fixed now.

> > Requires: gdbm
> Why is that needed? RPM automatically adds a requires for "libgdbm.so.2()",
> hence please remove it if there there are no good reason for it.
Thanks, you're of course right, removed.

> >$ rpmlint rpmbuild/RPMS/x86_64/jpilot-backup-* jpilot-backup-0.53-4.fc10.src.rpm 
> >jpilot-backup.src: W: strange-permission jpilot-backup.spec 0600
> >3 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> Please fix

Fixed.

$ rpmlint jpilot-backup.spec /var/lib/mock//fedora-10-x86_64/result/*rpm
jpilot-backup.spec:33: W: configure-without-libdir-spec
jpilot-backup.src:33: W: configure-without-libdir-spec
3 packages and 1 specfiles checked; 0 errors, 2 warnings.

I let it nag as '%configure --disable-static' did not help in making no .la, so there is little point in leaving a useless flag in.

Comment 7 Thorsten Leemhuis 2008-12-19 17:59:47 UTC
http://pcfe.fedorapeople.org/jpilot-backup-0.53-5.fc10.src.rpm looks good; the rpmlint warning seems to be a false positive

APPROVED

I'll sponsor you once you apply for the packager group

Comment 8 Patrick C. F. Ernzer 2008-12-31 14:22:56 UTC
(In reply to comment #7)
> http://pcfe.fedorapeople.org/jpilot-backup-0.53-5.fc10.src.rpm looks good;

Thank you both for the review and the comments.


> the rpmlint warning seems to be a false positive

ah good.

> APPROVED
> 
> I'll sponsor you once you apply for the packager group

A request to join the 'Fedora Packager CVS Commit Group' is pending approval.

Comment 9 Patrick C. F. Ernzer 2009-01-17 02:06:27 UTC
New Package CVS Request
=======================
Package Name: jpilot-backup
Short Description: Enhanced backup plugin for J-Pilot
Owners: pcfe
Branches: F-10
InitialCC: thl

Comment 10 Kevin Fenzi 2009-01-17 03:15:12 UTC
cvs done.

Comment 11 Patrick C. F. Ernzer 2009-01-17 04:40:05 UTC
jpilot-backup-0.53-5.fc10 is now in cvs and has built fine (taskID=1060518).

a scratch build for dist-rawhide failed with "BuildrootError: could not init mock buildroot, mock exited with status 20" is that an error on my side?

Comment 12 Thorsten Leemhuis 2009-01-17 09:55:46 UTC
(In reply to comment #11)
> a scratch build for dist-rawhide failed with "BuildrootError: could not init
> mock buildroot, mock exited with status 20" is that an error on my side?

No idea without the detailed logs, but guess not.

Comment 13 Patrick C. F. Ernzer 2009-01-17 14:21:32 UTC
as discussed on IRC, make build in devel worked, so a problem with scratch
closing NEXTRELEASE as per step 13 of http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess

Comment 14 Fedora Update System 2009-01-17 14:29:18 UTC
jpilot-backup-0.53-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/jpilot-backup-0.53-5.fc10

Comment 15 Fedora Update System 2009-02-07 22:19:14 UTC
jpilot-backup-0.53-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.


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