Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1509568 - Review Request: mellowplayer - Cloud music integration for your desktop
Summary: Review Request: mellowplayer - Cloud music integration for your desktop
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin
QA Contact: Fedora Extras Quality Assurance
URL: https://colinduquesnoy.github.io/Mell...
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-11-04 19:26 UTC by MartinKG
Modified: 2018-01-23 12:01 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-01-23 12:01:32 UTC
zebob.m: fedora-review+


Attachments (Terms of Use)

Description MartinKG 2017-11-04 19:26:37 UTC
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/mellowplayer.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/mellowplayer-3.1.0-1.fc27.src.rpm

Description: MellowPlayer is a free, open source and cross-platform desktop application that integrates online music services with your desktop.

Fedora Account System Username: martinkg

Comment 1 Robert-André Mauchin 2017-11-04 22:37:08 UTC
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}

Comment 2 MartinKG 2017-11-05 11:26:41 UTC
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@fedoraproject.org> - 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

Comment 3 Robert-André Mauchin 2017-11-05 15:19:38 UTC
- 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

Comment 4 MartinKG 2017-11-05 17:31:39 UTC
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@fedoraproject.org> - 3.1.0-3
- Use %%{buildroot} macro for consistency
- Large documentation must go in a -doc subpackage

Comment 5 Robert-André Mauchin 2017-11-05 23:03:13 UTC
You should keep %{_mandir}/man1/%{rname}.1.* along with the binary in the main package.


Package accepted otherwise..

Comment 6 MartinKG 2017-11-06 08:44:05 UTC
(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

Comment 7 Gwyn Ciesla 2017-11-06 15:03:10 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mellowplayer


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