Bug 1441831
Summary: | Review Request: nuvolasdk - SDK for building Nuvola Player's web app scripts | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | MartinKG <mgansser> |
Component: | Package Review | Assignee: | Vít Ondruch <vondruch> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | package-review, vondruch |
Target Milestone: | --- | Flags: | vondruch:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | https://github.com/tiliado/nuvolasdk/ | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2017-04-26 11:41:52 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
MartinKG
2017-04-12 20:19:37 UTC
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 sorry wrong ticket, forget last commit. Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolasdk.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolasdk-1.3.0-1.fc25.src.rpm %changelog * Sun Apr 16 2017 Martin Gansser <martinkg> - 1.3.0-1 - Update to stable 1.3.0-1 I'll take this for a review. First of all, I am not Python packager, so not sure I catch everything ... * Dependencies - Why there all the dependencies listed? I tried to remove all of them except of "python3-devel" and the build passed just fine. - Actually, trying to prepare some module via nuvolasdk, it seems that you should add: Requires: %{bindir}/scour Requires: %{bindir}/gm You have the runtime dependency on "lasem" there, but that is actually just library, you would need "lasem-render" executable or in case of Fedora "lasem-render-0.4". But since it has huge dependency chain, I'd suggest to go with GraphicsMagic instead, which has the smallest footprint of all the supported graphics engines. - Also, I am thinking if that might be just soft dependencies? Because in case you'd like to use "lasem" instead of "gm", why to force somebody. OTOH, if some Nuvola app is going to have build dependency on nuvolasdk, there should be probably hard dependency. I probably thinking too much ;) * Python macros - Shouldn't you use %py3_build and %py3_install macros instead of the "python setup.py" calls? * License - It should be only BSD, shouldn't it? * nuvolasdk/data/template/LICENSE-BSD.txt - This file should not be marked as %licesne IMO. This file is just part of a template for new project, not a real license file. (In reply to Vít Ondruch from comment #5) > First of all, I am not Python packager, so not sure I catch everything ... > > * Dependencies > - Why there all the dependencies listed? I tried to remove all of them > except > of "python3-devel" and the build passed just fine. > - Actually, trying to prepare some module via nuvolasdk, it seems that you > should add: > > Requires: %{bindir}/scour > Requires: %{bindir}/gm done > > You have the runtime dependency on "lasem" there, but that is actually > just > library, you would need "lasem-render" executable or in case of Fedora > "lasem-render-0.4". But since it has huge dependency chain, I'd suggest > to > go with GraphicsMagic instead, which has the smallest footprint of all > the > supported graphics engines. > - Also, I am thinking if that might be just soft dependencies? Because > in case you'd like to use "lasem" instead of "gm", why to force somebody. > OTOH, if some Nuvola app is going to have build dependency on nuvolasdk, > there should be probably hard dependency. I probably thinking too much ;) > i used now GraphicsMagic > * Python macros > - Shouldn't you use %py3_build and %py3_install macros instead of the > "python > setup.py" calls? used mentioned macros > > * License > - It should be only BSD, shouldn't it? > ok, changed to BSD > * nuvolasdk/data/template/LICENSE-BSD.txt > - This file should not be marked as %licesne IMO. This file is just part > of a template for new project, not a real license file. unmarked nuvolasdk/data/template/LICENSE-BSD.txt as license file. NEW rpm files: Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolasdk.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolasdk-1.3.0-2.fc25.src.rpm %changelog * Thu Apr 13 2017 Martin Gansser <martinkg> - 1.3.0-2 - Changed license field to BSD only - Add RR %%{_bindir}/scour - Add RR %%{_bindir}/gm - Use %%py3_build and %%py3_install macros instead of the "python setup.py" calls - Unmark %%license LICENSE nuvolasdk/data/template/LICENSE-BSD.txt - Remove BR ImageMagick-devel, librsvg2-devel, librsvg2-tools - Remove BR python3-scour, vala-devel, lasem I can't see any other issues. I asked also some python guys and they can't see nothing wrong with the package => APPROVED Thanks for the review. Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/nuvolasdk package has been built successfully on f25, f26 and rawhide. |