Skip to content
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

Open
wants to merge 1 commit into
base: master
from
Open

urserver: init at 3.6.0.745 #87369

wants to merge 1 commit into from

Conversation

@SFrijters
Copy link
Member

SFrijters commented May 9, 2020

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Contributor

aanderse left a comment

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.

nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/urserver.nix Show resolved Hide resolved
nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
@SFrijters SFrijters force-pushed the SFrijters:urserver branch from 06c1ffa to 3246c19 May 9, 2020
@SFrijters SFrijters force-pushed the SFrijters:urserver branch from 3246c19 to 6ac2367 May 9, 2020
@SFrijters
Copy link
Member Author

SFrijters commented May 9, 2020

@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).

@SFrijters SFrijters changed the title urserver: init at 3.6.0.745 WIP urserver: init at 3.6.0.745 May 9, 2020
@SFrijters SFrijters force-pushed the SFrijters:urserver branch 2 times, most recently from fe099b6 to 01e45cf May 10, 2020
@SFrijters SFrijters requested a review from aanderse May 10, 2020
@SFrijters
Copy link
Member Author

SFrijters commented May 10, 2020

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 SFrijters changed the title WIP urserver: init at 3.6.0.745 urserver: init at 3.6.0.745 May 10, 2020
@SFrijters SFrijters force-pushed the SFrijters:urserver branch from 01e45cf to 4c31891 May 15, 2020
@aanderse
Copy link
Contributor

aanderse commented May 18, 2020

@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.

@SFrijters SFrijters force-pushed the SFrijters:urserver branch from 4c31891 to 2dcf859 May 19, 2020
@SFrijters
Copy link
Member Author

SFrijters commented May 23, 2020

@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.

@nixos-discourse
Copy link

nixos-discourse commented May 23, 2020

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

@SFrijters SFrijters force-pushed the SFrijters:urserver branch from 2dcf859 to 604361b Jun 8, 2020
@SFrijters
Copy link
Member Author

SFrijters commented Jun 8, 2020

Rebased on master to stay up-to-date.

@SFrijters SFrijters force-pushed the SFrijters:urserver branch 2 times, most recently from be34009 to 7a96907 Jun 21, 2020
@SFrijters SFrijters force-pushed the SFrijters:urserver branch from 7a96907 to a48c5e3 Jul 5, 2020
@SFrijters
Copy link
Member Author

SFrijters commented Jul 5, 2020

@aanderse is there anything I can do to move this forward? It seems like your call for review on discourse didn't help.

@nixos-discourse
Copy link

nixos-discourse commented Jul 6, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/32

nixos/modules/services/x11/urserver.nix Outdated Show resolved Hide resolved
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.

This comment has been minimized.

Copy link
@doronbehar

doronbehar Jul 7, 2020

Contributor

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.

This comment has been minimized.

Copy link
@SFrijters

SFrijters Jul 12, 2020

Author Member

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.

@SFrijters SFrijters force-pushed the SFrijters:urserver branch from a48c5e3 to 2af10ab Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.