-
-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
dnscrypt-proxy2: 2.0.15 -> 2.0.17, add NixOS module #48289
Conversation
json = pkgs.writeText "dnscrypt-proxy.json" (builtins.toJSON cfg.config); | ||
|
||
toml = pkgs.runCommand "dnscrypt-proxy.toml" {} '' | ||
${pkgs.json-to-toml}/bin/json-to-toml < ${json} > $out |
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.
I think it make sense to just re-use remarshal
instead. It does the same thing and if we agree on one tool, it can be re-used for different services. See also in the telegraf, influxdb and gitlab-runner module.
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.
I know about remarshal
, but concerned about build time and closure size. One of my targets for this service is a 1GHz single-core armv7l
.
If I add more conversion types to that tool, will it perhaps be more appropriate?
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.
I guess so. Both implementations are not terrible complex.
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.
Btw. I can recommend nixops for you use case independent from your pr. Our nix builder also handles armv7l for that matter.
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.
It would be nice if the output is pretty-printed with indentation to make it more readable.
Success on aarch64-linux (full log) Attempted: dnscrypt-proxy2, json-to-toml Partial log (click to expand)
|
Success on x86_64-darwin (full log) Attempted: dnscrypt-proxy2, json-to-toml Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: dnscrypt-proxy2, json-to-toml Partial log (click to expand)
|
{ config, lib, pkgs, ... }: with lib; | ||
|
||
let | ||
cfg = config.services.dnscrypt-proxy2; |
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.
I think we should deprecate dnscrypt-proxy v1 for the next release. It is no longer maintained afaik.
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.
Agreed. v1 should be removed for 19.03
LGTM |
Success on x86_64-darwin (full log) Attempted: dnscrypt-proxy2 Partial log (click to expand)
|
Success on aarch64-linux (full log) Attempted: dnscrypt-proxy2 Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: dnscrypt-proxy2 Partial log (click to expand)
|
repo = "dnscrypt-proxy"; | ||
rev = "${version}"; | ||
sha256 = "0iwvndk1h550zmwhwablb0smv9n2l51hqbmzj354mcgi6frnx819"; | ||
src = fetchzip { |
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.
How is this different from fetchFromGitHub
btw?
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.
Not much, wanted to reuse goPackagePath
.
''; | ||
example = literalExample '' | ||
{ | ||
sources.public-resolvers = { |
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.
This could become the default value, no?
I currently see the the following errors on startup:
Can dnscrypt-proxy be configured to store its data in /var/lib/dnscrypt for example? |
@Mic92 absolutely. Currently:
So should just need to set {
CacheDirectory = "dnscrypt";
WorkingDirectory = "/var/cache/dnscrypt";
} With Otherwise, looks OK. Personally I was taking the route of more manually constructing the configuration file from well-typed options, as I like the benefits that offers:
But it's also a lot more work to implement, and presents some cognitive overhead to people already familiar with configuring the service directly. I will most likely get around to finishing a version of the module done that way, but at the current rate it won't be for a couple of months (too many irons in the fire -- and too many fires, for that matter). I'm definitely happy to have something working in nixpkgs before then 👍. |
type = types.attrs; | ||
}; | ||
|
||
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.
What is the rationale for making this configurable?
I'd be happy to merge this as if, if not for the package option. I really dislike those ... Anybody have a usecase for it in this case? |
On the other hand, I think that all service modules should have Primary use case is freezing specific version in your overlay with a different attr name. Without package option, you're stuck with shadowing the original package, which might cause unnecessary rebuilds if something else depends on that same package. Secondary use case is that it allows to maintain multiple versions/flavors of the package in Nixpkgs while having a single NixOS module. Assumption that it is not the case doesn't hold over time. |
My view is that options should be judged on their merit on a module-to-module basis. I fail to see how the general rationale for package options pertains to dnscrypt-proxy. |
I don't understand why it has to be judged on module-to-module basis. It has a few general benefits that I've listed above. Deciding on case-by-case basis will inevitably cause friction downstream, because we won't be able to predict all use cases, and the option itself is trivial and adds three lines of Nix code. |
Don't suppose updates such as in recent PR's (see above) resolve the outstanding issues? If any? |
Can this make it into 19.03 or is it too late already? |
- Remove package option, which seems to have been the only blocker to merging NixOS#48289. - Add configFile option as an alternative to translating toml/nix.
Any updates? |
There's new PRs inheriting this work. |
Motivation for this change
Resolves #43298.
cc @joachifm @Shados
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)