nixos/shadowsocks: add package option
#407592
Conversation
| }; | ||
| }; | ||
|
|
||
| getServiceUnitName = pkg: "shadowsocks-${pkgTitleMap.${getName cfg.package}}"; |
There was a problem hiding this comment.
Looks like all the transformations on getName reduce to just the identity?
| getServiceUnitName = pkg: "shadowsocks-${pkgTitleMap.${getName cfg.package}}"; | |
| getServiceUnitName = getName cfg.package; |
| }; | ||
|
|
||
| getServiceUnitName = pkg: "shadowsocks-${pkgTitleMap.${getName cfg.package}}"; | ||
| getExecCommand = pkg: runMode: commandsMap.${getName cfg.package}.${runMode}; |
There was a problem hiding this comment.
I think there is also getExecutable, no?
There was a problem hiding this comment.
I found lib.getExe, but shadowsocks-libev and shadowsocks-rust provides multiple binaries (without mainProgram). Also i have plans to add the ability to use other binaries in the future (e.g. ss-local)
There was a problem hiding this comment.
There is lib.getExe' for this use case, which allows you to specify the binary name.
There was a problem hiding this comment.
Thank you for the observation. But in any case a conversion table will be needed, because different packages have different binary names (libev -> ss-server, rust -> ssserver). Anyway module relies on knowledge of binaries name, so i doesn't see point in using lib.getExe'. If you think different, i would be happy for your feedback
| @@ -48,14 +64,27 @@ in | |||
| ''; | |||
| }; | |||
|
|
|||
| package = mkPackageOption pkgs "Shadowsocks" { | |||
| default = "shadowsocks-libev"; | |||
| example = "services.shadowsocks.package = pkgs.shadowsocks-libev"; | |||
There was a problem hiding this comment.
I think example should be only the right-hand side.
| example = "services.shadowsocks.package = pkgs.shadowsocks-libev"; | |
| example = "pkgs.shadowsocks-libev"; |
That said, I don't think an example is really necessary here.
| package = mkPackageOption pkgs "Shadowsocks" { | ||
| default = "shadowsocks-libev"; | ||
| example = "services.shadowsocks.package = pkgs.shadowsocks-libev"; | ||
| extraDescription = "Shadowsocks implementation package to use."; |
There was a problem hiding this comment.
Does mkPackageOption not accept description?
| extraDescription = "Shadowsocks implementation package to use."; | |
| description = "Shadowsocks implementation package to use."; |
There was a problem hiding this comment.
There is no description argument. As i found it's generated automatically, so extraDescription makes no sense. I'll remove it
74bc19d to
950abff
Compare
| @@ -164,22 +182,29 @@ in | |||
| (noPasswd && !noPasswdFile) || (!noPasswd && noPasswdFile); | |||
| message = "Option `password` or `passwordFile` must be set and cannot be set simultaneously"; | |||
| } | |||
| { | |||
| # Ensure localAddress is a string if package is shadowsocks-rust | |||
| assertion = !(cfg.package == pkgs.shadowsocks-rust && !lib.strings.isString cfg.localAddress); | |||
There was a problem hiding this comment.
Should probably check pname here. People like to override the package with package option.
There was a problem hiding this comment.
Tanks for highlight this corner case
950abff to
9457218
Compare
|
@ofborg test shadowsocks.basic-libev shadowsocks.basic-rust shadowsocks.v2ray-plugin-libev shadowsocks.v2ray-plugin-rust |
9457218 to
8bdd8cb
Compare
Closes: NixOS#384226 - add `package` option to services.shadowsocks and related changes - add tests for `pkgs.shadowsocks-rust` - add postfix 'libev' to original tests
8bdd8cb to
ea14385
Compare
|
Apologies for the ping - just following up as this has been open for three months. I've rebased onto master today to fix conflicts. ofborg tests were triggered but stuck in queue for ~two months. I'm still eager to contribute this - happy to make any tweaks for merge. Thanks! |
Related issues:
Changes:
packageoption to services.shadowsocks and related changespkgs.shadowsocks-rustThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.