Skip to content

lib.types.boolByOr: init #272764

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.

Sign up for GitHub

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

Merged
roberth merged 1 commit into from
Dec 10, 2023
Merged

lib.types.boolByOr: init #272764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/tests/modules.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ checkConfigError 'The option .* does not exist. Definition values:\n\s*- In .*'
checkConfigError 'while evaluating a definition from `.*/define-enable-abort.nix' config.enable ./define-enable-abort.nix
checkConfigError 'while evaluating the error message for definitions for .enable., which is an option that does not exist' config.enable ./define-enable-abort.nix

# Check boolByOr type.
checkConfigOutput '^false$' config.value.falseFalse ./boolByOr.nix
checkConfigOutput '^true$' config.value.trueFalse ./boolByOr.nix
checkConfigOutput '^true$' config.value.falseTrue ./boolByOr.nix
checkConfigOutput '^true$' config.value.trueTrue ./boolByOr.nix

checkConfigOutput '^1$' config.bare-submodule.nested ./declare-bare-submodule.nix ./declare-bare-submodule-nested-option.nix
checkConfigOutput '^2$' config.bare-submodule.deep ./declare-bare-submodule.nix ./declare-bare-submodule-deep-option.nix
checkConfigOutput '^42$' config.bare-submodule.nested ./declare-bare-submodule.nix ./declare-bare-submodule-nested-option.nix ./declare-bare-submodule-deep-option.nix ./define-bare-submodule-values.nix
Expand Down
14 changes: 14 additions & 0 deletions lib/tests/modules/boolByOr.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{ lib, ... }: {

options.value = lib.mkOption {
type = lib.types.lazyAttrsOf lib.types.boolByOr;
};

config.value = {
falseFalse = lib.mkMerge [ false false ];
trueFalse = lib.mkMerge [ true false ];
falseTrue = lib.mkMerge [ false true ];
trueTrue = lib.mkMerge [ true true ];
};
}

16 changes: 16 additions & 0 deletions lib/types.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,22 @@ rec {
merge = mergeEqualOption;
};

boolByOr = mkOptionType {
name = "boolByOr";
description = "boolean (merged using or)";
descriptionClass = "noun";
check = isBool;
merge = loc: defs:
foldl'
(result: def:
# Under the assumption that .check always runs before merge, we can assume that all defs.*.value
# have been forced, and therefore we assume we don't introduce order-dependent strictness here
result || def.value
)
false
defs;
};
Copy link
Member

@roberth roberth Dec 7, 2023

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

@roberth roberth Dec 7, 2023

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

@infinisil infinisil Dec 7, 2023

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

@roberth roberth Dec 8, 2023

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

@roberth roberth Dec 8, 2023

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

@infinisil infinisil Dec 8, 2023

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

@roberth roberth Dec 8, 2023

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

@infinisil infinisil Dec 8, 2023

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


int = mkOptionType {
name = "int";
description = "signed integer";
Expand Down
7 changes: 7 additions & 0 deletions nixos/doc/manual/development/option-types.section.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ merging is handled.
`types.bool`

: A boolean, its values can be `true` or `false`.
All definitions must have the same value, after priorities. An error is thrown in case of a conflict.

`types.boolByOr`

: A boolean, its values can be `true` or `false`.
The result is `true` if _any_ of multiple definitions is `true`.
In other words, definitions are merged with the logical _OR_ operator.

`types.path`

Expand Down