-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
lighthouse: add package
option to service
#285005
Conversation
20f14a1
to
6b3b48c
Compare
@ritave Do you mind having a look into this? EDIT: @centromere @philipmw Since you are the lighthouse package maintainers, maybe you could look into this, too? |
@rolfschr LGTM. |
Can we merge this? |
@@ -207,12 +207,19 @@ in { | |||
default = ""; | |||
example = ""; | |||
}; | |||
|
|||
package = mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mkPackageOption will be nicer here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using lib.getExe
is greatly preferred as it makes any possible changes forwards-compatible, otherwise great job, hopefully we can get this merged asap.
@@ -229,7 +236,7 @@ in { | |||
# make sure the chain data directory is created on first run | |||
mkdir -p ${cfg.beacon.dataDir}/${cfg.network} | |||
${pkgs.lighthouse}/bin/lighthouse beacon_node \ | |||
${cfg.package}/bin/lighthouse beacon_node \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${cfg.package}/bin/lighthouse beacon_node \ | |
${lib.getExe cfg.package} beacon_node \ |
@@ -277,7 +284,7 @@ in { | |||
# make sure the chain data directory is created on first run | |||
mkdir -p ${cfg.validator.dataDir}/${cfg.network} | |||
${pkgs.lighthouse}/bin/lighthouse validator_client \ | |||
${cfg.package}/bin/lighthouse validator_client \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${cfg.package}/bin/lighthouse validator_client \ | |
${lib.getExe cfg.package} validator_client \ |
Okay, thanks for the additinal input. I updated the branch accordingly and aslo applied |
Ah, I realize something is wrong (it doesn't build). I had not run anything locally. Will look into this 🤷 . |
463874a
to
6b5d6b7
Compare
Fixed my syntax error diff --git a/nixos/modules/services/blockchain/ethereum/lighthouse.nix b/nixos/modules/services/blockchain/ethereum/lighthouse.nix
index 2c5e3d2bff7f..247174028adc 100644
--- a/nixos/modules/services/blockchain/ethereum/lighthouse.nix
+++ b/nixos/modules/services/blockchain/ethereum/lighthouse.nix
@@ -216,7 +216,7 @@ in
example = "";
};
- package = lib.mkPackageOption pkgs "lighthouse";
+ package = lib.mkPackageOption pkgs "lighthouse" { };
};
};
|
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.