Skip to content

lib.types.boolByOr: init#272764

Merged
roberth merged 1 commit intoNixOS:masterfrom
tweag:anyBool
Dec 10, 2023
Merged

lib.types.boolByOr: init#272764
roberth merged 1 commit intoNixOS:masterfrom
tweag:anyBool

Conversation

@infinisil
Copy link
Member

Description of changes

This type is necessary to have correct merging behavior for allowUnfreePredicate and allowInsecurePredicate, as discussed in #272686 (comment).

Things done

  • Added tests
  • Wrote docs

Add a 👍 reaction to pull requests you find important.

@infinisil infinisil requested a review from roberth December 7, 2023 21:11
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Dec 7, 2023
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I'm undecided
  • Functionality looks alright. One suggestion. Deep, despite the deeply unfunny comment.
  • I think you need to git add the test files. Happens to me all the time. Needs a git filter?

lib/types.nix Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should bool have type merging instead?
bias = true; => or
bias = false; => and

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bias = "quorum"; => chaos actually. Use mkMerge replicate to win.
🤡

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit worried about backwards compat if we had something like bool = boolWith { bias = null; }, since the name of the type would change (unless we did name = if bias == null then "bool" else "bool-with-${bias}"). Though I guess the name shouldn't really influence anything, probably..

Other than that, I'm not sure if we'll ever even need the AND-based variant. Even the OR-based one here is a bit dubious. It's probably only really necessary because of the functionTo bool of allowInsecurePredicate, which we should actually be replaced with RFC 127 instead anyways.

Actually considering that, it might make the most sense to declare this boolean type just locally in pkgs/top-level/config.nix. I don't know of any other use cases of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think combined process capability flags are a good candidate for the type. A service may have a set of security capabilities that should encompass all processes in that service.

Example:

  • system default policy for any service: cap.SYS_NET = mkDefault true;, cap.SYS_CHOWN = mkDefault false;
  • each programs sets its requests, both positive (need capability) as well as negative (do not need default capability):
    • program A: cap.SYS_NET = false;, cap.SYS_CHOWN = false;
    • program B: cap.SYS_NET = false;, cap.SYS_CHOWN = true;

SYS_NET shows the relevance of priorities, whereas SYS_CHOWN shows the need for boolByOr.

So I think it makes sense as a first class type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AND-based variant would be for things that are enabled by default but can be disabled by any other module.
I suspect that these are mostly things that are phrased negatively.

networking.firewall.enable would be an example of a secretly negative option, although definitely not one that any module should just go and disable. It's negative, because enabling it prevents functionality.
I think it may be acceptable for a module to set mkDefault false on a nice to have feature that isn't as critical.
Maybe in general you want cpufreq enabled, but some hardware service that suffers from it may disable cpufreq, but only as a default.
So I guess AND is for all flags that are nice to enable by default but might have negative consequences, so that some other modules can disable it by default. All while letting the user pick a setting without mkForce.

Currently we may (ab)use the mkOptionDefault/mkDefault separation to get similar behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm really not convinced. It seems undecidable whether something is "positive" or "negative". Even enabling the firewall newly allows ip commands to succeed when they didn't before, so you have new features in a way.

In practice, merging with mkDefault and co. priorities works well enough. One module setting true and the other false means the modules are incompatible with each other as is, so either don't use one of them, or make them have a mkDefault and co.

I don't think I've ever seen anybody run into an actual problem due to not having boolByOr/boolByAnd semantics. It really seems that it's only necessary for allowUnfreePredicate, due to how it would be impractical to write

allowUnfreePredicate = pkg:
  if
    builtins.elem (lib.getName pkg) [
      "foo"
    ]
  then
    true
  else
    mkDefault false;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm convinced of boolByOr, just not of boolByAnd, because its uses cases are more rare than boolByOr, and more contrived because of the negatives. I'm also not convinced of needing to type merged a bias onto an existing bool, so we can skip that idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright let's have just boolByOr for now then, sounds fine to me

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 8, 2023
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also s/anyBool/boolByOr/, and then I think this is good to merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
All definitions must have the same value, an error is thrown in case of a conflict.
All definitions must have the same value, after priorities. An error is thrown in case of a conflict.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied!

This type is necessary to have correct merging behavior for
`allowUnfreePredicate` and `allowInsecurePredicate`

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@infinisil
Copy link
Member Author

Good to go from my side!

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 8, 2023
@roberth roberth merged commit 584463c into NixOS:master Dec 10, 2023
@github-actions
Copy link
Contributor

Successfully created backport PR for release-23.11:

@infinisil infinisil deleted the anyBool branch December 10, 2023 22:35
@infinisil infinisil changed the title lib.types.anyBool: init lib.types.boolByOr: init Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants