nixos/syncthing: applied rfc42 and added some additional options #226088

Xyz00777 · 2023-04-13T22:29:34Z

Description of changes

try to implement options who are not available as nixos options at the moment, e.g. copyOwnershipFromParent who is a suboptions of folders and can not be implemented at the moment with the extraoptions option.

Things done

its my first commit and merge request for nixos, so i sadly have absolut no plan how to test the new config.
I tried to implement all options i found beside of <minDiskFree unit="%">1</minDiskFree> in options and folders because i didnt know how to implement that and also not <encryptionPassword></encryptionPassword> under folders.device.
If someone could help me i would really like to see that :)

Greetings

Xyz00777 · 2023-04-13T22:37:03Z

i have seen a moment ago that encyptionPassword will already get implemented by #205653.

bjornfor · 2023-04-15T14:36:01Z

Can we use a generic settings* option (NixOS/rfcs#42) instead of having to duplicate every upstream option in downstream nixos?

Xyz00777 · 2023-04-15T20:10:24Z

i have no problems with that @bjornfor . we could do that with the change to 23.05 but not in 22.11 atm because that could be breaking for some users

Xyz00777 · 2023-04-15T21:57:43Z

tested together with @Lassulus and the tests worked

Lassulus · 2023-04-28T19:02:45Z

ok, after spending an hour looking at the code, this would be my plan to move forward:

rename the extraOptions to settings. move the devices & folders to settings and make them into freeformTypes (like extraOptions is now). Then we don't need to serialize all the possibe options for device, folder and global separately and only document the most important ones.

Lassulus · 2023-04-28T21:59:18Z

alright and I spend another hour doing that refactor, although it's not finished yet. I guess we don't need to document all the options (for example startBrowser, releasesUrl, ...)

Xyz00777 · 2023-04-28T22:07:50Z

i just done the aditional like them because i had time :D

nixos/modules/services/networking/syncthing.nix

zarelit · 2023-04-30T20:32:51Z

Related to #144575, too

doronbehar

Looks great to me! That's my only comment.

nixos/modules/services/networking/syncthing.nix

doronbehar · 2023-05-06T17:18:15Z

nixos/modules/services/networking/syncthing.nix

@@ -530,6 +520,10 @@ in {
      This option was removed because Syncthing now has the inotify functionality included under the name "fswatcher".
      It can be enabled on a per-folder basis through the web interface.
    '')
+    (mkRenamedOptionModule [ "services" "syncthing" "extraOptions" ] [ "services" "syncthing" "settings" ])
+    (mkRenamedOptionModule [ "services" "syncthing" "folders" ] [ "services" "syncthing" "settings" "folders" ])


Something like this (along with the other options):

Suggested change

(mkRenamedOptionModule [ "services" "syncthing" "folders" ] [ "services" "syncthing" "settings" "folders" ])

(mkRenamedOptionModule [ "services" "syncthing" "folders" ] [ "services" "syncthing" "settings" "folders" ])

(mkRenamedOptionModule [ "services" "syncthing" "folders" "rescanIntervalS" ] [ "services" "syncthing" "settings" "folders" "rescanInterval" ])

hmm, this gives me an eval error. not sure how we can fix that

Because the folder's path is supposed to be in between the "folders" and the rescanIntervalS... I'm not sure how to do that, as the path can be anything of course.

I tend to think it's impossible to perform such a 'variable' rename. If indeed we won't find a way to do it, I think it'd be OK to give up on this, as anyway we rename an module option a level higher, which might trigger hopefully a second look on the documentation anyway..

Also, in anycase, it'd be nice to mention all of the changes here in the release notes.. (@Xyz00777 )

...so the solution we went with here is really "silently ignore the problem and hope the user will read the release notes"? How is this acceptable?

We should either reintroduce the mapping or add an assertion.

...so the solution we went with here is really "silently ignore the problem and hope the user will read the release notes"? How is this acceptable?

We should either reintroduce the mapping or add an assertion.

Indeed I forgot about the release notes entry... If you know how to create that mapping, or add an assertion, I'd be happy to review and test it. I am not sure it's possible to do it TBH. I also asked for help on discourse and got no response.

I've added an assertion in #232439. We can't use the module system for this because of #96006.

Could you open a PR for the release note entry?

nixos-discourse · 2023-05-08T08:22:16Z

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

https://discourse.nixos.org/t/rename-a-module-option-when-one-components-in-the-option-path-is-a-variable/27946/1

doronbehar · 2023-05-08T08:32:02Z

@Xyz00777 could you please also perform the proper renames in:

nixos/tests/syncthing-init.nix
nixos/tests/syncthing.nix

doronbehar · 2023-05-15T12:12:34Z

Friendly ping @Xyz00777 . Could you help us with the nixos/tests/syncthing{,-init}.nix files?

Xyz00777 · 2023-05-16T14:32:10Z

Hi sorry for my delay, I forgot to answer. I had question @Lassulus because I didn't know how to add them and he mean that he already answered an question like that somewhere.

Thanks for the merge! :)

I will try to get better with nix for doing it myself

azahi · 2023-05-17T12:56:36Z

Looks like this introduced a regression in configuration. Setting for example services.syncthing.settings.options.autoUpgradeIntervalH or services.syncthing.settings.folders.${name}.versioning.params doesn't work anymore for me.

doronbehar · 2023-05-17T13:38:51Z

What do you mean by "doesn't work"? Do you mean that it is essentially ignored?

…

On 17 May 2023 15:56:47 GMT+03:00, Azat Bahawi ***@***.***> wrote: Looks like this introduced a regression in configuration. Setting for example `services.syncthing.settings.options.autoUpgradeIntervalH` or `services.syncthing.settings.folders.${name}.versioning.params` doesn't work anymore for me. -- Reply to this email directly or view it on GitHub: #226088 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

ncfavier · 2023-05-17T13:40:12Z

nixos/modules/services/networking/syncthing.nix

-        cfg.devices != {} || cfg.folders != {} || cfg.extraOptions != {}
+        cfg.settings.devices != {} || cfg.folders != {} || cfg.extraOptions != {}


This looks suspicious

Indeed this looks bad. What do you think about simplify it to:

diff --git i/nixos/modules/services/networking/syncthing.nix w/nixos/modules/services/networking/syncthing.nix index b5ebda6da04..d5531409428 100644 --- i/nixos/modules/services/networking/syncthing.nix +++ w/nixos/modules/services/networking/syncthing.nix @@ -610,7 +610,7 @@ in { }; }; syncthing-init = mkIf ( - cfg.settings.devices != {} || cfg.folders != {} || cfg.extraOptions != {} + cfg.settings != {} ) { description = "Syncthing configuration updater"; requisite = [ "syncthing.service" ];

That condition hasn't prevented the syncthing-init.service from being generated, and this is not the issue @azahi is experiencing. It is still worth fixing though.

azahi · 2023-05-17T13:55:27Z

What do you mean by "doesn't work"? Do you mean that it is essentially ignored?

error: The option `services.syncthing.settings.folders.share.versioning.params' does not exist. Definition values:
       - In `/nix/store/4waix1crf1vadmk5dc1yqsbn7zxkdr4x-source/modules/nixos/syncthing.nix':
           {
             cleanoutDays = "7";
           }

error: A definition for option `services.syncthing.settings.options.crashReportingEnabled' is not of type `JSON value'. Definition values:
       - In `/nix/store/l7165dvk51aahr3bglq8pi6idgh9qwj2-source/modules/nixos/syncthing.nix': false

azahi · 2023-05-17T14:06:00Z

Could this be some kind of an attribute name collision with nixpkgs module system? settings.gui.bar = "baz" or settings.foo.bar = "baz" works fine, only settings.options.bar = "baz" breaks. Or maybe it's something with toJSON?

ncfavier · 2023-05-17T14:07:15Z

nixos/modules/services/networking/syncthing.nix

-                  fsPath = mkOption {
-                    default = "";
-                    type = either str path;
+
+                  id = mkOption {
+                    type = types.str;
                    description = mdDoc ''
-                      Path to the versioning folder.
-                      See <https://docs.syncthing.net/users/versioning.html>.
+                      The device ID. See <https://docs.syncthing.net/dev/device-ids.html>.
                    '';
                  };
-                  params = mkOption {
-                    type = attrsOf (either str path);
+
+                  autoAcceptFolders = mkOption {
+                    type = types.bool;
+                    default = false;
                    description = mdDoc ''
-                      The parameters for versioning. Structure depends on
-                      [versioning.type](#opt-services.syncthing.folders._name_.versioning.type).
-                      See <https://docs.syncthing.net/users/versioning.html>.
+                      Automatically create or share folders that this device advertises at the default path.
+                      See <https://docs.syncthing.net/users/config.html?highlight=autoaccept#config-file-format>.
                    '';
                  };


First error @azahi reports is due to these options being removed without a corresponding freeformType added.

nixos/modules/services/networking/syncthing.nix

ncfavier · 2023-05-17T14:07:53Z

nixos/modules/services/networking/syncthing.nix

              '';
-            };
+              type = types.attrsOf (types.submodule ({ name, ... }: {


Second issue @azahi reports is due to the extra attrsOf here.

ncfavier · 2023-05-17T14:15:26Z

#232439 should fix both issues, please double-check.

nixos/syncthing: fixup #226088

doronbehar · 2023-05-17T19:22:25Z

Well done! Working on a release notes PR right now.

…

On Wed, May 17, 2023 at 12:20:11PM -0700, Naïm Favier wrote: @ncfavier commented on this pull request. > @@ -530,6 +520,10 @@ in { This option was removed because Syncthing now has the inotify functionality included under the name "fswatcher". It can be enabled on a per-folder basis through the web interface. '') + (mkRenamedOptionModule [ "services" "syncthing" "extraOptions" ] [ "services" "syncthing" "settings" ]) + (mkRenamedOptionModule [ "services" "syncthing" "folders" ] [ "services" "syncthing" "settings" "folders" ]) I've added an assertion in #232439. We can't use the module system for this because of #96006. Could you open a PR for the release note entry? -- Reply to this email directly or view it on GitHub: #226088 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

Fixup to NixOS#226088 and NixOS#232439.

RaitoBezarius · 2023-05-18T18:18:56Z

It seems like this PR caused more damage than expected and everyone seems to continue moving forward on this.

Breaking changes were RESTRICTED since a while. I was affected too, please tell me what's your plan forward regarding the blast area caused by those changes? If not, please consider a mass revert of those changes and wait for branch-off which is supposed to take place in 4 days if you cannot stabilize those changes.

ncfavier · 2023-05-22T08:33:43Z

I am proposing to revert this in #233377. Sorry @Xyz00777, bad timing.

Xyz00777 · 2023-05-22T09:00:31Z

Sad but absolutely okay and understandable for me @Lassulus how far is the fix you wanted to make? I didn't tracked all the changes and messages everyone done here...

ncfavier · 2023-05-22T09:11:39Z

I think the main problem is that it's not clear any longer how to detect whether the user intends to manage their configuration declaratively. Previously we could test each of folders, devices and extraOptions, but with this PR extraOptions gets moved into settings which has default values so we can't just check that settings != {}. Maybe we could check options.services.syncthing.settings.isDefined or something but I'm not sure and I don't have time to check.

This check is important to avoid destroying users' imperatively configured folders and devices (see the original motivation for #62157).

Beyond that, as mentioned in #232679 (comment), I am not convinced we need to move folders and devices at all. Putting them inside settings seems to just complicate things for users and developers. I don't think anything prevents us from adding freeformTypes to the existing options.

The breaking change in this patch note has been reverted for now, see NixOS#226088.

The breaking change in this patch note has been reverted for now, see #226088. (cherry picked from commit 2251304)

Ever since this PR [1], some of the Syncthing options that I’ve been using have new names. The old names are deprecated. This commit makes it so that I don’t use the deprecated names anymore. Fixes <mid:bu4dbznoivldxaxkdyzkd472vryl7wqzcvjunrg5pqe7ta5bu4@6pic33ncld5n>. [1]: <NixOS/nixpkgs#226088>

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 13, 2023

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 13, 2023

Lassulus force-pushed the master branch 2 times, most recently from 2f2ccdf to 3d77d9d Compare April 28, 2023 21:02

Lassulus force-pushed the master branch 2 times, most recently from df123d4 to 3498b5b Compare April 28, 2023 22:40

Xyz00777 mentioned this pull request Apr 29, 2023

syncthing: add support for untrusted devices #205653

Closed

12 tasks

Xyz00777 changed the title ~~nixos/syncthing: included new options who are not available at the moment~~ nixos/syncthing: applied rfc42 Apr 29, 2023

Xyz00777 changed the title ~~nixos/syncthing: applied rfc42~~ nixos/syncthing: applied rfc42 and added some additional options Apr 29, 2023

Lassulus force-pushed the master branch from 471b602 to e77b42c Compare April 29, 2023 22:37

schnusch reviewed Apr 30, 2023

View reviewed changes

nixos/modules/services/networking/syncthing.nix Show resolved Hide resolved

Lassulus force-pushed the master branch from e77b42c to 4b164c2 Compare May 3, 2023 15:35

Lassulus mentioned this pull request May 6, 2023

nixos/syncthing: Use API to merge / override configurations #230196

Merged

12 tasks

doronbehar reviewed May 6, 2023

View reviewed changes

Lassulus force-pushed the master branch from 4b164c2 to 380ccbc Compare May 7, 2023 12:57

nixos/syncthing: use rfc42 style settings

32866f8

Lassulus force-pushed the master branch from 380ccbc to 32866f8 Compare May 15, 2023 12:39

doronbehar merged commit 9b0a03f into NixOS:master May 16, 2023

ncfavier reviewed May 17, 2023

View reviewed changes

doronbehar reviewed May 17, 2023

View reviewed changes

nixos/modules/services/networking/syncthing.nix Show resolved Hide resolved

nixos/modules/services/networking/syncthing.nix Show resolved Hide resolved

ncfavier reviewed May 17, 2023

View reviewed changes

ncfavier mentioned this pull request May 17, 2023

nixos/syncthing: fixup #226088 #232439

Merged

12 tasks

ncfavier added a commit to ncfavier/nixpkgs that referenced this pull request May 17, 2023

nixos/syncthing: fixup NixOS#226088

40a2df0

ncfavier added a commit that referenced this pull request May 17, 2023

Merge pull request #232439 from ncfavier/fix-syncthing

10aa44f

nixos/syncthing: fixup #226088

doronbehar added a commit to doronbehar/nixpkgs that referenced this pull request May 17, 2023

release-notes: Mention services.syncthing changes due to RFC 42

84d5e9b

Fixup to NixOS#226088 and NixOS#232439.

doronbehar mentioned this pull request May 17, 2023

release-notes: Mention services.syncthing changes due to RFC 42 #232483

Merged

12 tasks

jupblb mentioned this pull request May 18, 2023

All syncthing folders are always enabled #232574

Closed

RaitoBezarius mentioned this pull request May 18, 2023

syncthing-init broke suddenly because failure to connect to 127.0.0.1:8483 #232679

Closed

ncfavier mentioned this pull request May 22, 2023

Revert "nixos/syncthing: use rfc42 style settings" #233377

Merged

12 tasks

mart-w added a commit to mart-w/nixpkgs that referenced this pull request Jun 2, 2023

nixos/syncthing: Remove unnecessary patch note

2251304

The breaking change in this patch note has been reverted for now, see NixOS#226088.

mart-w mentioned this pull request Jun 2, 2023

nixos/syncthing: Remove unnecessary patch note #235672

Merged

12 tasks

github-actions bot pushed a commit that referenced this pull request Jun 3, 2023

nixos/syncthing: Remove unnecessary patch note

0320e2c

The breaking change in this patch note has been reverted for now, see #226088. (cherry picked from commit 2251304)

Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023

WillPower3309 mentioned this pull request Oct 20, 2023

nixos/syncthing: readd settings.device.<name>.introducer option #262191

Closed

12 tasks

	(mkRenamedOptionModule [ "services" "syncthing" "folders" ] [ "services" "syncthing" "settings" "folders" ])
	(mkRenamedOptionModule [ "services" "syncthing" "folders" ] [ "services" "syncthing" "settings" "folders" ])
	(mkRenamedOptionModule [ "services" "syncthing" "folders" "rescanIntervalS" ] [ "services" "syncthing" "settings" "folders" "rescanInterval" ])

		cfg.devices != {} \|\| cfg.folders != {} \|\| cfg.extraOptions != {}
		cfg.settings.devices != {} \|\| cfg.folders != {} \|\| cfg.extraOptions != {}

nixos/syncthing: applied rfc42 and added some additional options #226088

nixos/syncthing: applied rfc42 and added some additional options #226088

Conversation

Xyz00777 commented Apr 13, 2023 • edited Loading

Description of changes

Things done

Xyz00777 commented Apr 13, 2023

bjornfor commented Apr 15, 2023

Xyz00777 commented Apr 15, 2023 • edited Loading

Xyz00777 commented Apr 15, 2023

Lassulus commented Apr 28, 2023

Lassulus commented Apr 28, 2023

Xyz00777 commented Apr 28, 2023

zarelit commented Apr 30, 2023

doronbehar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented May 8, 2023

doronbehar commented May 8, 2023

doronbehar commented May 15, 2023

Xyz00777 commented May 16, 2023

azahi commented May 17, 2023

doronbehar commented May 17, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azahi commented May 17, 2023

azahi commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncfavier commented May 17, 2023

doronbehar commented May 17, 2023 via email

RaitoBezarius commented May 18, 2023

ncfavier commented May 22, 2023

Xyz00777 commented May 22, 2023

ncfavier commented May 22, 2023

Xyz00777 commented Apr 13, 2023 •

edited

Loading

Xyz00777 commented Apr 15, 2023 •

edited

Loading