https://bugzilla.redhat.com/show_bug.cgi?id=1441828 --- Comment #32 from mgansser@xxxxxxxx <mgansser@xxxxxxxxx> --- (In reply to Vít Ondruch from comment #30) > 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? my mistake, changed. > > > * 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. > forgot it, changed. > > * 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. > done > > > * 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 ... > will contact upstream. > > > * 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. > done > > > * 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 ... > > add recommend gstreamer1-plugin-mpg123 > > * 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. > I changed the license field to BSD, or do you mean "BSD or MIT" i never saw this combination. > 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. > done > * /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. > leave it for now. > * 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. > ok > ~~~ > 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. > will ask upstream. > ~~~ > 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. > > removed this file as well. > Overall, I am quite positive about the latest iteration. I hope the next > iteration will be the last one :) Thx. > hopefully I have not forgotten anything. :-( > [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 new rpm files: Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.3-4.fc25.src.rpm -- 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
