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

StevenBlack-hosts: init at 2.5.52 #80113

Open
wants to merge 6 commits into
base: master
from
Open

StevenBlack-hosts: init at 2.5.52 #80113

wants to merge 6 commits into from

Conversation

@pasqui23
Copy link
Contributor

pasqui23 commented Feb 14, 2020

Motivation for this change

Adbloking based on @StevenBlack 's excellent hosts file

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Feb 23, 2020

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

Copy link
Contributor

mmilata left a 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=";

This comment has been minimized.

Copy link
@mmilata

mmilata Feb 25, 2020

Contributor

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="
homepage = "https://github.com/StevenBlack/hosts";
description = "Extending and consolidating hosts files from several well-curated sources";
license = licenses.unfreeRedistributable;
platforms = platforms.all;

This comment has been minimized.

Copy link
@mmilata

mmilata Feb 25, 2020

Contributor

maintainer field is missing. Probably a good time to add yourself to maintainers-list.nix:) Preferably in separate commit.

Co-Authored-By: Martin Milata <martin@martinmilata.cz>
@Infinisil Infinisil mentioned this pull request Mar 7, 2020
2 of 2 tasks complete
};

networking.extraHosts =
builtins.readFile (pkgs.StevenBlack-hosts.override{

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 7, 2020

Member

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 { /* ... */ })
  ];
}

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 12, 2020

Member

Just merged above PR, so this can be done now

'';

installPhase = ''
cp hosts $out

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 7, 2020

Member

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

@Infinisil Infinisil mentioned this pull request Mar 12, 2020
2 of 9 tasks complete
@pasqui23

This comment has been minimized.

Copy link
Contributor Author

pasqui23 commented Mar 13, 2020

I have incorporated @Infinisil 's suggestions

Copy link
Member

Infinisil left a comment

Also see @mmilata's review here: #80113 (review)

whitelist = mkOption {
description = "List of hosts to allow";
default = "";
type = types.lines;

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 15, 2020

Member

What's the format of this option? This should be documented in the description. Also could there be a more appropriate type for this than just lines?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.