Bug 473590 - Review Request: libiphone - A library for connecting to Apple iPhone and iPod touch
Review Request: libiphone - A library for connecting to Apple iPhone and iPod...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Bill Nottingham
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 473591
  Show dependency treegraph
 
Reported: 2008-11-29 11:18 EST by Peter Robinson
Modified: 2014-03-16 23:16 EDT (History)
5 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description Peter Robinson 2008-11-29 11:18:03 EST
SPEC: http://pbrobinson.fedorapeople.org/libiphone.spec
SRPM: http://pbrobinson.fedorapeople.org/libiphone-0.1.0-1.fc10.src.rpm

libiphone is a library for connecting to Apple's iPhone or iPod touch devices
Comment 1 Bill Nottingham 2008-12-01 15:43:12 EST
MUST items:
- Package meets naming and packaging guidelines - OK
- Spec file matches base package name. - OK
- Spec has consistent macro usage. - OK
- Meets Packaging Guidelines. - ***

Summary should probably drop the leading 'A'.

You appear to be packaging a git snapshot. (In fact, you're including the entire .git directory in the tarball, which isn't really needed.) 

Please see https://fedoraproject.org/wiki/Packaging/SourceURL for how to handle this, and how to version the package.

- License  - LGPLv2+ - OK
- License field in spec matches - ***

"GPLv2+ and LGPLv2+"

I don't actually see any GPLv2+ code in the tarball.

- License file included in package - OK
- Spec in American English - OK
- Spec is legible. - OK
- Sources match upstream md5sum: - ***

See above re: snapshot packaging.

- Package needs ExcludeArch - Possibly pointless on s390, but don't really need to exclude it - OK
- BuildRequires correct  - OK
- Spec handles locales/find_lang - N/A
- Package is relocatable and has a reason to be. - N/A
- Package has %defattr and permissions on files is good. - OK
- Package has a correct %clean section. - OK
- Package has correct buildroot - OK
- Package is code or permissible content. - OK
- Doc subpackage needed/used. - OK
- Packages %doc files don't affect runtime. - OK

- Headers/static libs in -devel subpackage. - OK
- Spec has needed ldconfig in post and postun - OK
- .pc files in -devel subpackage/requires pkgconfig - OK
- .so files in -devel subpackage. - OK
- -devel package Requires: %{name} = %{version}-%{release} - OK
- .la files are removed. - N/A

- Package compiles and builds on at least one arch. - Tested x86_64 (w/mock)
- Package has no duplicate files in %files. - OK
- Package doesn't own any directories other packages own. - OK
- Package owns all the directories it creates. - OK
- No rpmlint output.

libiphone-devel.x86_64: W: no-documentation

Barring building the docs with doxygen, nothing to add here.

- final provides and requires are sane:

Looks good.

SHOULD Items:

- Should build in mock. - OK
- Should function as described. - No hardware, can't test
- Should have sane scriptlets. - OK
- Should have subpackages require base package with fully versioned depend. - OK
- Should have dist tag - OK
- Should package latest version - ***

See above re: source control pulls.

So, for approval:
- fix %{version} and source control URL to specify what revision you're pulling
- fix License: tag
- maybe tweak summary

If this is going to change ABI frequently without changing soname, a warning in the -devel package might be nice. Then again, if nothing other than the FUSE client is going to use the library, it may not be relevant.
Comment 2 Till Maas 2008-12-01 16:11:33 EST
Additionally to changing the state to ASSGINED and assigned to the bug to onself, fedora‑review should be set to "?" if a reviewer wants to review a request. I do this now.
Comment 3 Peter Robinson 2008-12-01 16:42:40 EST
(In reply to comment #1)
> - Meets Packaging Guidelines. - ***
> 
> Summary should probably drop the leading 'A'.

Will do

> You appear to be packaging a git snapshot. (In fact, you're including the
> entire .git directory in the tarball, which isn't really needed.) 
> 
> Please see https://fedoraproject.org/wiki/Packaging/SourceURL for how to handle
> this, and how to version the package.

I missed that. Will update it.
 
> - License  - LGPLv2+ - OK
> - License field in spec matches - ***
> 
> "GPLv2+ and LGPLv2+"
> 
> I don't actually see any GPLv2+ code in the tarball.

There was a COPYING and COPYING.LESSER file included which is why I marked it as such. If you think the COPYING file is irrelevant.

> - License file included in package - OK
> - Spec in American English - OK
> - Spec is legible. - OK
> - Sources match upstream md5sum: - ***
> 
> See above re: snapshot packaging.

ACK.

> - Should package latest version - ***
> 
> See above re: source control pulls.

ACK.

> So, for approval:
> - fix %{version} and source control URL to specify what revision you're pulling
> - fix License: tag
> - maybe tweak summary
> 
> If this is going to change ABI frequently without changing soname, a warning in
> the -devel package might be nice. Then again, if nothing other than the FUSE
> client is going to use the library, it may not be relevant.

Would the warning be contained in an included text file or in the description or somewhere else? I'm not sure whether the library would be used by conduit or rhythmbox or whether they'd use the ifuse client.

I'll update the file with the suggestions at update the ticket once complete.
Comment 4 Bill Nottingham 2008-12-01 16:55:37 EST
(In reply to comment #3)
> > - License  - LGPLv2+ - OK
> > - License field in spec matches - ***
> > 
> > "GPLv2+ and LGPLv2+"
> > 
> > I don't actually see any GPLv2+ code in the tarball.
> 
> There was a COPYING and COPYING.LESSER file included which is why I marked it
> as such. If you think the COPYING file is irrelevant.

What's listed in the headers of the code trumps whatever COPYING files are in the package itself.

> > If this is going to change ABI frequently without changing soname, a warning in
> > the -devel package might be nice. Then again, if nothing other than the FUSE
> > client is going to use the library, it may not be relevant.
> 
> Would the warning be contained in an included text file or in the description
> or somewhere else? I'm not sure whether the library would be used by conduit or
> rhythmbox or whether they'd use the ifuse client.

Maybe just in the README, if it's not already there.

If you don't know of any other users of the library, it's not really a big deal.
Comment 5 Peter Robinson 2008-12-01 17:22:51 EST
SPEC: http://pbrobinson.fedorapeople.org/libiphone.spec
SRPM: http://pbrobinson.fedorapeople.org/libiphone-0.1.0-2.git8c3a01e.fc10.src.rpm

Updated based on review. I think I've done the details on pulling the specific git version (its currently based on HEAD).

Removed the GPLv2+ tag and the COPYING file that specifies it as well, and the other listed bits.
Comment 6 Bill Nottingham 2008-12-01 22:58:45 EST
You've got two git-clone lines in the spec.

For making the tarball, you may want to do something like:

git-archive --format=tar --prefix libiphone-%{version}/ <ref> | gzip > libiphone-%{version}.tar.gz


For the version, you need to prefix it with a date, otherwise it won't sort right when the ref changes. xorg-x11-drv-nouveau has an example of this.
Comment 7 Peter Robinson 2008-12-02 04:59:46 EST
Thanks, they were the commands I couldn't find late last night. Third time lucky I think. Spec in same location. SRPM: http://pbrobinson.fedorapeople.org/libiphone-0.1.0-3.20081201git8c3a01e.fc10.src.rpm
Comment 8 Bill Nottingham 2008-12-02 12:38:53 EST
Looks good. APPROVED.
Comment 9 Peter Robinson 2008-12-02 14:04:35 EST
New Package CVS Request
=======================
Package Name: libiphone
Short Description: Library for connecting to Apple iPhone and iPod touch
Owners: pbrobinson
Branches: F-9 F-10
InitialCC:
Comment 10 Kevin Fenzi 2008-12-03 19:55:09 EST
cvs done.
Comment 11 Peter Robinson 2008-12-05 12:17:24 EST
Built and in rawhide. Thanks for the review.

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