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

Services: Auto-enable dependencies #251

Merged
merged 1 commit into from Oct 20, 2020

Conversation

erikarvstedt
Copy link
Collaborator

@erikarvstedt erikarvstedt commented Oct 18, 2020

Also, add missing dependencies to bitcoind, because bitcoind.enable is false outside of secure-node.nix.

Comparison: auto-enable vs. using assertions

Pros:

  • Less code
  • Adheres to the NixOS philosophy that services should automatically enable their dependencies
  • Works like cryptoanarchy-deb-repo-builder, where, e.g., sudo apt install btcpayserver automatically installs all deps, including lnd.
  • Enables auto-scenarios for all services, like run-tests.sh -s btcpayserver or run-tests.sh -s spark-wallet

Cons:

  • The user might implicitly enable a lightning service and forget to back up its data. This is taken care of by the backup module.

Details

Note that we can't use the following elegant definition in btcpayserver:

services.${cfg.btcpayserver.lightningBackend}.enable = true;

This leads to infinite recursion due to the intricacies of module evaluation.
Instead, we use

services.clightning.enable = mkIf (cfg.btcpayserver.lightningBackend == "clightning") true;
services.lnd.enable = mkIf (cfg.btcpayserver.lightningBackend == "lnd") true;

@erikarvstedt
Copy link
Collaborator Author

Travis just had a spurious failure: The CI build just stopped while running the VM test, but the test itself didn't fail.
Solved by re-pushing the commit with a changed date.

@nixbitcoin
Copy link
Member

If this is the NixOS way to do it, I think we should be consistent with NixOS user expectations. So concept ACK.

@jonasnick stated in #189 (comment) referring to modules implicitly enabling other modules, "I'd prefer to not do that to allow users to easily see and opt-in to what software they actually use on their node (for now at least... the number of modules is still manageable)." Has your opinion changed?

@nixbitcoin
Copy link
Member

nixbitcoin commented Oct 19, 2020

We should also probably remove the Only available if comments in examples/configuration.nix

@erikarvstedt
Copy link
Collaborator Author

Thanks, I've updated examples/configuration.nix.
I've left the webindex section unchanged. Fixing webindex to better adapt to the enabled services is on my to-do list.

@jonasnick
Copy link
Member

It's fine either way. I'd personally still prefer being reminded of the dependencies but I can see that this is generally unnecessary. And if everyone has to manually enable the dependencies that's a lot of code duplication in a sense. Also, it's well documented now.

Copy link
Member

@nixbitcoin nixbitcoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 67e49fe4154ef188f920f1a7cd2f16bdf4ed39bb
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEV3o0Un8+KoXoD+Fk3RH5rVMIs7oFAl+Ov1kACgkQ3RH5rVMI
s7rhhxAAlW5XJY3OvjCbKb8uFpKp6CM1RqKslrUR2YT8PYSOZL1hyHltAJHV7fPr
z0MK018CmuSCpj3xzpkb8pXNjDibduXxkCxyaBn64KbxvRs5huxrI/nFyZaoGrtM
MPrIdmBAvezt2fXOUqLCPPfdUM7YKTnxRnwzivadsLtJ3lO7tuXSMphffnQtkOKj
W6shf+VzkzHwHO1g8xoJZ30whY9i+x674rMG8uDQrLbHP90bS3LfiE522UWfXh4O
8EgJrXUDZanFEkl9iLjhNtM1VuMYvWQXEoQUCCOeOCeWNxMLP2LCQ7XZtPvuOxYU
PFTmH4SE7hu9J//9KXLNddAVBcz1ywdJjC8Ju3W1TEuH4624LwWQ3VxHBhL0dKIh
u0Iyd/CWZeOtOnftMhxk35DmrS5SGwcF8W1w7RxuSdExRu22WguwbLcUzpMwAGvn
dZe7c57QEy6Qe/H5Ve0fOAxRxYYI25ND92IFAESdoDhDSsSSLd1oqgixE/F7DGyK
mTiiHhpqNjhA6v/LFAkAfqMTbtAcwm87aw2eNXzo9Fd7CwJMwwms/5Gn9a2VmGZ5
Qt9NTsx8MyK6lCY5Dof0dDnv3vYYjbw/i4tM6uXyMgcmsEDCwmGynfVfUyCvGeRG
zN80hrFzhCINpo+8gHksLc8wlN6RGBQD2BEbUt/NjqAFWeVRQBk=
=1XtD
-----END PGP SIGNATURE-----

@jonasnick jonasnick merged commit 6933b0e into fort-nix:master Oct 20, 2020
@erikarvstedt erikarvstedt deleted the auto-enable-deps branch December 15, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants