nixos/kdeconnect: Added programs.kdeconnect option #63899

pasqui23 · 2019-06-28T16:12:20Z

Motivation for this change

KdeConnect requires to open TCP and UDP ports to work properly

Things done

matthiasbeyer

Diff looks good to me.

bjornfor

Please update commit message to "nixos: add programs.kdeconnect option" and add an "...openFirewall" boolean option that defaults to false.

bjornfor · 2019-09-15T14:59:42Z

nixos/modules/programs/kdeconnect.nix

+      default = pkgs.kdeconnect;
+      type = types.package;
+      description=''
+        The package providing the implementation for kdeconnect


Missing period at end of sentence.

nixos/modules/programs/kdeconnect.nix

bjornfor

Please squash fixup commit. And this still applies: Please update commit message to "nixos: add programs.kdeconnect option" and add an "...openFirewall" boolean option that defaults to false.

pasqui23 · 2019-09-16T19:48:45Z

On 16/09/19 13:50, Bjørn Forsman wrote: @bjornfor requested changes on this pull request. add an "...openFirewall" boolean option that defaults to false. Kdeconnect won't work at all without those ports open,and it won't even give any error message

bjornfor · 2019-09-17T18:06:34Z

Kdeconnect won't work at all without those ports open,and it won't even give any error message

Right. But most nixos options don't automatically open the firewall, even though opening the firewall is the "correct" thing to do in many cases.

I guess this is a bit different than running a webserver, since that can be accessed via localhost, whereas for this program that doesn't make sense.

On the other hand, you're pretty likely to see the openFirewall option when you search for kdeconnect in nixos config options.

Let's get input from other community members.

Please fix git history so that it's ready to merge.

infinisil · 2019-10-30T12:42:33Z

I guess another reason to not open ports by default is that users might be running other things on those ports already, which would then be open for anybody. An openFirewall option is like a warning sign to the user that ports are being opened, they should make sure those aren't used by something else already (the ports should ideally be mentioned in the description).

You could also add

{
  assertions = [{
    assertion = cfg.openFirewall;
    message = "You need to enable `programs.kdeconnect.openFirewall` to use KDEConnect, which opens ports ${portRange}.";
  }];
}

to make sure it's enabled.

pasqui23 · 2019-11-09T11:29:00Z

@infinisil I don't like that a module needs 2 flags set to true when any other module only requre 1,with maybe some other more specific configuration.
I have clarified that the module will open the given port

bjornfor · 2019-12-29T15:21:13Z

nixos/modules/programs/kdeconnect.nix

+    let
+      cfg = config.programs.kdeconnect;
+    in mkIf cfg.enable {
+      environment.systemPackages = [cfg.package];


Perhaps also add gsconnect to systemPackages, if desktop is gnome 3?

Maybe in a further commit

stale · 2020-06-26T15:52:01Z

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the related code and @ mention them in a comment.
Ask on the NixOS Discourse.
Ask on the #nixos channel on irc.freenode.net.

pasqui23 · 2020-06-29T00:54:41Z

Still waiting on @infinisil 'S review

oxalica

There are some format issues. You can run nix run nixpkgs.nixpkgs-fmt -c nixpkgs-fmt <path_to_nix_file> to fix most of them.

Also I agree the comment #63899 (review) that it's better to squash all commits and tweak commit message.

nixos/modules/programs/kdeconnect.nix

nixos-discourse · 2020-10-08T22:40:17Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/331

doronbehar · 2020-11-13T08:00:22Z

@pasqui23 you still have some typos mentioned in the reviews not fixed in current state of the PR.

Kiwi · 2020-12-20T10:32:07Z

I didn't mean to remove @infinisil >.> I'll try and get around to making the changes sometime if it's still open later

nixos-discourse · 2021-02-11T15:18:58Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/465

nixos-discourse · 2021-03-20T23:30:03Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/485

doronbehar

@pasqui23 could you please address @oxalica's review? The typos / formatting may not be super important for functionality but we'd like these kind of things to be consistent across Nixpkgs.

nixos/modules/programs/kdeconnect.nix

doronbehar · 2021-03-21T09:07:12Z

nixos/modules/programs/kdeconnect.nix

+  options.programs.kdeconnect = {
+    enable = mkEnableOption ''
+      kdeconnect.Note that it will open the TCP and UDP port from
+      1714 to 1764 as they are needed for its correct working


Suggested change

1714 to 1764 as they are needed for its correct working

1714 to 1764 as they are needed for it to function properly.

You can use the <option>package</option> to use

<code>gnomeExtensions.gsconnect</code> as an alternative

implementation if you use Gnome.

oxalica

LGTM. Not sure if gsconnect should go here.

nixos/modules/programs/kdeconnect.nix

infinisil · 2021-05-03T17:00:40Z

Does this have any integration with plasma? Should enabling plasma also enable this option?

pasqui23 · 2021-05-03T19:46:13Z

I don't think so, I'm using it on windows too and there is gsconnect for GNOME. This is mostly so it opens the right ports.

infinisil · 2021-05-03T19:55:27Z

I guess this looks good after a squash

nixos-discourse · 2021-06-09T07:26:30Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/540

pasqui23 · 2021-06-09T09:05:19Z

@infinisil I've squashed all the commits in one. Are there any other blockers?

doronbehar · 2021-06-26T21:00:21Z

Let's merge this old PR :).

pasqui23 requested a review from infinisil as a code owner June 28, 2019 16:12

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jun 28, 2019

pasqui23 changed the title ~~Nixos/kdeconnect:Added programs.kdeconnect option~~ nixos/kdeconnect:Added programs.kdeconnect option Jun 30, 2019

matthiasbeyer approved these changes Jul 3, 2019

View reviewed changes

bjornfor requested changes Sep 15, 2019

View reviewed changes

bjornfor reviewed Sep 15, 2019

View reviewed changes

nixos/modules/programs/kdeconnect.nix Show resolved Hide resolved

bjornfor requested changes Sep 16, 2019

View reviewed changes

pasqui23 force-pushed the kconnect branch from 29d3b95 to f4ede7a Compare September 17, 2019 23:27

bjornfor reviewed Dec 29, 2019

View reviewed changes

stale bot added 2.status: stale and removed 2.status: stale labels Jun 26, 2020

oxalica suggested changes Sep 26, 2020

View reviewed changes

pasqui23 force-pushed the kconnect branch from 203e0bc to edf9ce6 Compare September 26, 2020 13:22

ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 labels Sep 26, 2020

Kiwi requested review from Kiwi and removed request for infinisil December 20, 2020 10:16

Kiwi requested a review from infinisil December 20, 2020 10:28

SuperSandro2000 changed the title ~~nixos/kdeconnect:Added programs.kdeconnect option~~ nixos/kdeconnect: Added programs.kdeconnect option Feb 11, 2021

doronbehar added the 2.status: work-in-progress label Feb 15, 2021

doronbehar requested changes Mar 21, 2021

View reviewed changes

doronbehar mentioned this pull request Mar 21, 2021

GSConnect requires manual iptables configuration to work #116388

Closed

oxalica approved these changes Mar 21, 2021

View reviewed changes

oxalica suggested changes Mar 22, 2021

View reviewed changes

nixos/modules/programs/kdeconnect.nix Outdated Show resolved Hide resolved

oxalica approved these changes Apr 20, 2021

View reviewed changes

nixos: add programs.kdeconnect option

42af3b3

pasqui23 force-pushed the kconnect branch from 149416d to 42af3b3 Compare May 15, 2021 23:49

oxalica requested a review from bjornfor June 9, 2021 07:19

doronbehar approved these changes Jun 10, 2021

View reviewed changes

SuperSandro2000 requested a review from aanderse June 26, 2021 19:59

aanderse approved these changes Jun 26, 2021

View reviewed changes

doronbehar merged commit aedc67d into NixOS:master Jun 26, 2021

Shawn8901 mentioned this pull request Oct 3, 2023

kdeconnect-kde needs ports 1714:1764 open for udp and tcp #258690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/kdeconnect: Added programs.kdeconnect option #63899

nixos/kdeconnect: Added programs.kdeconnect option #63899

pasqui23 commented Jun 28, 2019 •

edited

Loading

matthiasbeyer left a comment

bjornfor left a comment

bjornfor Sep 15, 2019

bjornfor left a comment

pasqui23 commented Sep 16, 2019 via email

bjornfor commented Sep 17, 2019

infinisil commented Oct 30, 2019

pasqui23 commented Nov 9, 2019

bjornfor Dec 29, 2019

pasqui23 Jun 29, 2020

stale bot commented Jun 26, 2020

pasqui23 commented Jun 29, 2020

oxalica left a comment

nixos-discourse commented Oct 8, 2020

doronbehar commented Nov 13, 2020

Kiwi commented Dec 20, 2020

nixos-discourse commented Feb 11, 2021

nixos-discourse commented Mar 20, 2021

doronbehar left a comment

doronbehar Mar 21, 2021

oxalica left a comment

infinisil commented May 3, 2021

pasqui23 commented May 3, 2021

infinisil commented May 3, 2021

nixos-discourse commented Jun 9, 2021

pasqui23 commented Jun 9, 2021

doronbehar commented Jun 26, 2021

-to 1764 as they are needed for its correct working
+to 1764 as they are needed for it to function properly.
+      You can use the <option>package</option> to use
+      <code>gnomeExtensions.gsconnect</code> as an alternative
+      implementation if you use Gnome.

nixos/kdeconnect: Added programs.kdeconnect option #63899

nixos/kdeconnect: Added programs.kdeconnect option #63899

Conversation

pasqui23 commented Jun 28, 2019 • edited Loading

Motivation for this change

Things done

matthiasbeyer left a comment

Choose a reason for hiding this comment

bjornfor left a comment

Choose a reason for hiding this comment

bjornfor Sep 15, 2019

Choose a reason for hiding this comment

bjornfor left a comment

Choose a reason for hiding this comment

pasqui23 commented Sep 16, 2019 via email

bjornfor commented Sep 17, 2019

infinisil commented Oct 30, 2019

pasqui23 commented Nov 9, 2019

bjornfor Dec 29, 2019

Choose a reason for hiding this comment

pasqui23 Jun 29, 2020

Choose a reason for hiding this comment

stale bot commented Jun 26, 2020

pasqui23 commented Jun 29, 2020

oxalica left a comment

Choose a reason for hiding this comment

nixos-discourse commented Oct 8, 2020

doronbehar commented Nov 13, 2020

Kiwi commented Dec 20, 2020

nixos-discourse commented Feb 11, 2021

nixos-discourse commented Mar 20, 2021

doronbehar left a comment

Choose a reason for hiding this comment

doronbehar Mar 21, 2021

Choose a reason for hiding this comment

oxalica left a comment

Choose a reason for hiding this comment

infinisil commented May 3, 2021

pasqui23 commented May 3, 2021

infinisil commented May 3, 2021

nixos-discourse commented Jun 9, 2021

pasqui23 commented Jun 9, 2021

doronbehar commented Jun 26, 2021

pasqui23 commented Jun 28, 2019 •

edited

Loading