-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
urserver: init at 3.6.0.745 #87369
urserver: init at 3.6.0.745 #87369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a review on the module part of this PR. I'll leave package review to someone else.
I hope my comments are helpful, though I don't know anything about this software so it's very likely some of the assumptions I have made are incorrect.
After a quick look at the website it looks like plugins are a big part of this application and I'm wondering how they will be handled.
@aanderse I just pushed an update just to try and make ofborg happy; I will still have to look into implemention your suggestions later (hopefully tomorrow). |
fe099b6
to
01e45cf
Compare
To address the earlier comment about the plugin system: via the web interface additional search paths can be added for remotes, and the default ones that come with the installer are put into the nix store. Since this service is now running as the user, the configuration ends up in $HOME/.urserver and can be updated manually. So maybe this is not as declarative as one would ideally like, but I don't think it should be a showstopper to be honest. I use the remote pretty much only as something to move my mouse with the basic remote, so I also have no experience with expanding it myself. |
@SFrijters good work on the module. I'd like to hear some opinions on the firewall options from some other people, though. I haven't reviewed the package so we'll need someone to take a look at that. |
@aanderse Thanks again for reviewing. Do you have any suggestions for people to ask for the package review? Those mentioned in the sidebar have already been tagged. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/166 |
Rebased on master to stay up-to-date. |
be34009
to
7a96907
Compare
@aanderse is there anything I can do to move this forward? It seems like your call for review on discourse didn't help. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
description = '' | ||
TCP ports that are used by urserver. | ||
By default port 9510 is used for the web server. | ||
By default port 9512 is used for WiFi connections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be sufficient to deal with the firewall reminder, with an additional line here saying that ports should be opened in order for the service to be exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at https://github.com/Infinisil/rfcs/blob/config-option/rfcs/0042-config-option.md#default-values and the example below it seems desirable that the default firewall ports are opened when the service is enabled?
I'm pushing a very minimal service implementation now. The proposed settings
bit is still just a proposal, right? It didn't seem to work for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems better but I haven't tested it.
FWIW, it works on my machine :) |
@jonringer Taking you up on your offer on Discourse - could you take a look at this? Either way I would like to take this off my to-do list, so if this is not something that should be in nixpkgs or nix modules just let me know that as well, then I will just keep it in my local fork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be two commits:
urserver: init at 3.6.0.745
nixos/urserver: init
the autoPatchelfHook makes the wrapProgram
unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Result of nixpkgs-review pr 87369 1
1 package blacklisted:
- tests.nixos-functions.nixos-test
1 package built:
- urserver
Thanks for approving! |
Motivation for this change
Unified Remote is a remote control app for your computer. It requires the 'urserver' to run on the computer so you can control it with your smartphone.
The application itself was tested on both NixOS and Ubuntu, the service only on NixOS.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)