Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.2-0.1git95103ca.fc25.src.rpm Description: Nuvola Player 3 is a runtime for web-based music streaming services providing more native user experience and integration with Linux desktop environments than usual web browsers can offer. Nuvola Players handles multimedia keys, shows desktop notifications, integrates with various sound menus, applets and launchers and more. Additional features include Last FM# scrobling. Fedora Account System Username: martinkg %changelog * Wed Apr 12 2017 Martin Gansser <martinkg> - 3.1.2-0.1git95103ca - Update to 3.1.2-0.1git95103ca rpmlint -i nuvolaplayer.spec /home/martin/rpmbuild/SRPMS/nuvolaplayer-3.1.2-0.1git95103ca.fc25.src.rpm /home/martin/rpmbuild/RPMS/x86_64/nuvolaplayer-3.1.2-0.1git95103ca.fc25.x86_64.rpm /home/martin/rpmbuild/RPMS/x86_64/nuvolaplayer-devel-3.1.2-0.1git95103ca.fc25.x86_64.rpm /home/martin/rpmbuild/RPMS/x86_64/nuvolaplayer-debuginfo-3.1.2-0.1git95103ca.fc25.x86_64.rpm (none): E: error while reading /home/martin/rpmbuild/SRPMS/nuvolaplayer-3.1.2-0.1git95103ca.fc25.src.rpm: 'str' object has no attribute 'decode' 0 packages and 1 specfiles checked; 0 errors, 0 warnings.
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolasdk.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.2-1.fc25.src.rpm %changelog * Sun Apr 16 2017 Martin Gansser <martinkg> - 3.1.2-1 - Update to 3.1.2-1
correct links: Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.2-1.fc25.src.rpm %changelog * Sun Apr 16 2017 Martin Gansser <martinkg> - 3.1.2-1 - Update to 3.1.2-1
I'll take this for a review. However, prior I start, could you check that the system waf is used instead of the local copy, the license is correct and the build and runtime dependencies? Also, it seems that the nuvola apps, although packaged into RPM and providing their own .desktop files, nuvolaplayer keeps to create other copy of the .desktop file in ~/.local/share/applications/, is there any way how to control this behavior?
compiling with this spec file fails: https://martinkg.fedorapeople.org/Packages/nuvolaplayer/nuvolaplayer.spec [martin@fc25 SPECS]$ rpmbuild -ba nuvolaplayer.spec Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.nP0nhP + umask 022 + cd /home/martin/rpmbuild/BUILD + cd /home/martin/rpmbuild/BUILD + rm -rf nuvolaplayer-3.1.2 + /usr/bin/tar -xof - + /usr/bin/gzip -dc /home/martin/rpmbuild/SOURCES/nuvolaplayer-3.1.2.tar.gz + STATUS=0 + '[' 0 -ne 0 ']' + cd nuvolaplayer-3.1.2 + /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w . + exit 0 Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.d1Bxoh + umask 022 + cd /home/martin/rpmbuild/BUILD + cd nuvolaplayer-3.1.2 + VALAFLAGS=--quiet + waf configure --prefix=/usr --libdir=/usr/lib64 --allow-fuzzy-build fatal: Not a git repository (or any of the parent directories): .git Setting top to : /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2 Setting out to : /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/build Version : 3.1.2+snapshot Upstream revision : unknown (unsupported build) Target platform : LINUX Install prefix : /usr Checking for 'gcc' (C compiler) : /usr/bin/gcc Checking for program 'pkg-config' : /usr/bin/pkg-config Checking for 'gobject-2.0' : yes Checking for 'gthread-2.0' : yes Checking for program 'valac-0.8' : not found Checking for program 'valac' : /usr/bin/valac Checking for valac version >= (0, 8, 0) : (0, 34, 7) Checking for program 'valac-0.26' : /usr/bin/valac Checking for valac-0.26 version >= (0, 26, 1) : (0, 34, 7) Compiler optimizations : ON Checking for 'glib-2.0' : yes Checking for 'gio-2.0' : yes Checking for 'gthread-2.0' : yes Checking for 'gtk+-3.0' : yes Checking for 'gdk-3.0' : yes Checking for 'gdk-x11-3.0' : yes Checking for 'x11' : yes Checking for 'dioriteglib-0.3' : yes Checking for 'dioritegtk-0.3' : yes Checking for 'json-glib-1.0' : yes Checking for 'libarchive' : yes Checking for 'libnotify' : yes Checking for 'libsecret-1' : yes Checking for 'gstreamer-1.0' : yes Checking for 'webkit2gtk-4.0' : yes Checking for 'webkit2gtk-web-extension-4.0' : yes Checking for 'javascriptcoregtk-4.0' : yes Checking for 'uuid' : yes Checking for 'libsoup-2.4' : yes Checking for 'webkit2gtk-4.0' version : yes 'configure' finished successfully (0.283s) + waf build --prefix=/usr --libdir=/usr/lib64 fatal: Not a git repository (or any of the parent directories): .git Waf: Entering directory `/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/build' Waf: Leaving directory `/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/build' source not found: 'data/js//home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/data/js/audio.js' in bld(target=['share/nuvolaplayer3/js//home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/data/js/audio.js'], idx=26, install_task=None, meths=['process_rule', 'process_source'], install_path='${PREFIX}/share/nuvolaplayer3/js', rule='cp -v ${SRC} ${TGT}', _name='share/nuvolaplayer3/js//home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/data/js/audio.js', source='data/js//home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/data/js/audio.js', path=/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2, posted=True, features=[]) in /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2 error: Bad exit status from /var/tmp/rpm-tmp.d1Bxoh (%build)
Created attachment 1273606 [details] wscript patch After some fighting, I was able to come up with the attached patch which enables build of nuvolaplayer. But I am afraid this should be discussed with upstream ...
(In reply to Vít Ondruch from comment #5) > Created attachment 1273606 [details] > wscript patch > > After some fighting, I was able to come up with the attached patch which > enables build of nuvolaplayer. But I am afraid this should be discussed with > upstream ... I added the your patch to the spec file: https://martinkg.fedorapeople.org/Packages/nuvolaplayer/nuvolaplayer.spec now i get this message: .... [23/27] Compiling build/Transport.c [24/27] Compiling build/jsapi.c [25/27] Compiling build/jsexecutor.c [26/27] Linking build/libengineio.so [27/27] Linking build/libnuvolaplayer3-base.so Waf: Leaving directory `/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/build' File /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/src/nuvolakit-runner/secret.vapi has no mapping in ['.c', '.gs', '.vala', '.obj', '.o', '.pc.in'] (load a waf tool?) error: Bad exit status from /var/tmp/rpm-tmp.iXKaFq (%build)
(In reply to Vít Ondruch from comment #5) > Created attachment 1273606 [details] > wscript patch > > After some fighting, I was able to come up with the attached patch which > enables build of nuvolaplayer. But I am afraid this should be discussed with > upstream ... upstream ticket: https://github.com/tiliado/nuvolaplayer/issues/327
(In reply to mgansser from comment #6) > > .... > > [23/27] Compiling build/Transport.c > [24/27] Compiling build/jsapi.c > [25/27] Compiling build/jsexecutor.c > [26/27] Linking build/libengineio.so > [27/27] Linking build/libnuvolaplayer3-base.so > Waf: Leaving directory `/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/build' > File > /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/src/nuvolakit-runner/secret. > vapi has no mapping in ['.c', '.gs', '.vala', '.obj', '.o', '.pc.in'] (load > a waf tool?) > error: Bad exit status from /var/tmp/rpm-tmp.iXKaFq (%build) Is this "waf build" again? Is this Rawhide?
(In reply to Vít Ondruch from comment #8) > (In reply to mgansser from comment #6) > > > > .... > > > > [23/27] Compiling build/Transport.c > > [24/27] Compiling build/jsapi.c > > [25/27] Compiling build/jsexecutor.c > > [26/27] Linking build/libengineio.so > > [27/27] Linking build/libnuvolaplayer3-base.so > > Waf: Leaving directory `/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/build' > > File > > /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.2/src/nuvolakit-runner/secret. > > vapi has no mapping in ['.c', '.gs', '.vala', '.obj', '.o', '.pc.in'] (load > > a waf tool?) > > error: Bad exit status from /var/tmp/rpm-tmp.iXKaFq (%build) > > Is this "waf build" again? Is this Rawhide? yes it's "waf build" on F25 not Rawhide.
nuvolaplayer compiles fine on F25 without this part in your patch: @@ -546,16 +550,6 @@ def dist(ctx): ctx.archive = archive from waflib.TaskGen import extension -@extension('.vapi') -def vapi_file(self, node): - try: - valatask = self.valatask - except AttributeError: - valatask = self.valatask = self.create_task('valac') - self.init_vala_task() - - valatask.inputs.append(node) - from waflib import TaskGen, Utils, Errors, Node, Task from nuvolamergejs import mergejs as merge_js
(In reply to mgansser from comment #10) > nuvolaplayer compiles fine on F25 without this part in your patch: Yep, that is why I was asking about Rawhide, because there is already the latest waf, which integrates this snippet already. I always do all test just against Rawhide, since the package always lands there first ...
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.2-2.fc25.src.rpm %changelog * Fri Apr 21 2017 Martin Gansser <martinkg> - 3.1.2-2 - Use virtual provides for BR - Add BR libuuid-devel - Add BR %%{_bindir}/waf - Add BR %%{_bindir}/valac - Use system waf installation - Add %%{name}-wscript.patch - Add %%{name}-wscript-rawhide.patch
(In reply to Vít Ondruch from comment #3) > > Also, it seems that the nuvola apps, although packaged into RPM and > providing their own .desktop files, nuvolaplayer keeps to create other copy > of the .desktop file in ~/.local/share/applications/, is there any way how > to control this behavior? upstream ticket: https://github.com/tiliado/nuvolaplayer/issues/328
I tried it again with diorite-0.3.4 and waf-1.9.10 on Fedora 25, but this fails again: installed diorite version: $ rpm -qa |grep diorite diorite-0.3.4-1.fc25.x86_64 diorite-devel-0.3.4-1.fc25.x86_64 I have already install waf-1.9.10 from koji, in the next days waf-1.9.10 will be in the fedora stable repo. $ waf --version waf 1.9.10 (c83e485d1682d6af8a632234d9cbbc18be7b490b) I compile nuvolaplayer with the following rpm spec file: https://martinkg.fedorapeople.org/Packages/nuvolaplayer/nuvolaplayer.spec this is the error message: $ rpmbuild -ba nuvolaplayer.spec Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.Ke6XDi + umask 022 + cd /home/martin/rpmbuild/BUILD + cd /home/martin/rpmbuild/BUILD + rm -rf nuvolaplayer-3.1.3 + /usr/bin/gzip -dc /home/martin/rpmbuild/SOURCES/nuvolaplayer-3.1.3.tar.gz + /usr/bin/tar -xof - + STATUS=0 + '[' 0 -ne 0 ']' + cd nuvolaplayer-3.1.3 + /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w . + rm ./waf + rm -rf ./.waf + exit 0 Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.wGJ1dE + umask 022 + cd /home/martin/rpmbuild/BUILD + cd nuvolaplayer-3.1.3 + waf configure --prefix=/usr --libdir=/usr/lib64 Setting top to : /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.3 Setting out to : /home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.3/build fatal: Not a git repository (or any of the parent directories): .git Version : 3.1.3 Upstream revision : unknown Install prefix : /usr Branding : default Branding metadata : branding/default.json Traceback (most recent call last): File "/usr/share/waf/waflib/Scripting.py", line 121, in waf_entry_point run_commands() File "/usr/share/waf/waflib/Scripting.py", line 182, in run_commands ctx=run_command(cmd_name) File "/usr/share/waf/waflib/Scripting.py", line 173, in run_command ctx.execute() File "/usr/share/waf/waflib/Configure.py", line 84, in execute super(ConfigurationContext,self).execute() File "/usr/share/waf/waflib/Context.py", line 87, in execute self.recurse([os.path.dirname(g_module.root_path)]) File "/usr/share/waf/waflib/Context.py", line 128, in recurse user_function(self) File "/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.3/wscript", line 192, in configure branding = loadjson(branding_json, False) File "/home/martin/rpmbuild/BUILD/nuvolaplayer-3.1.3/wscript", line 105, in loadjson except FileNotFoundError: NameError: global name 'FileNotFoundError' is not defined
(In reply to mgansser from comment #14) This might help you to get further: "rm branding/default.json" There does not appear to be anything in that file ATM.
Actually, checking the waf, there is "waf-python3" package which executes waf using Python 3. So for the time being "Requires: %{_bindir}/waf-3" is the best solution IMO. I also opened ticket with waf maintainers to discuss alternative solutions: https://bugzilla.redhat.com/show_bug.cgi?id=1447231
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.3-1.fc25.src.rpm %changelog * Mon May 01 2017 Martin Gansser <martinkg> - 3.1.3-1 - Update to 3.1.3-1 - Add BR pkgconfig(unity) - Add RR %%{_bindir}/waf-3
(In reply to mgansser from comment #17) > - Add RR %%{_bindir}/waf-3 Actually, this should be BR of course. Sorry for me mistyping it in my previous comment :/ Why the "pkgconfig(unity)"? And on Rawhide, the configuration fails if I am not mistaken: ~~~ Checking for 'sqlite3' : not found The configuration failed ~~~
(In reply to Vít Ondruch from comment #18) > (In reply to mgansser from comment #17) > > - Add RR %%{_bindir}/waf-3 > > Actually, this should be BR of course. Sorry for me mistyping it in my > previous comment :/ changed to BR > > Why the "pkgconfig(unity)"? ok, disabled unity > > And on Rawhide, the configuration fails if I am not mistaken: > > ~~~ > Checking for 'sqlite3' : not found > The configuration failed > ~~~ added BR sqlite3 for rawhide Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.3-2.fc25.src.rpm %changelog * Tue May 02 2017 Martin Gansser <martinkg> - 3.1.3-2 - add BR pkgconfig(sqlite3) for fedora > 26 - dropped BR pkgconfig(unity) - add --nounity configure flag
scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=19385948
* AppData should be validated - Please add validation of AppData [1]. * Unneeded build dependencies - It appears that the following BRs are not needed (at least on Rawhide): ~~~ BuildRequires: gnome-python2-rsvg BuildRequires: intltool BuildRequires: librsvg2-tools BuildRequires: notification-daemon BuildRequires: pkgconfig(libarchive) BuildRequires: pkgconfig(dbusmenu-gtk-0.4) BuildRequires: pkgconfig(gee-1.0) BuildRequires: pkgconfig(librsvg-2.0) BuildRequires: pkgconfig(libnm-glib) BuildRequires: pkgconfig(unique-1.0) BuildRequires: pkgconfig(dbus-glib-1) BuildRequires: xorg-x11-utils BuildRequires: xorg-x11-server-Xvfb ~~~ * The sqlite dependency should not be conditional - The sqlite dependency is needed everywhere [2]. There is not any reason for that conditional around. * The gcc build dependency is missing - There should be BR: gcc [3]. * Test suite. - Is there test suite? Is it possible to execute it? * Redundant runtime requies. - What is the reason for "R: control-center-filesystem" - What is the reason for "R: tsocks" * Hardcode diorite dependency - Since diorite does not provide any API/ABI stability ensurance, I think that nuvolaplayer should hardcode the diorite dependency, e.g.: Requires: diorite >= 0.3.4 - This should prevent issues such as: ~~~ $ rpm -q diorite diorite-0.3.3-3.fc27.x86_64 $ nuvola Master: [WARNING Δ009161us GLib-GObject] value "((GApplicationFlags) G_APPLICATION_SEND_ENVIRONMENT | G_APPLICATION_NON_UNIQUE | G_APPLICATION_CAN_OVERRIDE_APP_ID | 17827712)" of type 'GApplicationFlags' is invalid or out of range for property 'flags' of type 'GApplicationFlags' nuvola: symbol lookup error: /lib64/libnuvolaplayer3-runner.so: undefined symbol: diorite_actions_add_actions ~~~ * Vala files => -devel subpackage - I am not expert in Vala, but are the %{_datadir}/vala/vapi/* files needed for runtime? I'd be surprised if they are. Don't they belong to -devel subpackage? * 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]. * What is the purpose of %{_datadir}/nuvolaplayer3/web_apps/test - Is the content of the directory used somewhere? * What is the purpose of %{_datadir}/nuvolaplayer3/audio - Is the .mp3 fie used somewhere for something? * Directory ownership - Please check the directory ownership. It seems that the package quite often owns just files, but not the directories. * License - Could you please check the licensing? Nuvolaplayer is BSD, but the engine.io is MIT. [1] https://fedoraproject.org/wiki/Packaging:AppData#app-data-validate_usage [2] https://github.com/tiliado/nuvolaplayer/blob/master/wscript#L248 [3] https://fedoraproject.org/wiki/Packaging:C_and_C++#BuildRequires_and_Requires [4] https://github.com/tiliado/nuvolaplayer/commit/56171f4d021d65adebf1ae234ba3218a2ba9ad11 [5] https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle
(In reply to Vít Ondruch from comment #21) > > * Test suite. > - Is there test suite? Is it possible to execute it? > i tried it with this rpm spec file, but it fails with: https://martinkg.fedorapeople.org/Packages/nuvolaplayer/nuvolaplayer.spec + pushd tests ~/rpmbuild/BUILD/nuvolaplayer-3.1.3/tests ~/rpmbuild/BUILD/nuvolaplayer-3.1.3 + make all mkdir -p ../build/tests/run diorite-testgen-0.3 -o ../build/tests/run/testcase.vala webappregistrytest.vala BindingsTest.vala usage: diorite-testgen-0.3 [-h] [-i INPUT] [-o OUTPUT] if i run make all in the source folder i get this: [martin@fc25 tests]$ make all valac -C -d ../build/tests -b . --thread -v \ --library=testcase -H ../build/tests/testcase.h \ --target-glib=2.32 --vapidir ../build/tests --vapidir ../build --vapidir ../vapi --pkg=glib-2.0 --pkg=dioriteglib-0.3 --pkg=gtk+-3.0 --pkg=javascriptcoregtk-4.0 --pkg=webkit2gtk-4.0 --pkg nuvolaplayer3-runner --pkg nuvolaplayer3-base webappregistrytest.vala BindingsTest.vala Loaded package `../vapi/glib-2.0.vapi' Loaded package `../vapi/gobject-2.0.vapi' Loaded package `/usr/share/vala/vapi/dioriteglib-0.3.vapi' Loaded package `../vapi/posix.vapi' Loaded package `../vapi/gio-2.0.vapi' Loaded package `../vapi/gio-unix-2.0.vapi' Loaded package `../vapi/gtk+-3.0.vapi' Loaded package `/usr/share/vala-0.34/vapi/atk.vapi' Loaded package `/usr/share/vala-0.34/vapi/cairo.vapi' Loaded package `../vapi/gdk-pixbuf-2.0.vapi' Loaded package `../vapi/gdk-3.0.vapi' Loaded package `/usr/share/vala-0.34/vapi/pango.vapi' Loaded package `/usr/share/vala-0.34/vapi/pangocairo.vapi' Loaded package `/usr/share/vala-0.34/vapi/x11.vapi' Loaded package `../vapi/javascriptcoregtk-4.0.vapi' Loaded package `../vapi/webkit2gtk-4.0.vapi' Loaded package `../vapi/libsoup-2.4.vapi' error: Package `nuvolaplayer3-runner' not found in specified Vala API directories or GObject-Introspection GIR directories error: Package `nuvolaplayer3-base' not found in specified Vala API directories or GObject-Introspection GIR directories Compilation failed: 2 error(s), 0 warning(s) Makefile:59: recipe for target '../build/tests/webappregistrytest.c' failed make: *** [../build/tests/webappregistrytest.c] Error 1
(In reply to mgansser from comment #22) Well, the tests were not modified in past 3 years, so they might not be in good shape ... dunno :/
(In reply to Vít Ondruch from comment #23) > (In reply to mgansser from comment #22) > Well, the tests were not modified in past 3 years, so they might not be in > good shape ... dunno :/ I think we should not use the test.
(In reply to Vít Ondruch from comment #21) I could not answer the questions, so I opened a ticket. https://github.com/tiliado/nuvolaplayer/issues/334 > > * 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. > * 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. > * 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.
(In reply to mgansser from comment #24) > (In reply to Vít Ondruch from comment #23) > > (In reply to mgansser from comment #22) > > Well, the tests were not modified in past 3 years, so they might not be in > > good shape ... dunno :/ > > I think we should not use the test. At minimum, it would be good to document it somehow in the .spec file. And it would be even better to poke upstream about that ... (In reply to mgansser from comment #25) > (In reply to Vít Ondruch from comment #21) > > > > * 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. Then I would suggest to not ship the library ATM if possible ... > > * 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. Good advice. > > * 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. How to do that? Is this available somewhere in UI? Is this available somewhere to end user? I'd suggest to not ship the file unless it proves we are missing it for something ...
(In reply to Vít Ondruch from comment #26) > > > > * 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. > > How to do that? Is this available somewhere in UI? Is this available > somewhere to end user? I'd suggest to not ship the file unless it proves we > are missing it for something ... answer from the developer: > > How to do that? It is done on Nuvola's start-up automatically. > > Is this available somewhere in UI? Is this available somewhere to end user? Yes, in the Format support dialog. > > I'd suggest to not ship the file unless it proves we are missing it for something ... Hm, I should check the code to find out how Nuvola behaves in that case and make sure it won't crash.
(In reply to mgansser from comment #27) > (In reply to Vít Ondruch from comment #26) > > > > > > * 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. > > > > How to do that? Is this available somewhere in UI? Is this available > > somewhere to end user? I'd suggest to not ship the file unless it proves we > > are missing it for something ... > > answer from the developer: > > > > How to do that? > > It is done on Nuvola's start-up automatically. > > > > Is this available somewhere in UI? Is this available somewhere to end user? > > Yes, in the Format support dialog. > > > > I'd suggest to not ship the file unless it proves we are missing it for something ... > > Hm, I should check the code to find out how Nuvola behaves in that case and > make sure it won't crash. answer from the developer: It doesn't crash but format support check doesn't work then.
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.3-3.fc25.src.rpm %changelog * Thu May 04 2017 Martin Gansser <martinkg> - 3.1.3-3 - add validation of AppData - remove unneeded build dependencies - sqlite dependency is not conditional - add BR gcc - moved %%{_datadir}/vala/vapi/* to -devel subpackage - fix directory ownership - %%exclude %%{_libdir}/libengineio.so - add MIT license
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.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
(In reply to Vít Ondruch from comment #30) > Actually, may be we should add some "gstreamer1-plugin-mpg123", not sure. ... we should add some "Recommends: gstreamer1-plugin-mpg123" ...
(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.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
(In reply to Vít Ondruch from comment #30) > > > * 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 ... > answer from upstream: I see. I thought you talked about libengineio-soup - the Vala port of engineio. It didn't occur to me there is also engine.io.js. That could be unbundled. However, I have no idea what is the typical filesystem location for JavaScript libraries. Let's continue at #341. Ticket: https://github.com/tiliado/nuvolaplayer/issues/341 > ~~~ > nuvolaplayer.x86_64: W: shared-lib-calls-exit > /usr/lib64/libnuvolaplayer3-runner.so exit.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. > answer from upstream: libnuvolaplayer3-runner.so contains the main application logic, so it ok to call exit(). Anyway, it does so only as a last resort when WebKitWebProcess hangs. Ticket: https://github.com/tiliado/nuvolaplayer/issues/340
(In reply to mgansser from comment #32) > (In reply to Vít Ondruch from comment #30) > > (In reply to Vít Ondruch from comment #21) > > > > * 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 Do I understand it correctly that you leave it in -devel package 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 ... > > > > > > add recommend gstreamer1-plugin-mpg123 That is good ... It just worries me a bit, that next in line should be recommended flash plugin. But flash is not available in Fedora, so it is not good idea to recommend it ... just thinking loud :) > > > * 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. Actually I meant "BSD and MIT" ("or" would apply for double licensing, which is not the case). The Nuvola itself is BSD licensed, but the bundled engineio-soup content is MIT. It is typically good idea to attach some clarifying comment above the license tag. (In reply to mgansser from comment #33) > (In reply to Vít Ondruch from comment #30) > > > > > * 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 ... > > > > answer from upstream: > I see. I thought you talked about libengineio-soup - the Vala port of > engineio. It didn't occur to me there is also engine.io.js. That could be > unbundled. However, I have no idea what is the typical filesystem location > for JavaScript libraries. Let's continue at #341. > Ticket: https://github.com/tiliado/nuvolaplayer/issues/341 These [1] are JavaScript guidelines which applies for Fedora if you decide to unbundle. If you decide to keep it bundle, then you should follow these guidelines [2]. [1]: https://fedoraproject.org/wiki/Packaging:JavaScript [2]: https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle
(In reply to Vít Ondruch from comment #34) > (In reply to mgansser from comment #32) > > (In reply to Vít Ondruch from comment #30) > > > (In reply to Vít Ondruch from comment #21) > > > > > * 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 > > Do I understand it correctly that you leave it in -devel package ATM? > yes i have leave it in the -devel package. > > > > > * 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 > > That is good ... It just worries me a bit, that next in line should be > recommended flash plugin. But flash is not available in Fedora, so it is not > good idea to recommend it ... just thinking loud :) > no further commends added. > > > > * 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. > > Actually I meant "BSD and MIT" ("or" would apply for double licensing, which > is not the case). The Nuvola itself is BSD licensed, but the bundled > engineio-soup content is MIT. It is typically good idea to attach some > clarifying comment above the license tag. > > changed it to "BSD and MIT" and add comment above the license tag. > (In reply to mgansser from comment #33) > > (In reply to Vít Ondruch from comment #30) > > > > > > > * 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 ... > > > > > > > answer from upstream: > > I see. I thought you talked about libengineio-soup - the Vala port of > > engineio. It didn't occur to me there is also engine.io.js. That could be > > unbundled. However, I have no idea what is the typical filesystem location > > for JavaScript libraries. Let's continue at #341. > > Ticket: https://github.com/tiliado/nuvolaplayer/issues/341 > > These [1] are JavaScript guidelines which applies for Fedora if you decide > to unbundle. If you decide to keep it bundle, then you should follow these > guidelines [2]. > > > > [1]: https://fedoraproject.org/wiki/Packaging:JavaScript > [2]: > https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle i keep the library bundled for now Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.3-5.fc25.src.rpm
(In reply to mgansser from comment #35) > > (In reply to mgansser from comment #33) > > > (In reply to Vít Ondruch from comment #30) > > > > > > > > > * 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 ... > > > > > > > > > > answer from upstream: > > > I see. I thought you talked about libengineio-soup - the Vala port of > > > engineio. It didn't occur to me there is also engine.io.js. That could be > > > unbundled. However, I have no idea what is the typical filesystem location > > > for JavaScript libraries. Let's continue at #341. > > > Ticket: https://github.com/tiliado/nuvolaplayer/issues/341 > > > > These [1] are JavaScript guidelines which applies for Fedora if you decide > > to unbundle. If you decide to keep it bundle, then you should follow these > > guidelines [2]. > > > > > > > > [1]: https://fedoraproject.org/wiki/Packaging:JavaScript > > [2]: > > https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle > > i keep the library bundled for now The version should be probably there. If I am not mistaken, rpmlint complains about that. Have you considered to modify the branding [1]? Actually we already dealt with the branding/default.json earlier, but after the last upstream comment [2], I have the feeling that we might want to link our users to RH bugzilla at minimum. But not sure what is possible and reasonable ... [1]: https://github.com/tiliado/nuvolaplayer#branding [2]: https://github.com/tiliado/nuvolaplayer/issues/334#issuecomment-301033372
(In reply to Vít Ondruch from comment #36) > (In reply to mgansser from comment #35) > > > (In reply to mgansser from comment #33) > > > > (In reply to Vít Ondruch from comment #30) > > > > > > > > > > > * 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 ... > > > > > > > > > > > > > answer from upstream: > > > > I see. I thought you talked about libengineio-soup - the Vala port of > > > > engineio. It didn't occur to me there is also engine.io.js. That could be > > > > unbundled. However, I have no idea what is the typical filesystem location > > > > for JavaScript libraries. Let's continue at #341. > > > > Ticket: https://github.com/tiliado/nuvolaplayer/issues/341 > > > > > > These [1] are JavaScript guidelines which applies for Fedora if you decide > > > to unbundle. If you decide to keep it bundle, then you should follow these > > > guidelines [2]. > > > > > > > > > > > > [1]: https://fedoraproject.org/wiki/Packaging:JavaScript > > > [2]: > > > https://fedoraproject.org/wiki/Bundled_Libraries#Requirement_if_you_bundle > > > > i keep the library bundled for now > > The version should be probably there. If I am not mistaken, rpmlint > complains about that. > I don't know how to determine the version with rpmlint ? > Have you considered to modify the branding [1]? Actually we already dealt > with the branding/default.json earlier, but after the last upstream comment > [2], I have the feeling that we might want to link our users to RH bugzilla > at minimum. But not sure what is possible and reasonable ... > > [1]: https://github.com/tiliado/nuvolaplayer#branding > [2]: https://github.com/tiliado/nuvolaplayer/issues/334#issuecomment-301033372 I've never heard about this branding, but if it's necessary, we might should consider how to link our users to RH bugzilla, as you already said.
(In reply to mgansser from comment #37) > (In reply to Vít Ondruch from comment #36) > > The version should be probably there. If I am not mistaken, rpmlint > > complains about that. > > > > I don't know how to determine the version with rpmlint ? You should probably try to compare with engine.io.js upstream releases ...
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaplayer.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaplayer-3.1.3-6.fc25.src.rpm %changelog * Mon May 15 2017 Martin Gansser <martinkg> - 3.1.3-6 - add version to bundled engine.io
There is still some polish which could be applied, but nothing major to block this any longer => APPROVED
Hi Vit, many thanks for your review ! polish changes are welcome.
package has been built successfully on f25, f26 and rawhide.