[Bug 1441828] Review Request: nuvolaplayer- Cloud Music Integration for your Linux Desktop

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1441828



--- Comment #30 from Vít Ondruch <vondruch@xxxxxxxxxx> ---
There are still few things remaining from my previous review:

(In reply to Vít Ondruch from comment #21)
> * AppData should be validated
>   - Please add validation of AppData [1].

Actually have you followed the link I provided? Haven't you add validation of
the .desktop file instead?

> * Unneeded build dependencies
>   - It appears that the following BRs are not needed (at least on Rawhide):
> 
> ~~~
> BuildRequires:  xorg-x11-utils
> BuildRequires:  xorg-x11-server-Xvfb
> ~~~

Still not sure why you keep these dependencies. Testing the build on Rawhide,
the build succeeds without them.

> * Test suite.
>   - Is there test suite? Is it possible to execute it?

Just nitpicking but it would be probably good to move the note about the test
suite into the %check section.

> > * Bundled engine.io library?
> >   - It seems engine.io library is bundled by nuvolaplayer. Is it required for
> >     something? If yes, shouldn't it be extracted or at least there should be
> >     the "bundled()" provider [4].
> > 
> 
> Engine.io is not a bundled library in the meaning as a copy of an
> independent library. It just hasn't been separated into and independent
> library yet. Engineio is used in some experimental features which are not
> built by default yet but that may change in the future.

Frankly, I am not very satisfied with upstream response, since this [2] commit
clearly links engine.io.js file from different upstream repository. Moreover,
the .js file is in two copies currently in the source code ...

> > * What is the purpose of %{_datadir}/nuvolaplayer3/web_apps/test
> >   - Is the content of the directory used somewhere?
> > 
> 
> This is a test service to test various features of Nuvola (nuvola -D -a
> test). It may be helpful when you need provide your users with support. It
> might go to devel subpackage though.

Actually, now I understand what it is :) It should be packaged as other
nuvola-app- plugins. IOW this should go into nuvola-app-test subpackage.

But for the moment, the -devel package should be good as well. You can move it
into subpackage any time later. IOW some TODO: in .spec file should be enough
ATM.

> > * What is the purpose of %{_datadir}/nuvolaplayer3/audio
> >   - Is the .mp3 fie used somewhere for something?
> > 
> 
> The mp3 file is used to check that gstreamer can play mp3 files.

Ok, it seems this file is referenced in the code, so it should be preserved in
the package.

Actually, testing this now (using the test app included in -devel subpackage
for now), Nuvola works without this file, but apparently, when the file is
available, I am prompted to install mp3 codec.

And the behavior is quite funny. If I remove the file, Nuvola just starts. If I
keep that file, asks me to install mp3 coded. When I install the codec, it
keeps asking me for Flash :) Well, this could be improved ...

Actually, may be we should add some "gstreamer1-plugin-mpg123", not sure. Lets
polish this later ...


> * License
>   - Could you please check the licensing? Nuvolaplayer is BSD, but the
>     engine.io is MIT.

The license should be just "BSD or MIT" IMO. I can't see any GPL code in
Nuvola.

And here are some new issues I just noticed:

* Use "%setup -q"
  - I just noticed that you are using "%setup -qn %{name}-%{version}" where
    "%setup -q" should be enough.

* /usr/lib64/nuvolaplayer3/apprunner should go to %{_libexecdir}
  - I think that the apprunner should got to %{_libexecdir} on Fedora [3].
    Unfortunately, it does not appear to be properly supported by upstream [4].
    I don't think this is showstopper, since %{_libdir}/%{name} is the second
    choice according to the guidelines, but probably something to consider.

* rpmlint output:

~~~
nuvolaplayer.x86_64: W: no-soname /usr/lib64/libnuvolaplayer3-base.so
nuvolaplayer.x86_64: W: no-soname /usr/lib64/libnuvolaplayer3-runner.so
~~~

  - I am not really sure about it ^^. Of course there are not API/ABI promises
    and nothing except Nuvola is supposed to use these libraries, so this is
    probably false positive.

~~~
nuvolaplayer.x86_64: W: shared-lib-calls-exit
/usr/lib64/libnuvolaplayer3-runner.so exit@GLIBC_2.2.5
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.
~~~

  - But this one ^^ seems to be interesting. Not sure, probably good to ask
    upstream about it.

~~~
nuvolaplayer.x86_64: W: dangling-relative-symlink
/usr/lib/.build-id/3e/a3077f6c6b3ae4480d38d88f7dc630076c46f0
../../../../usr/lib64/libengineio.so
~~~

  - Unfortunately, you cannot do just "%exclude %{_libdir}/libengineio.so".
    You have to "rm" the file in the install section to avoid this warning.
    Actually, there are some "engineio" related files kept in -devel
subpackage.
    They should be probably removed as well.


Overall, I am quite positive about the latest iteration. I hope the next
iteration will be the last one :) Thx.


[1] https://fedoraproject.org/wiki/Packaging:AppData#app-data-validate_usage
[2]
https://github.com/tiliado/nuvolaplayer/commit/56171f4d021d65adebf1ae234ba3218a2ba9ad11
[3] https://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir
[4]
https://github.com/tiliado/nuvolaplayer/blob/master/src/nuvolakit-base/main.vala#L132

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux