Bug 1458394
| Summary: | Re-Review Request: nuvolaruntime - Tight integration of web apps with your desktop, renaming nuvolaplayer | ||
|---|---|---|---|
| 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/nuvolaruntime | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2017-06-07 09:18:03 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-06-02 18:12:52 UTC
You want to use correct Obsoletes/Provides: https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages Also, you want to adjust the description according to these guidelines: https://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or_Description Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaruntime.specSRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaruntime-4.4.0-2.fc25.src.rpm %changelog * Mon Jun 05 2017 Martin Gansser <martinkg> - 4.4.0-2 - remove Trademarks in Description - add Obsoletes: nuvolaplayer <= 3.1.3-7 * Please describe patches
- You carry aroun nuvoplaruntie-wscript patch, but what is it purpose? Where
is it coming from? Is it upstreamed or taken from upstream? While you
probably know, it would be helpful to others to attache 2 line comment
above the patch to explain the purpose and link the sources. Thx.
* appindicator dependency
- Is it mandatory? Is it possible to disable it? I don't think this is
supported by default in Fedora. As far as I understand it, you need Gnome
Shell KStatusNotifierItem/AppIndicator Support plugin [1] installed to
benefit from this ...
* Space characters on the end of line
- There are several lines containing empty space after the end of line. This
is just minor nit, but probably better to avoid this (the spaces are nicely
visualized by colorized git diff output).
* waf vs waf-3
- While the "waf-3" have to be required to pull in the Python3 version of waf,
I think it should be fine to use the "waf" command. Or do you prefer to be
explicit about it? I leave it up to you.
* description
- I'd keep just the first paragraph of the description (i.e. "Nuvola Apps
is a runtime for semi-sandboxed web apps ...").
Otherwise the package is quite similar to the nuvolaplayer (not surprisingly) and I don't see anything major => APPROVED
[1]: https://extensions.gnome.org/extension/615/appindicator-support/
(In reply to Vít Ondruch from comment #4) > * Please describe patches > - You carry aroun nuvoplaruntie-wscript patch, but what is it purpose? > Where > is it coming from? Is it upstreamed or taken from upstream? While you > probably know, it would be helpful to others to attache 2 line comment > above the patch to explain the purpose and link the sources. Thx. add description > * appindicator dependency > - Is it mandatory? Is it possible to disable it? I don't think this is > supported by default in Fedora. As far as I understand it, you need Gnome > Shell KStatusNotifierItem/AppIndicator Support plugin [1] installed to > benefit from this ... add configure option to disable appindicator > * Space characters on the end of line > - There are several lines containing empty space after the end of line. > This > is just minor nit, but probably better to avoid this (the spaces are > nicely > visualized by colorized git diff output). removed spaces at EOL > * waf vs waf-3 > - While the "waf-3" have to be required to pull in the Python3 version of > waf, > I think it should be fine to use the "waf" command. Or do you prefer to > be > explicit about it? I leave it up to you. will take waf when commit it to git > * description > - I'd keep just the first paragraph of the description (i.e. "Nuvola Apps > is a runtime for semi-sandboxed web apps ..."). > shorten the description as you mentioned > Otherwise the package is quite similar to the nuvolaplayer (not > surprisingly) and I don't see anything major => APPROVED > > > [1]: https://extensions.gnome.org/extension/615/appindicator-support/ thanks for your review Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/nuvolaruntime.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/nuvolaruntime-4.4.0-3.fc25.src.rpm (In reply to mgansser from comment #5) > (In reply to Vít Ondruch from comment #4) > > * Please describe patches > > - You carry aroun nuvoplaruntie-wscript patch, but what is it purpose? > > Where > > is it coming from? Is it upstreamed or taken from upstream? While you > > probably know, it would be helpful to others to attache 2 line comment > > above the patch to explain the purpose and link the sources. Thx. > > add description That is perfect. Thx. > > * description > > - I'd keep just the first paragraph of the description (i.e. "Nuvola Apps > > is a runtime for semi-sandboxed web apps ..."). > > > shorten the description as you mentioned I would remove the first two lines as well ... > thanks for your review YAW Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/nuvolaruntime package has been built successfully on f25, f26 and rawhide. |