stevenblack-blocklist: add ipv6 support

[Pandapip1]

See StevenBlack/hosts#47

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[Pandapip1]

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374656

---

x86_64-linux

✅ 6 packages built:

• stevenblack-blocklist
• stevenblack-blocklist.ads
• stevenblack-blocklist.fakenews
• stevenblack-blocklist.gambling
• stevenblack-blocklist.porn
• stevenblack-blocklist.social

[Frontear]

Just took a look at the attached GitHub issue as well as your PR changes for the upstream provided flake. I have some thoughts but I'll comment on them as soon as I am available, particularly in terms of the accompanying NixOS module in relation to this change.

[Pandapip1]

I think I know where you're going with this. Would you like me to add a toggleable flag?

EDIT: I'll just add one anyway.

[Pandapip1]

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374656

---

x86_64-linux

⏩ 1 package blacklisted:

• nixos-install-tools

✅ 13 packages built:

• nixpkgs-manual
• stevenblack-blocklist
• stevenblack-blocklist-ipv6
• stevenblack-blocklist-ipv6.ads
• stevenblack-blocklist-ipv6.fakenews
• stevenblack-blocklist-ipv6.gambling
• stevenblack-blocklist-ipv6.porn
• stevenblack-blocklist-ipv6.social
• stevenblack-blocklist.ads
• stevenblack-blocklist.fakenews
• stevenblack-blocklist.gambling
• stevenblack-blocklist.porn
• stevenblack-blocklist.social

[Frontear]

1. I think we should move dontAddIPv6 into the function arguments instead of as an attribute. That way, we can use .override instead of .overrideAttrs, since this is usually the common pattern I see in Nixpkgs.
2. The PR is trying to do too much now with the lib/options.nix and lib/tests/modules/declare-mkPackageOption.nix changes. These are functions used heavily in a lot of NixOS modules, and I don't think we should make a change to them in this random PR.
3. Good use of the config.networking.enableIPv6, I was actually debating on whether it'd be a good idea to use or not, especially since Nixpkgs defaults with it on but quite a lot of people still do not have IPv6 capable hardware available.

Currently I'm debating on the creation of a new package stevenblack-blocklist-ipv6, and the extra outputs. Both technically fit within the spirit of what I had initially done when refactoring this module, but I'm beginning to wonder if it makes it look cluttered and messy.

To clarify on the above, I don't intend to say these are necessarily bad, after all I do actually like that we leverage Hydra and the cache to store more and thereby download less on our individual machines, but I do wonder if it fits well with what other Nixpkgs maintainers/commiters would consider as good quality code. I'm hoping another reviewer can chime in on some of these things so that there's more assurance.

From my end though, once you address the numbered points above, I do feel like it's fine, though I will admit it still nags me a bit. Still, a small feeling isn't enough to justify stopping a good PR.

[Pandapip1]

Good use of the config.networking.enableIPv6, I was actually debating on whether it'd be a good idea to use or not, especially since Nixpkgs defaults with it on but quite a lot of people still do not have IPv6 capable hardware available.

Most people do have IPv6-capable hardware, and it's been that way for a while. It's their ISP that's holding them back, and even then they could host their own NAT64.

Currently I'm debating on the creation of a new package stevenblack-blocklist-ipv6, and the extra outputs. Both technically fit within the spirit of what I had initially done when refactoring this module, but I'm beginning to wonder if it makes it look cluttered and messy.

To clarify on the above, I don't intend to say these are necessarily bad, after all I do actually like that we leverage Hydra and the cache to store more and thereby download less on our individual machines, but I do wonder if it fits well with what other Nixpkgs maintainers/commiters would consider as good quality code. I'm hoping another reviewer can chime in on some of these things so that there's more assurance.

This is actually a semi-common pattern; it's just that usually these modifications are defined in all-packages.nix rather than by-name.

The PR is trying to do too much now with the lib/options.nix and lib/tests/modules/declare-mkPackageOption.nix changes. These are functions used heavily in a lot of NixOS modules, and I don't think we should make a change to them in this random PR.

I feel the same way, but I don't see an easy way around this without breaking existing configs. I'll make a kludge but it won't be pretty. I'll also cherry-pick it into its own PR.

[Frontear]

When I review code, I tend to try and think about how I would write a certain piece of code, and if what I am writing and seeing aligns well with other code I have viewed in said repository. Apologies in advance for my nitpicking but I'm still a bit on the fence about some of the things here.

I'm not really sure that we need a separate package entirely for the IPv6 hostfiles. This could be something that's part of the original package as a "feature-flag" opt-in, and could be overriden within the module upon request. Something akin to what I'll minimally type out below is what I would consider closer to what I see in Nixpkgs already, and a better choice rather than exposing a whole new package.

# pkgs/by-name/st/stevenblack/package.nix
{
  lib,
  stdenvNoCC,
  fetchFromGitHub,
  nix-update-script,

  enableIPv6 ? true, # or "withIPv6 ? true"
}:
...

# nixos/modules/config/stevenblack.nix
{
  config,
  lib,
  pkgs,
  ...
}:
{
  # [...omitted...]

  config = mkIf cfg.enable {
    networking.stevenblack.package = lib.mkIf cfg.enableIPv6 cfg.package.override { enableIPv6 = true; };

    # [...omitted...]
  };
}

An additional set of benefits to doing it this way would be that we can still support the cfg.package changing. With your current PR, if I wanted to "pin" the package via setting networking.stevenblack.package = someOlderVersionOfTheDrv;, I would subsequently be unable to make use of the networking.stevenblack.enableIPv6 flag, since your current code prioritizes the package and will not attempt to override it. The alternative way I have suggested would still allow for this behaviour.

On a different note, I'm also thinking that maybe we can enable IPv6 by default, just to match networking.enableIPv6 also being default enabled. As adoption for IPv6 grows in the future, most people will probably expect it to just work without actively scouring the NixOS modules to enable every little flag. This would be a perfect choice for future compatibility, whilst still allowing the user to toggle it off should they want to save on closure size.

I'm not sure if these changes constitute adding a release-note, but if it's acceptable I think that should be added as well, just to note on the fact that enableIPv6 is an option now and that it's enabled by default, so that users are aware of such a change.

[Pandapip1]

An additional set of benefits to doing it this way would be that we can still support the cfg.package changing. With your current PR, if I wanted to "pin" the package via setting networking.stevenblack.package = someOlderVersionOfTheDrv;, I would subsequently be unable to make use of the networking.stevenblack.enableIPv6 flag, since your current code prioritizes the package and will not attempt to override it. The alternative way I have suggested would still allow for this behaviour.

There is no networking.stevenblack.enableIPv6 flag. You're thinking of my upstream PR. Since the mechanism for adding the hosts there is different, it makes sense to do it that way there.

This way, if you want to pin an older drv, you pin the ipv6 version of it. I'm hesitant to make the ipv6 version the default; it's a pretty major change. I also really dislike the .override pattern; I believe it should be reserved only for overriding dependencies, behavioral changes (like the builtin mkDerivation ones!) belong in .overrideAttrs. This is how I originally had it, but I got feedback that it didn't look particularly good and was less maintainable. Ergo, the second package.

On a different note, I'm also thinking that maybe we can enable IPv6 by default, just to match networking.enableIPv6 also being default enabled. As adoption for IPv6 grows in the future, most people will probably expect it to just work without actively scouring the NixOS modules to enable every little flag. This would be a perfect choice for future compatibility, whilst still allowing the user to toggle it off should they want to save on closure size.

Again, there is no networking.stevenblack.enableIPv6 flag. Right now, as this PR currently stands, if this were merged, the IPv6 rules would take effect for all IPv6-enabled systems unless they overrode the package to the non-ipv6 version. I'm hesitant to add an option that modifies the derivation, as I know from experience that this tends to create hidden build failures and other nasty non-easily reproducible stuff. Also, what if it was already pinned to an older package that didn't have that option? That would cause an eval error, making this a breaking change. I'd like to avoid that if possible.

I'm not sure if these changes constitute adding a release-note, but if it's acceptable I think that should be added as well, just to note on the fact that enableIPv6 is an option now and that it's enabled by default, so that users are aware of such a change.

Again, there is no networking.stevenblack.enableIPv6 flag. I think that mentioning that networking.stevenblack supports IPv6 now is a good idea, however.

[Frontear]

I thought I'd already mentioned that adding networking.stevenblack.enableIPv6 would be good, hence why I was commenting on it as if it had already been added. Nonetheless, I also agree that it'd be a good idea to add. Whether it should be enabled by default or not is definitely subject to more opinions. Considering networking.enableIPv6 already defaults to true, we'd already be loading the IPv6 patched files for almost all NixOS users who use this module as the PR stands, so if you're against the idea of globally assuming IPv6 to be true, then I suggest adding the additional module option as above and keeping it as false by default.

I also really dislike the .override pattern; I believe it should be reserved only for overriding dependencies, behavioral changes (like the builtin mkDerivation ones!) belong in .overrideAttrs

Fair enough, I only say that because that's how I tend to see feature-flag behaviour in Nix derivations. There are numerous examples of this way which is why I preferred going about it that way as well.

as I know from experience that this tends to create hidden build failures and other nasty non-easily reproducible stuff.

Fair point, most people don't report their configs when reporting build failures, which does make things harder to reproduce.

I suppose with all that being said the second package can stay, it seems to be the cleaner way to go about things at this point. My only concern is the fact that now we expose a .package option that is a) mutated internally based on networking.enableIPv6, which then b) conflicts with re-defining the .package value. This also results in the same concern over reproducibility, given that in some cases the package used is modified according to IPv6 and not in others. To me, this means that we should either forgo allowing .package to be mutated, or we should use the module system to assign the package if enableIPv6 is true, which would then force an evaluation warning for conflicting priorities, forcing the user to choose one or the other.

Either way, we would need a minor addition to the release notes regarding this module. I don't see a way to keep the NixOS module exactly as-is, whilst also adding IPv6 support for it (unless we simply force users to use .package = pkgs.stevenblack-blocklist-ipv6 rather than .enableIPv6 in the module system).

[Pandapip1]

Fair enough, I only say that because that's how I tend to see feature-flag behaviour in Nix derivations. There are numerous examples of this way which is why I preferred going about it that way as well.

Yea, I see this too. It kind of grates me.

... To me, this means that we should either forgo allowing .package to be mutated, or we should use the module system to assign the package if enableIPv6 is true, which would then force an evaluation warning for conflicting priorities, forcing the user to choose one or the other.

I wonder if the best way to go about this might be to have a networking option that does the hosts file rewrite (we can even include automatic NAT64 translation!), which also avoids the need for separate packages or modifications to stevenblack-blocklist at all. I propose tentatively networking.rewriteHostsForIPv6, or moving all the hosts options to a submodule networking.hostsFile because at this point it probably should be. How does that sound?

[Frontear]

Fair enough, I only say that because that's how I tend to see feature-flag behaviour in Nix derivations. There are numerous examples of this way which is why I preferred going about it that way as well.

Yea, I see this too. It kind of grates me.

... To me, this means that we should either forgo allowing .package to be mutated, or we should use the module system to assign the package if enableIPv6 is true, which would then force an evaluation warning for conflicting priorities, forcing the user to choose one or the other.

I wonder if the best way to go about this might be to have a networking option that does the hosts file rewrite (we can even include automatic NAT64 translation!), which also avoids the need for separate packages or modifications to stevenblack-blocklist at all. I propose tentatively networking.rewriteHostsForIPv6, or moving all the hosts options to a submodule networking.hostsFile because at this point it probably should be. How does that sound?

Hmm, the main problem I can see is in regards to closure size. Every hosts file would increase by quite a lot, and there wouldn't be a good way for such an option to "check" if a hosts file supports IPv6 entries or not. It's not a bad idea overall, but it would definitely need some things ironed out.

[Pandapip1]

Most people don't have particularly large host files. IIRC, it typically contains just the loopback address. It's really just for hostfile based blocking or caching that these lists can get quite large.