Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: libiphone - A library for connecting to Apple iPhone and iPod touch
https://bugzilla.redhat.com/show_bug.cgi?id=473590
Summary: Review Request: libiphone - A library for connecting to Apple iPhone and iPod touch Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: pbrobinson@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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
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=473590
Peter Robinson pbrobinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |473591
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=473590
Bill Nottingham notting@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |notting@redhat.com
--- Comment #1 from Bill Nottingham notting@redhat.com 2008-12-01 15:43:12 EDT --- 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.
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=473590
Till Maas opensource@till.name changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |opensource@till.name Flag| |fedora-review?
--- Comment #2 from Till Maas opensource@till.name 2008-12-01 16:11:33 EDT --- 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.
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=473590
--- Comment #3 from Peter Robinson pbrobinson@gmail.com 2008-12-01 16:42:40 EDT --- (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.
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=473590
--- Comment #4 from Bill Nottingham notting@redhat.com 2008-12-01 16:55:37 EDT --- (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.
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=473590
--- Comment #5 from Peter Robinson pbrobinson@gmail.com 2008-12-01 17:22:51 EDT --- 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.
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=473590
--- Comment #6 from Bill Nottingham notting@redhat.com 2008-12-01 22:58:45 EDT --- 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.
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=473590
--- Comment #7 from Peter Robinson pbrobinson@gmail.com 2008-12-02 04:59:46 EDT --- 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...
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=473590
Bill Nottingham notting@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #8 from Bill Nottingham notting@redhat.com 2008-12-02 12:38:53 EDT --- Looks good. APPROVED.
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=473590
Peter Robinson pbrobinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #9 from Peter Robinson pbrobinson@gmail.com 2008-12-02 14:04:35 EDT --- 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:
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=473590
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #10 from Kevin Fenzi kevin@tummy.com 2008-12-03 19:55:09 EDT --- cvs done.
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=473590
Peter Robinson pbrobinson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |RAWHIDE
--- Comment #11 from Peter Robinson pbrobinson@gmail.com 2008-12-05 12:17:24 EDT --- Built and in rawhide. Thanks for the review.
package-review@lists.fedoraproject.org