Bug 474768 - Review Request: jpilot-backup - An enhanced backup plugin for J-Pilot
Review Request: jpilot-backup - An enhanced backup plugin for J-Pilot
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thorsten Leemhuis
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-05 05:31 EST by Patrick C. F. Ernzer
Modified: 2009-02-07 17:19 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-17 09:21:32 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Patrick C. F. Ernzer 2008-12-05 05:31:19 EST
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 09:18:50 EST
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 09:22:06 EST
- 'Release:  3' should be 'Release:  3%{dist}'
Comment 3 Thorsten Leemhuis 2008-12-08 09:43:23 EST
(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 18:07:52 EST
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 09:27:31 EST
(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 08:52:32 EST
(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 12:59:47 EST
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 09:22:56 EST
(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-16 21:06:27 EST
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-16 22:15:12 EST
cvs done.
Comment 11 Patrick C. F. Ernzer 2009-01-16 23:40:05 EST
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 04:55:46 EST
(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 09:21:32 EST
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 09:29:18 EST
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 17:19:14 EST
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.