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
