Bug 1245932 - Review Request: shairplay - Apple airplay and raop protocol server
Summary: Review Request: shairplay - Apple airplay and raop protocol server
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: William Moreno
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-07-23 07:06 UTC by Mohamed El Morabity
Modified: 2015-10-23 17:22 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-10-23 17:22:49 UTC
Type: ---
Embargoed:
williamjmorenor: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
License Check (6.80 KB, text/plain)
2015-08-09 17:57 UTC, William Moreno
no flags Details

Description Mohamed El Morabity 2015-07-23 07:06:14 UTC
Spec URL: https://melmorabity.fedorapeople.org/packages/shairplay/shairplay.spec
SRPM URL: https://melmorabity.fedorapeople.org/packages/shairplay/shairplay-0.9.0-1.20150508git0f41ade.fc22.src.rpm
Description:
Free portable AirPlay server implementation similar to ShairPort. Currently only
AirPort Express emulation is supported.
Fedora Account System Username: melmorabity

Comment 1 William Moreno 2015-08-09 17:57:32 UTC
Created attachment 1060780 [details]
License Check

Comment 2 William Moreno 2015-08-09 20:22:31 UTC
Package Review
==============

Need Work:
?:    Sources contain only permissible code or content.
      The shairplay-key file is redistributalbe?

Fail: If the package is under multiple licenses, the licensing breakdown
      must be documented in the spec.
      Please a License Breakdown file as Source and include it with %license, 
      can the attached file.

?:    Package contains desktop file if it is a GUI application.
      This app runs with a graphical interfase??? If yes 
      you must include a name.deskptop and a name.appdata.xml file 

?:    Package contains systemd file(s) if in need.
      This app runs as a system service? If yes you must include a systemd
      unit file so user can start the service with 
      systemctl start shairplay 

Please fix these issues so I can check:

?:   Package complies to the Packaging Guidelines

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

Generic:
Pass: Package is licensed with an open-source compatible license and meets
      other legal requirements as defined in the legal section of Packaging
      Guidelines.
Pass: License field in the package spec file matches the actual license.
Pass: License file installed when any subpackage combination is installed.
Pass: %build honors applicable compiler flags or justifies otherwise.
Pass: Package contains no bundled libraries without FPC exception.
Pass: Changelog in prescribed format.
Pass: Development files must be in a -devel package
Pass: Package uses nothing in %doc for runtime.
Pass: Package consistently uses macros (instead of hard-coded directory
      names).
Pass: Package is named according to the Package Naming Guidelines.
Pass: Package does not generate any conflict.
Pass: Package obeys FHS, except libexecdir and /usr/target.
NA:   If the package is a rename of another package, proper Obsoletes and
      Provides are present.
Pass: Requires correct, justified where necessary.
Pass: Spec file is legible and written in American English.
Pass: Useful -debuginfo package or justification otherwise.
Pass: Package is not known to require an ExcludeArch tag.
Pass: Large documentation must go in a -doc subpackage. 
Pass: Package successfully compiles and builds into binary rpms on at least
      one supported primary architecture.
Pass: Package installs properly.
Pass: Rpmlint is run on all rpms the build produces.
Pass: 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 %license.
Pass: Package requires other packages for directories it uses.
Pass: Package must own all directories that it creates.
Pass: Package does not own files or directories owned by other packages.
Pass: All build dependencies are listed in BuildRequires, except for any
      that are listed in the exceptions section of Packaging Guidelines.
Pass: Package uses either %{buildroot} or $RPM_BUILD_ROOT
Pass: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
      beginning of %install.
Pass: Macros in Summary, %description expandable at SRPM build time.
Pass: Dist tag is present.
Pass: Package does not contain duplicates in %files.
Pass: Permissions on files are set properly.
Pass: Package use %makeinstall only when make install DESTDIR=... doesn't
      work.
Pass: Package is named using only allowed ASCII characters.
Pass: Package does not use a name that already exists.
Pass: Package is not relocatable.
Pass: Sources used to build the package match the upstream source, as
      provided in the spec URL.
Pass: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
Pass: File names are valid UTF-8.
Pass: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====
Generic:
NA:   If the source package does not include license text(s) as a separate
      file from upstream, the packager SHOULD query upstream to include it.
Pass: Final provides and requires are sane (see attachments).
Na:   Fully versioned dependency in subpackages if applicable.
Pass: Latest version is packaged.
Pass: Package does not include license text files separate from upstream.
Pass: Scriptlets must be sane, if used.
NA:   Description and summary sections in the package spec file contains
      translations for supported Non-English languages, if available.
Pass: Package should compile and build into binary rpms on all supported
      architectures.
      http://koji.fedoraproject.org/koji/taskinfo?taskID=10657084
NA:   %check is present and all tests pass.
Pass: Packages should try to preserve timestamps of original installed
      files.
Pass: Packager, Vendor, PreReq, Copyright tags should not be in spec file
Pass: Sources can be downloaded from URI in Source: tag
Pass: Reviewer should test that the package builds in mock.
Pass: Buildroot is not present
Pass: Package has no %clean section with rm -rf %{buildroot} (or
      $RPM_BUILD_ROOT)
Pass: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
Pass: Uses parallel make %{?_smp_mflags} macro.
Pass: SourceX is a working URL.
Pass: Spec use %global instead of %define unless justified.

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

Rpmlint
-------
Checking: shairplay-0.9.0-1.20150508git0f41ade.fc21.x86_64.rpm
          shairplay-devel-0.9.0-1.20150508git0f41ade.fc21.x86_64.rpm
          shairplay-0.9.0-1.20150508git0f41ade.fc21.src.rpm
shairplay.x86_64: W: spelling-error Summary(en_US) raop -> rap, rasp, ramp
shairplay-devel.x86_64: W: only-non-binary-in-usr-lib
shairplay-devel.x86_64: W: no-documentation
shairplay.src: W: spelling-error Summary(en_US) raop -> rap, rasp, ramp
3 packages and 0 specfiles checked; 0 errors, 4 warnings.

Rpmlint (debuginfo)
-------------------
Checking: shairplay-debuginfo-0.9.0-1.20150508git0f41ade.fc21.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Rpmlint (installed packages)
----------------------------
shairplay.x86_64: W: spelling-error Summary(en_US) raop -> rap, rasp, ramp
shairplay.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libshairplay.so.0.0.0 /lib64/libm.so.6
shairplay-devel.x86_64: W: only-non-binary-in-usr-lib
shairplay-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 4 warnings.

Requires
--------
shairplay (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    rtld(GNU_HASH)

shairplay-devel (rpmlib, GLIBC filtered):
    libshairplay.so.0()(64bit)
    shairplay(x86-64)

Provides
--------
shairplay:
    libshairplay.so.0()(64bit)
    shairplay
    shairplay(x86-64)

shairplay-devel:
    shairplay-devel
    shairplay-devel(x86-64)

Source checksums
----------------
https://github.com/juhovh/shairplay/archive/0f41ade/shairplay-0f41ade.tar.gz :
  CHECKSUM(SHA256) this package     : 4d104fbf3d95999bd15c72537559a1fdcfd02ee2463fb79bade91ab255188b18
  CHECKSUM(SHA256) upstream package : 4d104fbf3d95999bd15c72537559a1fdcfd02ee2463fb79bade91ab255188b18


AutoTools: Obsoleted m4s found
------------------------------
  AC_PROG_LIBTOOL found in: shairplay-
  0f41ade2678f374aa8446d127d6aa9d5a3d428da/configure.ac:14

Comment 3 Mohamed El Morabity 2015-08-10 09:38:09 UTC
(In reply to William Moreno from comment #2)
> Package Review
> ==============
> 
> Need Work:
> ?:    Sources contain only permissible code or content.
>       The shairplay-key file is redistributalbe?
Which file exactly?
> 
> Fail: If the package is under multiple licenses, the licensing breakdown
>       must be documented in the spec.
>       Please a License Breakdown file as Source and include it with
> %license, 
>       can the attached file.
The license file in %license describes exactly the license of each part of the source code.


> ?:    Package contains desktop file if it is a GUI application.
>       This app runs with a graphical interfase??? If yes 
>       you must include a name.deskptop and a name.appdata.xml file 
Please consider the description and the content of the package:
- it's a library
- there's no executable provided by this library.


> ?:    Package contains systemd file(s) if in need.
>       This app runs as a system service? If yes you must include a systemd
>       unit file so user can start the service with 
>       systemctl start shairplay 
See above (once again, it's nothing else than a library).

> shairplay.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libshairplay.so.0.0.0 /lib64/libm.so.6
Not a blocker, fortunately.

> AutoTools: Obsoleted m4s found
> ------------------------------
>   AC_PROG_LIBTOOL found in: shairplay-
>   0f41ade2678f374aa8446d127d6aa9d5a3d428da/configure.ac:14
Not a blocker too.

Comment 4 Mohamed El Morabity 2015-08-10 09:44:27 UTC
Please forget about my previous comments! I've just realized I only build the libraries, whereas the project provides indeed a server. I will enable the server build, as well as the AirTV GUI, it's worth providing all the features of the project. Thanks for your interest and your tips on this review.

Comment 5 William Moreno 2015-08-10 16:06:18 UTC
(In reply to Mohamed El Morabity from comment #3)
> > ?:    Sources contain only permissible code or content.
> >       The shairplay-key file is redistributalbe?
> Which file exactly?

I mean this file:
https://github.com/juhovh/shairplay/blob/master/airport.key

But I see tha it is a si,ĺe RSA Key

> > Fail: If the package is under multiple licenses, the licensing breakdown
> >       must be documented in the spec.
> >       Please a License Breakdown file as Source and include it with
> > %license, 
> >       can the attached file.
> The license file in %license describes exactly the license of each part of
> the source code.
Looks fine.

Comment 6 Mohamed El Morabity 2015-08-17 11:56:01 UTC
New release:
SPEC URL: https://fedorapeople.org/~melmorabity/packages/shairplay/shairplay.spec
SRPM URL: https://fedorapeople.org/~melmorabity/packages/shairplay/shairplay-0.9.0-2.20150508git0f41ade.fc22.src.rpm

Changes:
- the shairplay server is now enabled (systemd service file provided as well)
- the AirTV GUI is enabled too

About the key, it was extracted from an Airport Express device (see https://mailman.videolan.org/pipermail/vlc-devel/2011-April/079148.html). I don't know if it could be considered to be a DMCA violation.
I request a FE-Legal review.

Comment 7 William Moreno 2015-08-17 15:01:41 UTC
Please note than if you want the airtv app to be included fine in gnome-software you SHOULD ad a airtv.appdata.xlm file, please see:
https://fedoraproject.org/wiki/Packaging:AppData

There is a icon file in .svg, I will recomend than you SHOULLD do a better treatment, add
BuildRequires:  libpng-devel
BuildRequires:  librsvg2-devel
BuildRequires:  librsvg2-tools
BuildRequires:  ImageMagick
Requires:       hicolor-icon-theme

And in %%install run something like this:
# Generate resized icons
pushd icons
mkdir -p %{buildroot}/%{_datadir}/icons/hicolor/{16x16,22x22,24x24,32x32,48x48,64x64,72x72,96x96,128x128,scalable}/apps
for s in 16x16 22x22 24x24 32x32 48x48 64x64 72x72 96x96 128x128
do
    convert ./airtv.svg -resize $s %{buildroot}/%{_datadir}/icons/hicolor/$s/apps/airtv.png;
done
install -p -m 0644 retext.svg %{buildroot}/%{_datadir}/icons/hicolor/scalable/apps
popd

You must add this scripts to handle the .desktopfile:

%post
/usr/bin/update-desktop-database &> /dev/null || :

%postun
/usr/bin/update-desktop-database &> /dev/null || :

But the Legal comments before conitue working in this.

By the way good job in the subpackaging of the gui and the service

Comment 8 Tom "spot" Callaway 2015-09-14 14:02:37 UTC
Legal review concluded that there are no issues with this package (and its components). Lifting FE-Legal.

Comment 9 William Moreno 2015-09-16 14:53:19 UTC
Cool.

So please:

1- You SHOULD include the appdata.xlm file, if so, you must include:

BuildRequires: libappstream-glib
%%check
appstream-util validate-relax --nonet %{buildroot}/%{_datadir}/appdata/*.appdata.xml

2- Handle the desktop file with

Requires(post): desktop-file-utils
Requires(postun): desktop-file-utils

%post
/usr/bin/update-desktop-database &> /dev/null || :

%postun
/usr/bin/update-desktop-database &> /dev/null || :

3- You a missing this:
BuildRequires: systemd-units

4- Please use the proposed tools to handle the icons in the package and do not forget to include:

%post
/bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :

%postun
if [ $1 -eq 0 ] ; then
    /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
    /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
fi

%posttrans
/usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :

5- Upstream include a test in the repo:
https://github.com/juhovh/shairplay/tree/master/src/test

So you must run this in %%check

6- Here is a html documentation tha you can include in a airtv-docs subpackage:
https://github.com/juhovh/shairplay/tree/master/AirTV-Qt/qtsingleapplication/doc/html

7- There will be a rpmlint warning about a missing manpage, if can include a manpage will good, you can create one using the man-editor package available in repos or writting one with a text editor using the groff format.

Comment 10 William Moreno 2015-09-16 14:56:27 UTC
I did not find blundled libs in the github repo, so this is a great +1 to upstream

Comment 11 Mohamed El Morabity 2015-09-24 09:25:06 UTC
New release: 
Spec file: https://melmorabity.fedorapeople.org/packages/shairplay/shairplay.spec
SRPM file: https://melmorabity.fedorapeople.org/packages/shairplay/shairplay-0.9.0-3.20150921git498bc5b.fc22.src.rpm

Changes:
- Update to latest snapshot
- Add missing runtime dependency on Avahi
- Add AppData file for AirTV
- Move icon to %{_datadir}/icons/hicolor

(In reply to William Moreno from comment #9)
> 1- You SHOULD include the appdata.xlm file, if so, you must include:
Done

> 2- Handle the desktop file with
> 
> Requires(post): desktop-file-utils
> Requires(postun): desktop-file-utils
> 
> %post
> /usr/bin/update-desktop-database &> /dev/null || :
> 
> %postun
> /usr/bin/update-desktop-database &> /dev/null || :
This is only needed when the desktop file has MimeType keys:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
There's no such key in AirTV desktop file.
 
> 3- You a missing this:
> BuildRequires: systemd-units
A BR on systemd is enough, according to:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
By the way, only the systemd package carries the systemd RPM macro definitions.

> 4- Please use the proposed tools to handle the icons in the package and do
> not forget to include:
> 
> %post
> /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :
> 
> %postun
> if [ $1 -eq 0 ] ; then
>     /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
>     /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
> fi
As long as a SVG icon is provided, it will work for all sizes, making bitmap icons useless. Today all desktop environments provided by Fedora support SVG icons.
Anyway, I moved the icon to %{_datadir}/icons/hicolor, as recommended by the FreeDesktop specs.


> 5- Upstream include a test in the repo:
> https://github.com/juhovh/shairplay/tree/master/src/test
> 
> So you must run this in %%check
Good catch :). Unfortunately I was unable to run those tests, as if they're not compatible anymore with the latest shairplay API.

> 6- Here is a html documentation tha you can include in a airtv-docs
> subpackage:
> https://github.com/juhovh/shairplay/tree/master/AirTV-Qt/qtsingleapplication/
> doc/html
This doc. is related to Qt developement, nor AirTV neither shairplay.
 
> 7- There will be a rpmlint warning about a missing manpage, if can include a
> manpage will good, you can create one using the man-editor package available
> in repos or writting one with a text editor using the groff format.
Not a blocker :)

Comment 12 William Moreno 2015-09-25 18:54:31 UTC
1- This app build in all suported Fedoras:
http://copr.fedoraproject.org/coprs/williamjmorenor/fedora-review-test/monitor/

2- This app build in 
ARM:   http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=3201533
PPC:   http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=2768443
s390:  http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=1951448

I did not find bundled libs in the source (a bit +1 to upstream) and I do not find bloquers so I will re run a fedora-review because the spec change a lot since last full review.

Comment 13 William Moreno 2015-09-25 19:04:20 UTC
The appdate dile says:

AirTV is a Qt GUI to start a RAOP server. AirTV Qt is a GUI to start a RAOP server. Once started, AirTV will add an icon to the system tray using  which you can stop the server.

Please remove the duplicate line, note than this is not a bloquer but fix it before send the build to koji

Comment 14 William Moreno 2015-09-25 19:24:47 UTC
PACKAGE APROVED
===============

Comment 15 Mohamed El Morabity 2015-09-29 07:57:45 UTC
Mant thanks for this good review William :).

(In reply to William Moreno from comment #13)
> The appdate dile says:
> 
> AirTV is a Qt GUI to start a RAOP server. AirTV Qt is a GUI to start a RAOP
> server. Once started, AirTV will add an icon to the system tray using  which
> you can stop the server.
> 
> Please remove the duplicate line, note than this is not a bloquer but fix it
> before send the build to koji
This will be fixed at package import.

Comment 16 Mohamed El Morabity 2015-09-29 08:00:10 UTC
New Package SCM Request
=======================
Package Name: shairplay
Short Description: Apple AirPlay and RAOP protocol server
Upstream URL: https://github.com/juhovh/shairplay/
Owners: melmorabity
Branches: f22 f23
InitialCC:

Comment 17 Gwyn Ciesla 2015-09-29 19:28:16 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2015-09-30 05:05:14 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-00189d7032

Comment 19 Fedora Update System 2015-10-01 23:53:52 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update shairplay'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-00189d7032

Comment 20 Fedora Update System 2015-10-02 01:49:43 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update shairplay'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-00189d7032

Comment 21 Fedora Update System 2015-10-12 08:29:00 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-61fb29333e

Comment 22 Fedora Update System 2015-10-13 00:06:09 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2015-10-13 18:21:27 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update shairplay'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-61fb29333e

Comment 24 Fedora Update System 2015-10-23 17:22:46 UTC
shairplay-0.9.0-3.20150921git498bc5b.fc22 has been pushed to the Fedora 22 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.