Bug 1509568

Not a formal review but a couple of points after reading the SPEC:

 - Don't add:

%dir %{_datadir}/icons/hicolor/scalable
%dir %{_datadir}/icons/hicolor/scalable/apps

   These directories should be owned by the Requires to hicolor-icon-theme

 - Please note last week change in the guidelines regarding Appdata. It was announced on the devel-announce mailing list:

Appstream metadata guidelines were updated to reflect the new location
into which appdata files should be placed.

* https://fedoraproject.org/wiki/Packaging:AppData
* https://pagure.io/packaging-committee/issue/704

   Per the new guidelines, appdata files must now be installed in %{_datadir}/metainfo/ instead of %{_datadir}/appdata/

 - You must run the Icon cache scriplet: https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Icon_Cache


%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 || :

 - Add the changelog and authors to %doc:

%doc AUTHORS.md CHANGELOG.md README.md

 - Use this simplified URL:

Source0:        https://github.com/ColinDuquesnoy/%{name}/archive/%{version}/%{name}-%{version}.tar.gz

 - Regarding:

# Build is broken on:
ExcludeArch: ppc64le ppc64 s390x

Build is not "broken", MellowPlayer is using Qt Web Engine, which is only available on some arches. However this is not how you should handle it, instead we have a specific macro:

ExclusiveArch:  %{qt5_qtwebengine_arches}

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/mellowplayer.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/mellowplayer-3.1.0-2.fc27.src.rpm

%changelog
* Sun Nov 05 2017 Martin Gansser <martinkg> - 3.1.0-2
- Don't add: %%dir %%{_datadir}/icons/hicolor/scalable and
             %%dir %%{_datadir}/icons/hicolor/scalable/apps
  These directories should be owned by the Requires to hicolor-icon-theme
- Per the new guidelines, appdata files must now be installed in
  %%{_datadir}/metainfo/ instead of %%{_datadir}/appdata/
- Add Icon cache scriplet
- Add changelog and authors to %%doc
- Use simplified URL
- Use ExclusiveArch: %%{qt5_qtwebengine_arches} due Qt Web Engine is only
  available on some arches

- Package uses either %{buildroot} or $RPM_BUILD_ROOT
  Note: Using both %{buildroot} and $RPM_BUILD_ROOT
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros
- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 17756160 bytes in 107 files.
  See:
  http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/mellowplayer.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/mellowplayer-3.1.0-3.fc27.src.rpm

%changelog
* Sun Nov 05 2017 Martin Gansser <martinkg> - 3.1.0-3
- Use %%{buildroot} macro for consistency
- Large documentation must go in a -doc subpackage

You should keep %{_mandir}/man1/%{rname}.1.* along with the binary in the main package.


Package accepted otherwise..

(In reply to Robert-André Mauchin from comment #5)
> You should keep %{_mandir}/man1/%{rname}.1.* along with the binary in the
> main package.

done, no extra package upload.
> 
> Package accepted otherwise..

thanks for the review

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mellowplayer