Bug 888224

Hi Lorenzo. 
Just some initial comments.

- I have some dubious about its license; it seems a MIT

- Source0 should indicate an entire link to the source package or how it has been created. 
https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL

- Description is not correct in my opinion. Better:

'This program emulates an AirPort Express for the purpose of streaming music from iTunes and compatible iPods. It implements a server for the Apple RAOP protocol.
ShairPort does not support AirPlay v2 (video and photo streaming).

It supports multiple simultaneous streams, if your audio output chain (as detected by libao) does so'

- This line 'rm -rf $RPM_BUILD_ROOT' can be omitted

- %doc line is empty. Why ?

I'll be sponsoring Lorenzo since this is a dependency for another package I maintain (xbmc).

Re: comment 1

Good suggestions.

With regard to the license, this one is important:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Hint: There are a few more packaging mistakes. Not necessarily covered by the guidelines, however.

[...]

Ken, would you mind setting the fedora-review flag appropriately?
https://fedoraproject.org/wiki/Package_Review_Process#Reviewer

(In reply to comment #1)

> - I have some dubious about its license; it seems a MIT

License has been corrected to MIT.

> - Source0 should indicate an entire link to the source package or how it has
> been created. 
> https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL

Added a description of how to generate source tarball.
 
> - Description is not correct in my opinion. Better:
> 
> 'This program emulates an AirPort Express for the purpose of streaming music
> from iTunes and compatible iPods. It implements a server for the Apple RAOP
> protocol.
> ShairPort does not support AirPlay v2 (video and photo streaming).
> 
> It supports multiple simultaneous streams, if your audio output chain (as
> detected by libao) does so'

I have to disagree: the description you pointed out is the one regarding original shairport tool that is not the same as libshairport.

> - This line 'rm -rf $RPM_BUILD_ROOT' can be omitted

Removed.
 
> - %doc line is empty. Why ?

My mistake, %doc line is now populated.

I have uploaded corrected version of spec file and srpm, also available on fedorapeople.org:

http://lorenzodalrio.fedorapeople.org/libshairport.spec
http://lorenzodalrio.fedorapeople.org/libshairport-1.2.1-1.20121218git16395d8.fc17.src.rpm

Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4821268

(In reply to comment #2)
> I'll be sponsoring Lorenzo since this is a dependency for another package I
> maintain (xbmc).

Thank You Ken!

Well, your current %description doesn't tell what libshairport is and what it _does_. It gives a history lesson ;), but instead it ought to _expand_ on the %summary and tell what this library does.

https://github.com/amejia1/libshairport#readme

(In reply to comment #6)
> Well, your current %description doesn't tell what libshairport is and what
> it _does_. It gives a history lesson ;), but instead it ought to _expand_ on
> the %summary and tell what this library does.
> 
> https://github.com/amejia1/libshairport#readme

Ok, corrected version uploaded. :)

(In reply to comment #3)
> Ken, would you mind setting the fedora-review flag appropriately?
> https://fedoraproject.org/wiki/Package_Review_Process#Reviewer

Thanks for the reminder. Set. I'll review by Monday (Dec 31st).

Created attachment 670170 [details]
patch to remove libssl link

Hi Lorenzo,

Good job on your package. I noticed five things to update. Please fix the first three issues at least.

The first thing is that rpmlint caught an unnecessary link in libshairport.so.2.0.0. The configure script adds libssl to the link flags, and as far as I can see, that library is not strictly necessary [1]. libshairport only uses OpenSSL's functions in libcrypto. It's not clear to me why this was ever linked in the first place. Looking at the git history of configure.in, it seems this libssl link was left over from the time that the code existed in the main xbmc tree. I'm attaching a patch which seems to fix this problem. Please coordinate with upstream to see if the author will accept it; otherwise, you may want to have the patch in your package.

The second thing is that the -devel subpackage contains two files that are unnecessary. Please remove "LICENSE" and "README" files from the -devel subpackage. You only need to ship them in the main package. [2]

The third thing is that the Summary: field should be capitalized, so change "emulates" to "Emulates".

The fourth thing is that rpmlint complains about permissions on the generate-tarball script. You can change the permissions on the generate-tarball script from 775 to 755 to silence rpmlint's warning.

The fifth thing is that rpmlint complains using %define instead of %global. The reason for using %global is explained in a packaging draft [3].

Lastly, and this is just a process thing: each time you make a change to the package, please update your "Release" field and changelog and post new links in Bugzilla. This will help to clarify to reviewers that you have changed the package. (This convention is sort of undocumented, so I've started a discussion about formalizing this on -devel [4].)

Overall you have a good package that basically meets the packaging guidelines.

[1] https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency
[2] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
[3] https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
[4] https://lists.fedoraproject.org/pipermail/devel/2012-December/175698.html

Full review follows.

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Yes, MIT.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: Rpmlint is run on all rpms the build produces.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 20480 bytes in 4 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[-]: Package functions as described.
Untested.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL. Comments contain instructions.
[x]: SourceX / PatchY prefixed with %{name}.
[x]: SourceX is a working URL.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
     Upstream has no "check" rule to run.
[x]: Packages should try to preserve timestamps of original installed files.
[!]: Spec use %global instead of %define.
     Note: %define checkout 20121218git16395d8

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.


Rpmlint
-------
Checking: libshairport-1.2.1-1.20121218git16395d8.fc17.src.rpm
          libshairport-devel-1.2.1-1.20121218git16395d8.fc17.i686.rpm
          libshairport-debuginfo-1.2.1-1.20121218git16395d8.fc17.i686.rpm
          libshairport-1.2.1-1.20121218git16395d8.fc17.i686.rpm
libshairport.src: W: summary-not-capitalized C emulates an AirPort Express
libshairport.src: W: spelling-error %description -l en_US iPods -> i Pods, iPod, iPod's
libshairport.src: W: spelling-error %description -l en_US libao -> libation
libshairport.src: W: strange-permission libshairport-generate-tarball-gz.sh 0775L
libshairport.src: W: invalid-url Source0: libshairport-1.2.1.20121218git16395d8.tar.gz
libshairport.i686: W: summary-not-capitalized C emulates an AirPort Express
libshairport.i686: W: spelling-error %description -l en_US iPods -> i Pods, iPod, iPod's
libshairport.i686: W: spelling-error %description -l en_US libao -> libation
4 packages and 0 specfiles checked; 0 errors, 8 warnings.



Rpmlint (installed packages)
----------------------------
# rpmlint libshairport-debuginfo libshairport-devel libshairport
libshairport.i686: W: summary-not-capitalized C emulates an AirPort Express
libshairport.i686: W: spelling-error %description -l en_US iPods -> i Pods, iPod, iPod's
libshairport.i686: W: spelling-error %description -l en_US libao -> libation
libshairport.i686: W: unused-direct-shlib-dependency /usr/lib/libshairport.so.2.0.0 /lib/libssl.so.10
3 packages and 0 specfiles checked; 0 errors, 4 warnings.


Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16
Buildroot used: fedora-17-i386
Command line :/usr/bin/fedora-review -b 888224

> Group:          Development/Libraries

It's possible to omit the Group tag, but if it's filled in, base libraries enter group "System Environment/Libraries". Only the corresponding -devel package enters group "Development/Libraries".


> %package        devel

"Requires: libao-devel%{?_isa}" is missing, because the libshairport header includes "ao" headers.


> src/alac.c

is an ALAC decoder from 2005. What's the current legal situation here with regard to Apple having published their codec as Open Source Software last year?

Hi Ken,
thank you.

> The first thing is that rpmlint caught an unnecessary link in libshairport.so.2.0.0.

I have included your patch in the package and proposed it upstream too: https://github.com/amejia1/libshairport/pull/3
If it will be included upstream i will remove it from package.

> The second thing is that the -devel subpackage contains two files that are unnecessary.

Files removed.

> The third thing is that the Summary: field should be capitalized, so change "emulates" to "Emulates".

Done.

> The fourth thing is that rpmlint complains about permissions on the generate-tarball script.

Permissions changed.

> The fifth thing is that rpmlint complains using %define instead of %global.

Changed from %define to %global.


Hi Michael,

> It's possible to omit the Group tag, but if it's filled in, base libraries enter group "System Environment/Libraries". Only the corresponding -devel package enters group "Development/Libraries".

I have corrected groups membership.

> "Requires: libao-devel%{?_isa}" is missing, because the libshairport header includes "ao" headers.

Requires added.

> is an ALAC decoder from 2005. What's the current legal situation here with regard to Apple having published their codec as Open Source Software last year?

I don't know about this, can anyone help?

New spec and srpm versions:
Spec URL: http://lorenzodalrio.fedorapeople.org/libshairport.spec
SRPM URL: http://lorenzodalrio.fedorapeople.org/libshairport-1.2.1-2.20121218git16395d8.fc17.src.rpm
Successful koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4826859

(In reply to comment #11)
> > is an ALAC decoder from 2005. What's the current legal situation here with regard to Apple having published their codec as Open Source Software last year?
> 
> I don't know about this, can anyone help?

The ASL does contain a patent grant to users of the official ALAC code [1]. However, alac.c is a third-party implementation developed back in 2005, so I don't know what the status is. I'm blocking FE-LEGAL to see what the experts say. Is this alac.c [2] ok to have in Fedora? Or is it infringing on any Apple patents?

[1] http://lists.apple.com/archives/coreaudio-api/2011/Oct/msg00150.html
[2] https://github.com/amejia1/libshairport/blob/master/src/alac.c

David Hammerton's ALAC decoder is not okay for Fedora, because of legal concerns.

The code at http://alac.macosforge.org is however under a Patent grant from Apple via the Apache License, so it would be acceptable. (That grant does not apply to the Hammerton implementation.)

The Apple ALAC code compiles into a static library with their Makefile, but it is almost certainly possible to make it into a shared library with a few minor Makefile changes. Then, libshairport could be patched to use it.

Alternately, you could delete the ALAC code from libshairport, though I'm not sure if that will render it useless or not.

Sorry to be a buzzkill. Leaving this FE-Legal blocked for now.

Thank you spot for the prompt analysis. It looks like either option will require coding and work upstream. Since this library is mainly designed to support xbmc, let's move it to RPM Fusion.

Thank you Tom, thank you Ken.

Opened bug 2631 [1] on rpm fusion bugzilla.

[1] https://bugzilla.rpmfusion.org/show_bug.cgi?id=2631

I'm closing this as CANTFIX for now, since the package is in RPM Fusion.