StevenBlack-hosts: init at 2.5.52 #80113
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/118 |
There was a problem hiding this comment.
Looks quite good, I've added a few formal nitpicks to make review easier for someone who can actually merge. Also:
- I think it's preferred to have package and module updates in separate commits
- there's bunch of whitespace issues, perhaps try using one of the nix formatting tools?
| owner = "StevenBlack"; | ||
| repo = "hosts"; | ||
| rev = version; | ||
| sha256 ="zQMzXci1/21PCvi8AGqyDataQl/kQJJ+jszxXd2XMQc="; |
There was a problem hiding this comment.
I'd suggest using the base32 hash format which is more prevalent in nixpkgs and used by tools such as nix-prefetch-url:
nix-hash --type sha256 --to-base32 "zQMzXci1/21PCvi8AGqyDataQl/kQJJ+jszxXd2XMQc="
Co-Authored-By: Martin Milata <martin@martinmilata.cz>
| }; | ||
|
|
||
| networking.extraHosts = | ||
| builtins.readFile (pkgs.StevenBlack-hosts.override{ |
There was a problem hiding this comment.
This is a bit ugly here, I think this is IFD (import-from-derivation), where a derivation needs to be built during evaluation time. I just created #81945 to get around this, after which you will be able to do
{
networking.hostFiles = [
(pkgs.StevenBlack-hosts.override { /* ... */ })
];
}There was a problem hiding this comment.
Just merged above PR, so this can be done now
| ''; | ||
|
|
||
| installPhase = '' | ||
| cp hosts $out |
There was a problem hiding this comment.
It might be a good idea to not put this in $out directly, but rather $out/share/StevenBlack-hosts/hosts. This then also allows people to install the package with environment.systemPackages or so, after which the hosts file will be in /run/current-system/sw/share/StevenBlack-hosts/hosts
|
I have incorporated @infinisil 's suggestions |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Motivation for this change
Adbloking based on @StevenBlack 's excellent hosts file
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)