[Bug 1311752] Review Request: vrpn -The Virtual Reality Peripheral Network

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1311752

Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |POST
              Flags|fedora-review?              |fedora-review+



--- Comment #9 from Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> ---
> Seems that it was written to be started by hands but not by any init system. 
> But I think that systemd service is a good option anyway.
Yes, this service file is better than nothing. I'm just saying that this
is something that would be nice to fix in the long run.

> There are several settings that can be specified only as command line options
> (address to listen on etc.). That's why I prefer to allow setting these 
> options via file in /etc/default instead forcing users to use defaults.
This is not a packaging issue, but an upstream issue. But vrpn already has a
config file, so it should simply read whatever options it need itself. Having a
second config file is just a workaround for a stupid deficiency in the server.
If you need to provide configuration options on the command-line, a systemd
unit drop-in override is probably a better option.

But that somewhat matter of opinion, so I'll just get on with the review ;)

+ latest version
+ license is OK (GPLv3+)
- %license should be used for README.Legal
  README doesn't have to be packaged, since it just refers to README.Legal

- %python_provide macro should be used
[https://fedoraproject.org/wiki/Packaging:Python#The_.25python_provide_macro]
+ provides/requires look OK otherwise
+ scriptlets look sane
+ builds and installs OK

Package is APPROVED. Please fix the license stuff and python provides.


You might consider defining %global _description to hold the repetead part of
description. One trick is to do something like this:
%global _description The Virtual........................................... \
..............................................................    \
................................................................  \
.................................................................
(I.e. wrap the text to 80 columns but then merge the first row with %global.)
This way you avoid an empty line in %description. I haven't found a nicer
way to do this.

You can also replace Requires*: systemd lines with %systemd_requires. FPC just
removed the ban a few days ago and it's more consise.

-- 
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
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]