Bug 474768

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.

- 'Release:  3' should be 'Release:  3%{dist}'

(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)

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

(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

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

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

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

New Package CVS Request
=======================
Package Name: jpilot-backup
Short Description: Enhanced backup plugin for J-Pilot
Owners: pcfe
Branches: F-10
InitialCC: thl

cvs done.

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?

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

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

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

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.