rss2email module: init #49228

Ekleog · 2018-10-27T09:37:45Z

Also adding system-sendmail package for sharing the code with other
modules or packages needing it.

The test for this module depends on #48901 being merged, as the configuration format of OpenSMTPD changed. Hence the fact it's based on the tip of #48901.

cc @Profpatsch

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nox --run "nox-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)
Fits CONTRIBUTING.md.

Ekleog · 2018-10-27T09:46:32Z

@GrahamcOfBorg test rss2email

GrahamcOfBorg · 2018-10-27T09:48:41Z

Success on aarch64-linux (full log)

Attempted: tests.rss2email

Partial log (click to expand)

server: exit status 1
syncing
server: running command: sync
server: exit status 0
test script finished in 15.85s
cleaning up
killing server (pid 631)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/2nx6rhcksi6ccc9dyzfsnsjkqcsrqwl2-vm-test-run-opensmtpd

GrahamcOfBorg · 2018-10-27T09:57:30Z

Failure on x86_64-linux (full log)

Attempted: tests.rss2email

Partial log (click to expand)

server: running command: systemctl --no-pager show "rss2email"
server: exit status 0
error: unit ‘rss2email’ is inactive and there are no pending jobs
unit ‘rss2email’ is inactive and there are no pending jobs
cleaning up
killing server (pid 597)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
builder for '/nix/store/kypkz206qi0lgq24x95aavbjbdqmikk1-vm-test-run-opensmtpd.drv' failed with exit code 255
error: build of '/nix/store/kypkz206qi0lgq24x95aavbjbdqmikk1-vm-test-run-opensmtpd.drv' failed

Ekleog · 2018-10-27T10:12:09Z

Looks like my sleep was optimistic despite being pessimistic… let's try again with something hopefully more resilient.

@GrahamcOfBorg test rss2email

GrahamcOfBorg · 2018-10-27T10:12:53Z

Success on aarch64-linux (full log)

Attempted: tests.rss2email

Partial log (click to expand)

server: exit status 1
syncing
server: running command: sync
server: exit status 0
test script finished in 14.75s
cleaning up
killing server (pid 631)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/wqya4ypjxnxlq8y0mirbx15l79adc50y-vm-test-run-opensmtpd

GrahamcOfBorg · 2018-10-27T10:12:56Z

Success on x86_64-linux (full log)

Attempted: tests.rss2email

Partial log (click to expand)

server: exit status 1
syncing
server: running command: sync
server: exit status 0
test script finished in 19.56s
cleaning up
killing server (pid 597)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/q22xxxa68p9hczzs3q1dmwb0is9g5fjy-vm-test-run-opensmtpd

GrahamcOfBorg · 2018-10-27T10:24:37Z

Success on aarch64-linux (full log)

Attempted: opensmtpd

Partial log (click to expand)

/nix/store/ihdq6cklcsdnkac9x6ly2y5r8bir707a-opensmtpd-6.4.0p1

GrahamcOfBorg · 2018-10-27T10:25:03Z

Success on x86_64-linux (full log)

Attempted: opensmtpd

Partial log (click to expand)

/nix/store/qwd5xz5k043v5v11xm90zjkmpsnphj3v-opensmtpd-6.4.0p1

nixos/modules/services/mail/rss2email.nix

Ekleog · 2018-10-28T13:38:44Z

Thanks! I have applied your suggestion and rebased on master (as the opensmtpd update has been merged).

This should be ready to merge :)

@GrahamcOfBorg test rss2email

GrahamcOfBorg · 2018-10-28T13:45:03Z

Success on aarch64-linux (full log)

Attempted: tests.rss2email

Partial log (click to expand)

server: exit status 1
syncing
server: running command: sync
server: exit status 0
test script finished in 26.00s
cleaning up
killing server (pid 631)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/z47369agysjw8n2shahvzr9blq46vhr2-vm-test-run-opensmtpd

GrahamcOfBorg · 2018-10-28T13:51:57Z

Success on x86_64-linux (full log)

Attempted: tests.rss2email

Partial log (click to expand)

server: exit status 1
syncing
server: running command: sync
server: exit status 0
test script finished in 185.39s
cleaning up
killing server (pid 600)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/d8izh96fa61d5qysdx2bqqh0zkg5fgg5-vm-test-run-opensmtpd

nixos/modules/services/mail/rss2email.nix

infinisil · 2018-10-28T21:44:55Z

nixos/modules/services/mail/rss2email.nix

+
+            to = mkOption {
+              type = with types; nullOr str;
+              default = null;


Along with the config change, you can then make these changes here:

Change the type to str

Remove the default value here

Set the default by doing to = mkDefault cfg.to; in the config section of the submodule.

This is possible indeed, but it moves the module farther away from upstream configuration, as upstream configuration just doesn't add a to, and I tried to replicate upstream behavior as well as possible… now you're right, there's little else that could legitimately be done other than using the system-wide to, so I guess this won't change visible behavior :)

Ah, I think your current implementation always just defaults to cfg.to though (line 12). So if you want to replicate upstream behavior, you should probably just not set it there instead.

I think lines 11-12 correctly set url from feed and add the to field iff. cfg.to is not null, thanks to the optionalAttrs (and testing validates this, if I'm not mistaken on the testing process).

I guess you confused with line 9, where I did set to unconditionally, but in the [DEFAULT] block?

nixos/modules/services/mail/rss2email.nix

pkgs/servers/mail/system-sendmail/default.nix

Ekleog · 2018-10-29T14:24:57Z

@infinisil Thank you for the review! I've hopefully addressed all your points and pushed an updated version, that I'm going to start trying on my server instead of the old version :)

Ekleog · 2018-11-01T15:17:01Z

It looks like @bjornfor's comment as well as my answer vanished on the push --force required for handling the merge conflict that arose from another package adding its id to ids.nix.

I reproduce them here for convenience: (source)

(on the if which sendmail > /dev/null 2>&1; then line of system-sendmail)
@bjornfor

If you change which to command -v you remove one implicit dependency. (command -v is part of posix sh, whereas which is an external binary thay may or may not exist on the system.)

@Ekleog

Good point! Actually this mistake of mine hid a bug in the previous system-sendmail: when it's in $PATH then it infinite-looped.

Both issues are fixed in latest push -f (required due to a merge conflict)

infinisil · 2018-11-07T21:48:24Z

nixos/modules/services/mail/rss2email.nix

+            imap-port = 143;
+            imap-ssl = false;
+            imap-mailbox = "INBOX";
+            verbose = "warning";


Now that we don't encode the default in the module anymore, I think this here should be removed as well and potentially link to some upstream default docs if available.

Indeed, and actually these are in the manpage… or should be, if rss2email had had a release in the last 3 years. Well, with @Profpatsch we now have access to rss2email, so should be able to cleanup and make a release as soon as we get some time :)

I've pointed to the online r2e.1 for the time being.

oxij · 2018-11-14T05:08:14Z

`system-sendmail` has multiple lines in `meta.description`, I think this is generally frowned upon. Also, as a side note, I think it would be good if somebody would finally add MDA support to `rss2email`. It is kinda crazy to run the whole of Postfix just to save files to users' maildirs. (I planned to do it 4 years ago...)

Ekleog · 2018-11-14T08:26:50Z

@oxij Thank you for the comment! I've put it on one line, as it didn't really deserve multiple lines anyway.

As for MDA support, there already is, but there hasn't been a release for a few years and we didn't find time to review enough the changes since last release to trigger one yet. In the meantime, OpenSMTPD makes for something much more lightweight than postfix :)

@infinisil I've rebased, fixed @oxij's concern, and added passthru.tests to rss2email, as its RFC has been merged since. If there are no remaining concerns would it be possible to merge before the next merge conflict on uid? :)
For DynamicUser, the problem stems from our current nscd/systemd setup that is broken, it looks like it'll be possible to switch to DynamicUser when it'll be fixed: systemd is able to migrate the directory seamlessly when moving from User= to DynamicUser= (but not the other way around, so it'll require a stateVersion barrier).

oxij · 2018-11-14T10:15:17Z

Looking at it some more: - I think - system-sendmail = callPackage ../servers/mail/system-sendmail { }; + system-sendmail = lowPrio (callPackage ../servers/mail/system-sendmail { }); and - platforms = platforms.linux; + platforms = platforms.unix; - Also, about defaulting to `1h`. Would it recheck all feeds every hour or would it respect recheck time supplied by the feeds themselves? It would be unfortunate if the former. In any case, I suggest setting it to at least `12h` by default.

Ekleog · 2018-11-14T13:41:16Z

Jan Malakhovski <notifications@github.com> writes:

Looking at it some more: - I think - system-sendmail = callPackage ../servers/mail/system-sendmail { }; + system-sendmail = lowPrio (callPackage ../servers/mail/system-sendmail { });

`system-sendmail` will be a dependency of `mdadm` (once #49229 will be merged), and it's a script that's literally just writing a file, so I'm not sure it really deserves being `lowPrio`. Also, from `rg sendmail pkgs`, there seem to be quite a few packages that would benefit from using it instead of hardcoding postfix's security wrapper path… I guess no one ever tried to use them with a decent smtpd that doesn't rely on setuid when it doesn't need to :p

and - platforms = platforms.linux; + platforms = platforms.unix;

Done

- Also, about defaulting to `1h`. Would it recheck all feeds every hour or would it respect recheck time supplied by the feeds themselves? It would be unfortunate if the former. In any case, I suggest setting it to at least `12h` by default.

Honestly, I have no idea. This should be raised on [1], I guess. Raising to 12hr sounds good to me, I had literally no idea what to put in there by default :) [1] https://github.com/rss2email/rss2email/issues I've pushed the updated version with the `linux` -> `unix` and `1h` -> `12h` changes. Thank you for your review!

oxij · 2018-11-14T15:52:37Z

> - system-sendmail = callPackage ../servers/mail/system-sendmail { }; > + system-sendmail = lowPrio (callPackage ../servers/mail/system-sendmail { }); `system-sendmail` will be a dependency of `mdadm` (once #49229 will be merged), and it's a script that's literally just writing a file, so I'm not sure it really deserves being `lowPrio`.

`lowPrio` AFAIU lowers the priority for `buildEnv` and such, i.e. with `lowPrio` it should not override other `sendmail`s if you install several of them into `systemPackages`, which seems like a good idea to me.

Also, from `rg sendmail pkgs`, there seem to be quite a few packages that would benefit from using it instead of hardcoding postfix's security wrapper path… I guess no one ever tried to use them with a decent smtpd that doesn't rely on setuid when it doesn't need to :p

Yes, I agree, I also grepped over `nixos` folder and got a bit discouraged by the amount of `sendmail` hackery there. Looking at the first few I wanted to propose to fix them too, but then it turned out there's a lot stuff to fix there, so I added it to my own low-prio TODO in case I get bored sometime.

> - Also, about defaulting to `1h`. Would it recheck all feeds every hour or would it respect recheck time supplied by the feeds themselves? It would be unfortunate if the former. In any case, I suggest setting it to at least `12h` by default. Honestly, I have no idea. This should be raised on [1], I guess. Raising to 12hr sounds good to me, I had literally no idea what to put in there by default :)

I looked at the source and it does the wrong thing. IMHO, ideally, `rss2email` should just run as a daemon and handle all timeouts itself since there's no single timeout that works well for all RSS feeds (added to TODO). But given current state of `rss2email` the `12h` is the sanest thing that can be done for now, IMHO.

Also adding `system-sendmail` package for sharing the code with other modules or packages needing it.

Ekleog · 2018-11-15T14:45:35Z

Jan Malakhovski <notifications@github.com> writes:

>> - system-sendmail = callPackage ../servers/mail/system-sendmail { }; >> + system-sendmail = lowPrio (callPackage ../servers/mail/system-sendmail { }); > > `system-sendmail` will be a dependency of `mdadm` (once > #49229 will be merged), and it's a > script that's literally just writing a file, so I'm not sure it really > deserves being `lowPrio`. `lowPrio` AFAIU lowers the priority for `buildEnv` and such, i.e. with `lowPrio` it should not override other `sendmail`s if you install several of them into `systemPackages`, which seems like a good idea to me.

Oh indeed I'm stupid, was thinking that was for lowering the priority of the hydra job… which would make no sense. And say I once knew it :( too many things. Fixed :)

> Also, from `rg sendmail pkgs`, there seem to be quite a few packages > that would benefit from using it instead of hardcoding postfix's > security wrapper path… I guess no one ever tried to use them with a > decent smtpd that doesn't rely on setuid when it doesn't need to :p Yes, I agree, I also grepped over `nixos` folder and got a bit discouraged by the amount of `sendmail` hackery there. Looking at the first few I wanted to propose to fix them too, but then it turned out there's a lot stuff to fix there, so I added it to my own low-prio TODO in case I get bored sometime.

Yeah, overall I'm thinking `system-sendmail` will be a good “factored” hackery to at least make everything agree on the behavior. The issue overall is that some packages need to depend on `sendmail`, and `sendmail` is by essence impure, as it depends on the mailer installed. NixOS could handle this better, but Nixpkgs in itself can't. And as it can even be provided by things that relay to external mail servers (eg. `msmtp`), we can't even just rely on a `sendmail` that pipes the mail to `localhost:25`. Maybe some hacks could be done with `/nix/var/run/sendmail-pipe` or similar stuff, but… let's say I'm not sure it'd actually be helpful.

>> - Also, about defaulting to `1h`. Would it recheck all feeds every hour or would it respect recheck time supplied by the feeds themselves? It would be unfortunate if the former. In any case, I suggest setting it to at least `12h` by default. > > Honestly, I have no idea. This should be raised on [1], I guess. > > Raising to 12hr sounds good to me, I had literally no idea what to put in there by default :) I looked at the source and it does the wrong thing. IMHO, ideally, `rss2email` should just run as a daemon and handle all timeouts itself since there's no single timeout that works well for all RSS feeds (added to TODO). But given current state of `rss2email` the `12h` is the sanest thing that can be done for now, IMHO.

At that point I'd guess you'd maybe do it better by just writing a `rss2email` daemon from scratch, I'm not sure you could re-use lots of current `rss2email` apart from its RSS-parsing library and its mail-sending library? Anyway, this is getting kind of off-topic :) I think all the concerns are now solved, and there appear not to be merge conflicts currently.

oxij · 2018-11-15T15:25:42Z

LGTM. Thanks!

NixOS could handle this better, but Nixpkgs in itself can't.

We can make everything use `system-sendmail` and make the latter inspect some environment variable that NixOS would then set, though.

7c6f434c · 2018-11-22T08:06:12Z

(triage) @infinisil are all your concerns addressed?

infinisil · 2018-11-23T20:51:56Z

@7c6f434c Yeah they're addressed. I still don't like the wrapper thing, but I guess if that's the only way then sure.

7c6f434c · 2018-11-23T22:30:25Z

I see why you don't like the need for the wrapper, but I also don't see a clean and cheap way around that.

Ekleog mentioned this pull request Oct 27, 2018

mdadm: use shared system-sendmail #49229

Merged

9 tasks

GrahamcOfBorg added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) labels Oct 27, 2018

Ekleog force-pushed the rss2email-module branch from 9ae447a to 366f408 Compare October 27, 2018 10:11

Ekleog force-pushed the rss2email-module branch from 366f408 to b483c03 Compare October 27, 2018 10:13

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Oct 27, 2018

Profpatsch reviewed Oct 27, 2018

View reviewed changes

nixos/modules/services/mail/rss2email.nix Outdated Show resolved Hide resolved

GrahamcOfBorg added the 2.status: merge conflict label Oct 28, 2018

Ekleog force-pushed the rss2email-module branch from ec598b2 to 829c58e Compare October 28, 2018 13:38

GrahamcOfBorg removed 8.has: changelog 8.has: documentation 2.status: merge conflict labels Oct 28, 2018

infinisil requested changes Oct 28, 2018

View reviewed changes

Ekleog force-pushed the rss2email-module branch from 829c58e to fbf1323 Compare October 29, 2018 14:15

infinisil mentioned this pull request Oct 29, 2018

nixos/darkhttpd: module to enable darkhttpd #48036

Merged

9 tasks

Ekleog force-pushed the rss2email-module branch from 29d7b28 to 4bc057c Compare November 1, 2018 15:12

infinisil reviewed Nov 7, 2018

View reviewed changes

Ekleog force-pushed the rss2email-module branch 2 times, most recently from 6cfb4f9 to 229519b Compare November 10, 2018 03:47

arianvp mentioned this pull request Nov 12, 2018

DynamicUser is broken #50273

Closed

Ekleog force-pushed the rss2email-module branch from 229519b to 1447f8e Compare November 14, 2018 08:04

GrahamcOfBorg added the 2.status: merge conflict label Nov 14, 2018

Ekleog force-pushed the rss2email-module branch from 1447f8e to cb4963d Compare November 14, 2018 08:24

GrahamcOfBorg removed the 2.status: merge conflict label Nov 14, 2018

Ekleog force-pushed the rss2email-module branch from cb4963d to 8cf6236 Compare November 14, 2018 13:39

GrahamcOfBorg added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 labels Nov 14, 2018

rss2email module: init

0483ce0

Also adding `system-sendmail` package for sharing the code with other modules or packages needing it.

Ekleog force-pushed the rss2email-module branch from 8cf6236 to 0483ce0 Compare November 15, 2018 14:44

infinisil mentioned this pull request Nov 16, 2018

NixOS services: When to have a package option #50476

Closed

infinisil approved these changes Nov 23, 2018

View reviewed changes

7c6f434c merged commit 5e159d4 into NixOS:master Nov 23, 2018

Ekleog mentioned this pull request Jan 16, 2019

mailutils: use system-sendmail instead of sendmailPath #54049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rss2email module: init #49228

rss2email module: init #49228

Ekleog commented Oct 27, 2018

Ekleog commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

Ekleog commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

Ekleog commented Oct 28, 2018

GrahamcOfBorg commented Oct 28, 2018

GrahamcOfBorg commented Oct 28, 2018

infinisil Oct 28, 2018

Ekleog Oct 29, 2018

infinisil Oct 29, 2018

Ekleog Oct 29, 2018

Ekleog commented Oct 29, 2018

Ekleog commented Nov 1, 2018

infinisil Nov 7, 2018

Ekleog Nov 10, 2018

oxij commented Nov 14, 2018 via email

Ekleog commented Nov 14, 2018

oxij commented Nov 14, 2018 via email

Ekleog commented Nov 14, 2018 via email

oxij commented Nov 14, 2018 via email

Ekleog commented Nov 15, 2018 via email

oxij commented Nov 15, 2018 via email

7c6f434c commented Nov 22, 2018

infinisil commented Nov 23, 2018

7c6f434c commented Nov 23, 2018

rss2email module: init #49228

rss2email module: init #49228

Conversation

Ekleog commented Oct 27, 2018

Things done

Ekleog commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

Ekleog commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

GrahamcOfBorg commented Oct 27, 2018

Ekleog commented Oct 28, 2018

GrahamcOfBorg commented Oct 28, 2018

GrahamcOfBorg commented Oct 28, 2018

infinisil Oct 28, 2018

Choose a reason for hiding this comment

Ekleog Oct 29, 2018

Choose a reason for hiding this comment

infinisil Oct 29, 2018

Choose a reason for hiding this comment

Ekleog Oct 29, 2018

Choose a reason for hiding this comment

Ekleog commented Oct 29, 2018

Ekleog commented Nov 1, 2018

infinisil Nov 7, 2018

Choose a reason for hiding this comment

Ekleog Nov 10, 2018

Choose a reason for hiding this comment

oxij commented Nov 14, 2018 via email

Ekleog commented Nov 14, 2018

oxij commented Nov 14, 2018 via email

Ekleog commented Nov 14, 2018 via email

oxij commented Nov 14, 2018 via email

Ekleog commented Nov 15, 2018 via email

oxij commented Nov 15, 2018 via email

7c6f434c commented Nov 22, 2018

infinisil commented Nov 23, 2018

7c6f434c commented Nov 23, 2018