Just two quick notes moment:

1) I am not sure whether the "Recommends: flash-plugin" is legit, i.e. if you can reference 3rd party repositories. You should probably consult this with some ML (legal?, packaging?, devel?).

2) Since this is add-on package, it should contain add-on metadata [1]. This way, Gnome Software will be able to list this add-on side by side with Nuvola player, which is pretty convenient.



[1]: https://fedoraproject.org/wiki/Packaging:AppData#.metainfo.xml_file_creation

(In reply to Vít Ondruch from comment #1)
> Just two quick notes moment:
> 
> 1) I am not sure whether the "Recommends: flash-plugin" is legit, i.e. if
> you can reference 3rd party repositories. You should probably consult this
> with some ML (legal?, packaging?, devel?).

I have started a discussion on the packaging forum [3].
> 
> 2) Since this is add-on package, it should contain add-on metadata [1]. This
> way, Gnome Software will be able to list this add-on side by side with
> Nuvola player, which is pretty convenient.
> 
> 
> [1]:
> https://fedoraproject.org/wiki/Packaging:AppData#.metainfo.xml_file_creation

I added a metainfo.xml file to the spec file.

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvola-app-amazon-cloud-player.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvola-app-amazon-cloud-player-5.4-2.fc25.src.rpm

%changelog
* Fri May 26 2017 Martin Gansser <martinkg> - 5.4-2
- Add %%{name}.metainfo.xml file
- Add BR libappstream-glib

[3] https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/RIYNP4CERE3VZ3TLPSOQQJVNHJMG7EYC/

I'll take this for a review.

(In reply to mgansser from comment #2)
> (In reply to Vít Ondruch from comment #1)
> > Just two quick notes moment:
> > 
> > 1) I am not sure whether the "Recommends: flash-plugin" is legit, i.e. if
> > you can reference 3rd party repositories. You should probably consult this
> > with some ML (legal?, packaging?, devel?).
> 
> I have started a discussion on the packaging forum [3].

As far as I understand the ML discussion, the references to 3rd party     repositories are not allowed in Fedora. Please drop the "Recommends: flash-plugin"

> > 
> > 2) Since this is add-on package, it should contain add-on metadata [1]. This
> > way, Gnome Software will be able to list this add-on side by side with
> > Nuvola player, which is pretty convenient.
> > 
> > 
> > [1]:
> > https://fedoraproject.org/wiki/Packaging:AppData#.metainfo.xml_file_creation
> 
> I added a metainfo.xml file to the spec file.

Thx. Have you tried to submit the file upstream?

Unfortunately, the build does not work at all:

https://koji.fedoraproject.org/koji/taskinfo?taskID=19799178

This seems to be related to the changes announced by upstream [1] and probably triggered by the nuvolasdk. This makes me think about providing either some helper macros, which would hide this changes, providing something like nuvolaplayer-filesystem or possibly requirement to be more strict about versioning of the interdependencies.



[1] https://medium.com/nuvola-news/nuvola-4-4-supports-appindicators-in-unity-elementaryos-and-gnome-changes-versioning-scheme-a323b80e5cda#8450

(In reply to Vít Ondruch from comment #4)
> (In reply to mgansser from comment #2)
> > (In reply to Vít Ondruch from comment #1)
> > > Just two quick notes moment:
> > > 
> > > 1) I am not sure whether the "Recommends: flash-plugin" is legit, i.e. if
> > > you can reference 3rd party repositories. You should probably consult this
> > > with some ML (legal?, packaging?, devel?).
> > 
> > I have started a discussion on the packaging forum [3].
> 
> As far as I understand the ML discussion, the references to 3rd party    
> repositories are not allowed in Fedora. Please drop the "Recommends:
> flash-plugin"
 
done

> > > 
> > > 2) Since this is add-on package, it should contain add-on metadata [1]. This
> > > way, Gnome Software will be able to list this add-on side by side with
> > > Nuvola player, which is pretty convenient.
> > > 
> > > 
> > > [1]:
> > > https://fedoraproject.org/wiki/Packaging:AppData#.metainfo.xml_file_creation
> > 
> > I added a metainfo.xml file to the spec file.
> 
> Thx. Have you tried to submit the file upstream?

yes
Ticket: https://github.com/tiliado/nuvola-app-amazon-cloud-player/issues/24

> 
> Unfortunately, the build does not work at all:
> 
> https://koji.fedoraproject.org/koji/taskinfo?taskID=19799178
> 
I know this

> This seems to be related to the changes announced by upstream [1] and
> probably triggered by the nuvolasdk. This makes me think about providing
> either some helper macros, which would hide this changes, providing
> something like nuvolaplayer-filesystem or possibly requirement to be more
> strict about versioning of the interdependencies.
> 
> 
> [1]
> https://medium.com/nuvola-news/nuvola-4-4-supports-appindicators-in-unity-
> elementaryos-and-gnome-changes-versioning-scheme-a323b80e5cda#8450

good idea

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvola-app-amazon-cloud-player.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvola-app-amazon-cloud-player-5.4-3.fc25.src.rpm

%changelog
* Fri Jun 16 2017 Martin Gansser <martinkg> - 5.4-3
- remove Recommends: flash-plugin
- rename RR nuvolaplayer-devel to nuvolaruntime-devel

* Build issues
  - It seems that you need to add BR on python3-six to fix the build. I think
    the issue is caused by this [1] commit.
  - BTW the same commit breaks Diorite [2]. The python3-pyparsing should be
    required unconditionally everywhere. I still don't understand what was the
    difference between f26/f27, that it was not needed, but it probably
    does not matter anyway ...

* nuvolaruntime-devel dependency.
  - Why is there the nuvolaruntime-devel BR? It does not appear to be required.

* Wrong "extends" tag in .metainfo.xml
  - The "extends" tag should reference the "nuvolaruntime" desktop file,
    right [3]?

* Are the nuvolaplayer3_amazon_cloud_player.* icons required?
  - I am not sure the icons located in %{_datadir}/icons/*/*/*
    /nuvolaplayer3_amazon_cloud_player.* are required. The .desktop file
    specifies they should be named eu.tiliado.NuvolaAppAmazonCloudPlayer, which
    is the other set available. This should be probably discussed with upstream.
  - BTW rpmlint complains about these files:

~~~
nuvola-app-amazon-cloud-player.noarch: W: dangling-relative-symlink /usr/share/icons/hicolor/128x128/apps/nuvolaplayer3_amazon_cloud_player.png ../../../../nuvolaplayer3/web_apps/amazon_cloud_player/icons/128.png
~~~

* Missing requires
  - The package specifies no requires, but it definitely needs nuvola runtime,
    so it should probably specify the dependency:

~~~
Requires: %{_bindir}/nuvola
~~~


[1] http://pkgs.fedoraproject.org/cgit/rpms/python-setuptools.git/commit/?id=5e5ba5a5931b254843baaf45e6dbd1ebe2769b91
[2] https://apps.fedoraproject.org/koschei/package/diorite?collection=f27
[3] https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Addon.html#tag-extends

(In reply to Vít Ondruch from comment #7)
> * Build issues
>   - It seems that you need to add BR on python3-six to fix the build. I think
>     the issue is caused by this [1] commit.
done

>   - BTW the same commit breaks Diorite [2]. The python3-pyparsing should be
>     required unconditionally everywhere. I still don't understand what was
>     the difference between f26/f27, that it was not needed, but it probably
>     does not matter anyway ...
> 
done

> * nuvolaruntime-devel dependency.
>   - Why is there the nuvolaruntime-devel BR? It does not appear to be
> required.
> 
removed


> * Wrong "extends" tag in .metainfo.xml
>   - The "extends" tag should reference the "nuvolaruntime" desktop file,
>     right [3]?
> 
Tries to change


> * Are the nuvolaplayer3_amazon_cloud_player.* icons required?
>   - I am not sure the icons located in %{_datadir}/icons/*/*/*
>     /nuvolaplayer3_amazon_cloud_player.* are required. The .desktop file
>     specifies they should be named eu.tiliado.NuvolaAppAmazonCloudPlayer,
> which
>     is the other set available. This should be probably discussed with
> upstream.

Upstream ticket:
https://github.com/tiliado/nuvola-app-amazon-cloud-player/issues/26

>   - BTW rpmlint complains about these files:
> 
> ~~~
> nuvola-app-amazon-cloud-player.noarch: W: dangling-relative-symlink
> /usr/share/icons/hicolor/128x128/apps/nuvolaplayer3_amazon_cloud_player.png
> ../../../../nuvolaplayer3/web_apps/amazon_cloud_player/icons/128.png
> ~~~
> 

Upstream ticket:
https://github.com/tiliado/nuvola-app-amazon-cloud-player/issues/25


> * Missing requires
>   - The package specifies no requires, but it definitely needs nuvola
> runtime,
>     so it should probably specify the dependency:
> 
> ~~~
> Requires: %{_bindir}/nuvola
> ~~~

added

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvola-app-amazon-cloud-player.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvola-app-amazon-cloud-player-5.4-4.fc25.src.rpm

Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvola-app-amazon-cloud-player.spec
SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvola-app-amazon-cloud-player-5.4-5.fc25.src.rpm

%changelog
* Wed Jun 21 2017 Martin Gansser <martinkg> - 5.4-5
- rename nuvolaplayer3 to nuvolaruntime in Makefile

* Tue Jun 20 2017 Martin Gansser <martinkg> - 5.4-4
- add BR  python3-six
- add RR  %%{_bindir}/nuvola
- delete BR nuvolaruntime-devel


rpmlint -i -v nuvola-app-amazon-cloud-player.spec /home/martin/rpmbuild/SRPMS/nuvola-app-amazon-cloud-player-5.4-5.fc25.src.rpm /home/martin/rpmbuild/RPMS/noarch/nuvola-app-amazon-cloud-player-5.4-5.fc25.noarch.rpm
nuvola-app-amazon-cloud-player.spec: I: checking
nuvola-app-amazon-cloud-player.spec:30: W: configure-without-libdir-spec
A configure script is run without specifying the libdir. configure options
must be augmented with something like --libdir=%{_libdir} whenever the script
supports it.

nuvola-app-amazon-cloud-player.spec: I: checking-url https://github.com/tiliado/nuvola-app-amazon-cloud-player/archive/5.4.tar.gz#/nuvola-app-amazon-cloud-player-5.4.tar.gz (timeout 10 seconds)
nuvola-app-amazon-cloud-player.src: I: checking
nuvola-app-amazon-cloud-player.src: W: spelling-error %description -l en_US linux -> Linux
The value of this tag appears to be misspelled. Please double-check.

nuvola-app-amazon-cloud-player.src: I: checking-url https://github.com/tiliado/nuvola-app-amazon-cloud-player (timeout 10 seconds)
nuvola-app-amazon-cloud-player.src:30: W: configure-without-libdir-spec
A configure script is run without specifying the libdir. configure options
must be augmented with something like --libdir=%{_libdir} whenever the script
supports it.

nuvola-app-amazon-cloud-player.src: I: checking-url https://github.com/tiliado/nuvola-app-amazon-cloud-player/archive/5.4.tar.gz#/nuvola-app-amazon-cloud-player-5.4.tar.gz (timeout 10 seconds)
nuvola-app-amazon-cloud-player.noarch: I: checking
nuvola-app-amazon-cloud-player.noarch: W: spelling-error %description -l en_US linux -> Linux
The value of this tag appears to be misspelled. Please double-check.

nuvola-app-amazon-cloud-player.noarch: I: checking-url https://github.com/tiliado/nuvola-app-amazon-cloud-player (timeout 10 seconds)
nuvola-app-amazon-cloud-player.noarch: W: desktopfile-without-binary /usr/share/applications/eu.tiliado.NuvolaAppAmazonCloudPlayer.desktop nuvola
the .desktop file is for a file not present in the package. You should check
the requires or see if this is not a error

2 packages and 1 specfiles checked; 0 errors, 5 warnings.

The build on Rawhide is failing for me:

~~~
RPM build errors:
    File not found: /builddir/build/BUILDROOT/nuvola-app-amazon-cloud-player-5.4-5.fc27.x86_64/usr/share/icons/*/*/*/nuvolaplayer3_amazon_cloud_player.*
~~~

Is this since the issues:

https://github.com/tiliado/nuvola-app-amazon-cloud-player/issues/25
https://github.com/tiliado/nuvola-app-amazon-cloud-player/issues/26

were resolved? Removing the "%{_datadir}/icons/*/*/*/nuvolaplayer3_amazon_cloud_player.*" line helps. Also, the sed applied to Makefile si not necessary anymore, I guess.

Otherwise the package looks good => I APPROVE the package. Please fix the build issues prior you import the package.

@Vit
Thanks for the review, i will fix the mentioned issues.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/nuvola-app-amazon-cloud-player

package has been built successfully on f25, f26 and rawhide.