]> git.ipfire.org Git - thirdparty/mkosi.git/commitdiff
Rework configuration parsing ordering and overrides 1424/head
authorDaan De Meyer <daan.j.demeyer@gmail.com>
Fri, 7 Apr 2023 10:32:58 +0000 (12:32 +0200)
committerDaan De Meyer <daan.j.demeyer@gmail.com>
Fri, 7 Apr 2023 11:25:40 +0000 (13:25 +0200)
Currently, aside from list based settings, if a setting is used
multiple times across different configuration files, the later
assignments override earlier assignments. On top of that, the CLI
arguments are parsed last and override everything from config files.

This has worked very well until now, but breaks now that we have
[Match] sections. If we override a setting using the CLI, we want
any configured [Match] sections to compare against the value specified
via the CLI. Because the CLI values are applied last, this currently
isn't the case.

Similarly, if we add 90-local.conf to override the distribution release
used for Debian, all the config files that are processed earlier will
still compare against the default release configured for Debian in
50-debian.conf. If we rename 90-local.conf to 00-local.conf, the default
release in 50-debian.conf will still override that value. Only by putting
the override after the config file that assigns the default release but
before the first config file that matches on that release can we make this
work, but depending on the configuration this might require a lot of
different override files which isn't ideal.

Also, because later values can override earlier ones, it's possible to have
[Match] sections that match against different values of the same setting
both apply, if the setting happens to be reassigned inbetween, which is not
intuitive and error prone. Ideally, [Match] settings only apply to the final
value of a setting.

To fix these problems, this commit reworks the way we order and override
settings. Instead of having later assignments override earlier ones, for
all settings aside from list settings, the first assignment is the one that
is used. All later assignments of the same setting are ignored. Along with, we
switch to parsing CLI arguments first, which means that any settings assigned
in CLI args take precedence over those configured in config files.

Because the first value is immediately the final value, any [Match] sections
will only ever see the final value. One caveat is default values for settings.
We only want to assign these at the end of parsing, if no value has been
explicitly configured, but we also want [Match] sections to apply to default
values if no value is explicitly configured. The solution to this is that if
we encounter a setting in a [Match] section and it has not been explicitly
assigned a value yet, it is assigned its default value.

For list based settings, ! now configures an ignore glob, which means that if any
later assignments try to assign values that match an ignore glob, those values
are ignored. We also prepend list values instead of appending so that list
values that are configured in a preceeding config file appear later in the final
list value than values configured in a later assignment.

Implementation wise, this commit reworks config parser functions to return the
new value that should be assigned instead of assigning it themselves. This makes
the config parsing functions slightly more generic.

mkosi.md
mkosi/__init__.py
mkosi/__main__.py
mkosi/config.py
tests/test_parse_load_args.py

index 9d830e123bffb69d3adffbba483a09cd5c5fbc21..6d9309566adeb9de3e55157bd86237d3e09239bd 100644 (file)
--- a/mkosi.md
+++ b/mkosi.md
@@ -150,6 +150,7 @@ in consecutive runs with data from the cached one.
 * Run SELinux relabel is a SELinux policy is installed
 * Generate unified kernel image
 * Generate final output format
+
 ## Supported output formats
 
 The following output formats are supported:
@@ -176,11 +177,55 @@ single-letter shortcut is also allowed. In the configuration files,
 the setting must be in the appropriate section, so the settings are
 grouped by section below.
 
+Configuration is parsed in the following order:
+
+* The command line arguments are parsed
+* `mkosi.conf` is parsed if it exists in the directory set with
+  `--directory=` or the current working directory if `--directory=` is
+  not used.
+* `mkosi.conf.d/` is parsed in the same directory if it exists. Each
+  directory and each file with the `.conf` extension in `mkosi.conf.d/`
+  is parsed. Any directory in `mkosi.conf.d` is parsed as if it were
+  a regular top level directory.
+* Any default paths (depending on the option) are configured if the
+  corresponding path exists.
+
+If a setting is specified multiple times across the different sources
+of configuration, the first assignment that is found is used. For example,
+a setting specified on the command line will always take precedence over
+the same setting configured in a config file. To override settings in a
+dropin file, make sure your dropin file is alphanumerically ordered
+before the config file that you're trying to override.
+
+Settings that take a list of values are merged by prepending each value
+to the previously configured values. If a value of a list setting is
+prefixed with `!`, if any later assignment of that setting tries to add
+the same value, that value is ignored. Values prefixed with `!` can be
+globs to ignore more than one value.
+
+To conditionally include configuration files, the `[Match]` section can
+be used. A configuration file is only included if all the conditions in the
+`[Match]` block are satisfied. If a condition in `[Match]` depends on a
+setting and the setting has not been explicitly configured when the condition
+is evaluated, the setting is assigned its default value.
+
 Command line options that take no argument are shown without "=" in
 their long version. In the config files, they should be specified with
 a boolean argument: either "1", "yes", or "true" to enable, or "0",
 "no", "false" to disable.
 
+### [Match] Section.
+
+`Distribution=`
+
+: Matches against the configured distribution.
+
+`Release=`
+
+: Matches against the configured distribution release. If this condition
+  is used and no distribution has been explicitly configured yet, the
+  host distribution and release are used.
+
 ### [Distribution] Section
 
 `Distribution=`, `--distribution=`, `-d`
index 6f070b9193e632bc16698208d79d6c1ba887cdfc..1cd3db431f9816bd633e857737f5bf15ac7e7478 100644 (file)
@@ -45,6 +45,7 @@ from mkosi.backend import (
 from mkosi.config import (
     MkosiConfigParser,
     MkosiConfigSetting,
+    config_default_release,
     config_make_action,
     config_make_enum_matcher,
     config_make_enum_parser,
@@ -54,7 +55,6 @@ from mkosi.config import (
     config_parse_base_packages,
     config_parse_boolean,
     config_parse_compression,
-    config_parse_distribution,
     config_parse_feature,
     config_parse_script,
     config_parse_string,
@@ -1079,7 +1079,7 @@ SETTINGS = (
     MkosiConfigSetting(
         dest="distribution",
         section="Distribution",
-        parse=config_parse_distribution,
+        parse=config_make_enum_parser(Distribution),
         match=config_make_enum_matcher(Distribution),
         default=detect_distribution()[0],
     ),
@@ -1088,7 +1088,7 @@ SETTINGS = (
         section="Distribution",
         parse=config_parse_string,
         match=config_match_string,
-        default=detect_distribution()[1],
+        default_factory=config_default_release,
     ),
     MkosiConfigSetting(
         dest="architecture",
@@ -1483,6 +1483,7 @@ def create_argument_parser() -> argparse.ArgumentParser:
         usage=USAGE,
         add_help=False,
         allow_abbrev=False,
+        argument_default=argparse.SUPPRESS,
     )
 
     parser.add_argument(
@@ -1998,18 +1999,11 @@ def create_argument_parser() -> argparse.ArgumentParser:
     return parser
 
 
-def parse_args(
-    argv: Optional[Sequence[str]] = None,
-    directory: Optional[Path] = None,
-    namespace: Optional[argparse.Namespace] = None
-) -> argparse.Namespace:
+def parse_args(argv: Optional[Sequence[str]] = None) -> argparse.Namespace:
     if argv is None:
         argv = sys.argv[1:]
     argv = list(argv)  # make a copy 'cause we'll be modifying the list later on
 
-    if namespace is None:
-        namespace = argparse.Namespace()
-
     # Make sure the verb command gets explicitly passed. Insert a -- before the positional verb argument
     # otherwise it might be considered as an argument of a parameter with nargs='?'. For example mkosi -i
     # summary would be treated as -i=summary.
@@ -2025,24 +2019,33 @@ def parse_args(
     else:
         argv += ["--", "build"]
 
+    argparser = create_argument_parser()
+    namespace = argparser.parse_args(argv)
+
+    if namespace.verb == Verb.help:
+        argparser.print_help()
+        argparser.exit()
+
+    if "directory" not in namespace:
+        setattr(namespace, "directory", None)
+
+    if namespace.directory and not namespace.directory.is_dir():
+        die(f"Error: {namespace.directory} is not a directory!")
+
+    namespace = MkosiConfigParser(SETTINGS).parse(namespace.directory or Path("."), namespace)
+
     for s in SETTINGS:
         if s.dest in namespace:
             continue
 
-        if s.default is None:
-            s.parse(s.dest, None, namespace)
+        if s.default_factory:
+            default = s.default_factory(namespace)
+        elif s.default is None:
+            default = s.parse(s.dest, None, namespace)
         else:
-            setattr(namespace, s.dest, s.default)
-
-    if directory:
-        namespace = MkosiConfigParser(SETTINGS, directory).parse(namespace)
-
-    argparser = create_argument_parser()
-    namespace = argparser.parse_args(argv, namespace)
+            default = s.default
 
-    if namespace.verb == Verb.help:
-        argparser.print_help()
-        argparser.exit()
+        setattr(namespace, s.dest, default)
 
     return namespace
 
index d6f390b4758ac3d3a3262bd73adddc202840830a..af8a69381e1c87399ca09bfbbdbaa07d0922f5a7 100644 (file)
@@ -5,7 +5,6 @@ import contextlib
 import os
 import sys
 from collections.abc import Iterator
-from pathlib import Path
 from subprocess import CalledProcessError
 
 from mkosi import parse_args, run_verb
@@ -34,7 +33,6 @@ def main() -> None:
         else:
             die(f"Error: {args.directory} is not a directory!")
 
-    args = parse_args(directory=Path("."))
     run_verb(args)
 
 
index 93642e649ba6b42e4dee4280cba393aa0981091a..6e6d142ef0a73faea8c4e3b6b3e848cf5ffc3bba 100644 (file)
@@ -2,12 +2,13 @@ import argparse
 import configparser
 import dataclasses
 import enum
+import fnmatch
 import os
 from collections.abc import Sequence
 from pathlib import Path
 from typing import Any, Callable, Optional, Type, Union, cast
 
-from mkosi.backend import Distribution
+from mkosi.backend import Distribution, detect_distribution
 from mkosi.log import die
 
 
@@ -30,61 +31,85 @@ def parse_source_target_paths(value: str) -> tuple[Path, Optional[Path]]:
     return Path(src), Path(target) if target else None
 
 
-def config_parse_string(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-    setattr(namespace, dest, value)
+def config_parse_string(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[str]:
+    if dest in namespace:
+        return getattr(namespace, dest) # type: ignore
+
+    return value if value else None
 
 
 def config_match_string(dest: str, value: str, namespace: argparse.Namespace) -> bool:
     return cast(bool, value == getattr(namespace, dest))
 
 
-def config_parse_script(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-    if value is not None:
+def config_parse_script(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[Path]:
+    if dest in namespace:
+        return getattr(namespace, dest) # type: ignore
+
+    if value:
         if not Path(value).exists():
             die(f"{value} does not exist")
         if not os.access(value, os.X_OK):
             die(f"{value} is not executable")
 
-    config_make_path_parser(required=True)(dest, value, namespace)
+    return Path(value) if value else None
 
 
-def config_parse_boolean(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-    setattr(namespace, dest, parse_boolean(value) if value is not None else False)
+def config_parse_boolean(dest: str, value: Optional[str], namespace: argparse.Namespace) -> bool:
+    if dest in namespace:
+        return getattr(namespace, dest) # type: ignore
+
+    return parse_boolean(value) if value else False
 
 
 def config_match_boolean(dest: str, value: str, namespace: argparse.Namespace) -> bool:
     return cast(bool, getattr(namespace, dest) == parse_boolean(value))
 
 
-def config_parse_feature(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-    if value is None:
-        value = "auto"
-    setattr(namespace, dest, parse_boolean(value) if value != "auto" else None)
+def config_parse_feature(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[bool]:
+    if dest in namespace:
+        return getattr(namespace, dest) # type: ignore
+
+    if value and value == "auto":
+        return None
 
+    return parse_boolean(value) if value else None
+
+
+def config_parse_compression(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Union[None, str, bool]:
+    if dest in namespace:
+        return getattr(namespace, dest) # type: ignore
 
-def config_parse_compression(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
     if value in ("zlib", "lzo", "zstd", "lz4", "xz"):
-        setattr(namespace, dest, value)
-    else:
-        setattr(namespace, dest, parse_boolean(value) if value is not None else None)
+        return value
+
+    return parse_boolean(value) if value else None
+
 
+def config_parse_base_packages(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Union[bool, str]:
+    if dest in namespace:
+        return getattr(namespace, dest) # type: ignore
 
-def config_parse_base_packages(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
     if value == "conditional":
-        setattr(namespace, dest, value)
-    else:
-        setattr(namespace, dest, parse_boolean(value) if value is not None else False)
+        return value
 
+    return parse_boolean(value) if value else False
 
-def config_parse_distribution(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-    assert value is not None
 
-    try:
-        d = Distribution[value]
-    except KeyError:
-        die(f"Invalid distribution {value}")
+def config_default_release(namespace: argparse.Namespace) -> Any:
+    # If we encounter Release in [Match] and no distribution has been set yet, configure the default
+    # distribution as well since the default release depends on the selected distribution.
+    if "distribution" not in namespace:
+        setattr(namespace, "distribution", detect_distribution()[0])
 
-    r = {
+    d = getattr(namespace, "distribution")
+
+    # If the configured distribution matches the host distribution, use the same release as the host.
+    hd, hr = detect_distribution()
+    if d == hd:
+        return hr
+
+    return {
         Distribution.fedora: "37",
         Distribution.centos: "9",
         Distribution.rocky: "9",
@@ -97,12 +122,10 @@ def config_parse_distribution(dest: str, value: Optional[str], namespace: argpar
         Distribution.gentoo: "17.1",
     }.get(d, "rolling")
 
-    setattr(namespace, dest, d)
-    setattr(namespace, "release", r)
 
-
-ConfigParseCallback = Callable[[str, Optional[str], argparse.Namespace], None]
+ConfigParseCallback = Callable[[str, Optional[str], argparse.Namespace], Any]
 ConfigMatchCallback = Callable[[str, str, argparse.Namespace], bool]
+ConfigDefaultCallback = Callable[[argparse.Namespace], Any]
 
 
 @dataclasses.dataclass(frozen=True)
@@ -113,6 +136,7 @@ class MkosiConfigSetting:
     match: Optional[ConfigMatchCallback] = None
     name: str = ""
     default: Any = None
+    default_factory: Optional[ConfigDefaultCallback] = None
     paths: tuple[str, ...] = tuple()
 
     def __post_init__(self) -> None:
@@ -121,12 +145,11 @@ class MkosiConfigSetting:
 
 
 class MkosiConfigParser:
-    def __init__(self, settings: Sequence[MkosiConfigSetting], directory: Path) -> None:
+    def __init__(self, settings: Sequence[MkosiConfigSetting]) -> None:
         self.settings = settings
-        self.directory = directory
         self.lookup = {s.name: s for s in settings}
 
-    def _parse_config(self, path: Path, namespace: argparse.Namespace) -> None:
+    def parse(self, path: Path, namespace: argparse.Namespace) -> argparse.Namespace:
         extras = path.is_dir()
 
         if path.is_dir():
@@ -150,32 +173,46 @@ class MkosiConfigParser:
                 if not (s := self.lookup.get(k)):
                     die(f"Unknown setting {k}")
 
-                if s.match and not s.match(s.dest, v, namespace):
-                    return
+                if not (match := s.match):
+                    die(f"{k} cannot be used in [Match]")
 
-        parser.remove_section("Match")
+                # If we encounter a setting in [Match] that has not been explicitly configured yet, we assign
+                # it it's default value first so that we can [Match] on default values for settings.
+                if s.dest not in namespace:
+                    if s.default_factory:
+                        default = s.default_factory(namespace)
+                    elif s.default is None:
+                        default = s.parse(s.dest, None, namespace)
+                    else:
+                        default = s.default
 
-        if extras:
-            for s in self.settings:
-                for f in s.paths:
-                    if path.parent.joinpath(f).exists():
-                        s.parse(s.dest, str(path.parent / f), namespace)
+                    setattr(namespace, s.dest, default)
+
+                if not match(s.dest, v, namespace):
+                    return namespace
+
+        parser.remove_section("Match")
 
         for section in parser.sections():
             for k, v in parser.items(section):
                 if not (s := self.lookup.get(k)):
                     die(f"Unknown setting {k}")
 
-                s.parse(s.dest, v, namespace)
+                setattr(namespace, s.dest, s.parse(s.dest, v, namespace))
 
-        if extras and path.parent.joinpath("mkosi.conf.d").exists():
-            for p in sorted(path.parent.joinpath("mkosi.conf.d").iterdir()):
-                if p.is_dir() or p.suffix == ".conf":
-                    self._parse_config(p, namespace)
+        if extras:
+            # Dropin configuration has priority over any default paths.
 
+            if path.parent.joinpath("mkosi.conf.d").exists():
+                for p in sorted(path.parent.joinpath("mkosi.conf.d").iterdir()):
+                    if p.is_dir() or p.suffix == ".conf":
+                        namespace = self.parse(p, namespace)
+
+            for s in self.settings:
+                for f in s.paths:
+                    if path.parent.joinpath(f).exists():
+                        setattr(namespace, s.dest, s.parse(s.dest, str(path.parent / f), namespace))
 
-    def parse(self, namespace: argparse.Namespace = argparse.Namespace()) -> argparse.Namespace:
-        self._parse_config(self.directory, namespace)
         return namespace
 
 
@@ -201,11 +238,11 @@ def config_make_action(settings: Sequence[MkosiConfigSetting]) -> Type[argparse.
                 die(f"Unknown setting {option_string}")
 
             if values is None or isinstance(values, str):
-                s.parse(self.dest, values, namespace)
+                setattr(namespace, s.dest, s.parse(self.dest, values, namespace))
             else:
                 for v in values:
                     assert isinstance(v, str)
-                    s.parse(self.dest, v, namespace)
+                    setattr(namespace, s.dest, s.parse(self.dest, v, namespace))
 
     return MkosiAction
 
@@ -215,14 +252,17 @@ def make_enum_parser(type: Type[enum.Enum]) -> Callable[[str], enum.Enum]:
         try:
             return type[value]
         except KeyError:
-            die(f"Invalid enum value {value}")
+            die(f"Invalid {type.__name__} value \"{value}\"")
 
     return parse_enum
 
 
 def config_make_enum_parser(type: Type[enum.Enum]) -> ConfigParseCallback:
-    def config_parse_enum(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-        setattr(namespace, dest, make_enum_parser(type)(value) if value else None)
+    def config_parse_enum(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[enum.Enum]:
+        if dest in namespace:
+            return getattr(namespace, dest) # type: ignore
+
+        return make_enum_parser(type)(value) if value else None
 
     return config_parse_enum
 
@@ -235,30 +275,45 @@ def config_make_enum_matcher(type: Type[enum.Enum]) -> ConfigMatchCallback:
 
 
 def config_make_list_parser(delimiter: str, parse: Callable[[str], Any] = str) -> ConfigParseCallback:
-    def config_parse_list(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
+    ignore: set[str] = set()
+
+    def config_parse_list(dest: str, value: Optional[str], namespace: argparse.Namespace) -> list[Any]:
+        if dest not in namespace:
+            ignore.clear()
+            l = []
+        else:
+            l = getattr(namespace, dest).copy()
+
         if not value:
-            setattr(namespace, dest, [])
-            return
+            return l # type: ignore
 
         value = value.replace("\n", delimiter)
         values = [v for v in value.split(delimiter) if v]
 
         for v in values:
-            if v == "!*":
-                getattr(namespace, dest).clear()
-            elif v.startswith("!"):
-                setattr(namespace, dest, [i for i in getattr(namespace, dest) if i == parse(v[1:])])
+            if v.startswith("!"):
+                ignore.add(v[1:])
+                continue
+
+            for i in ignore:
+                if fnmatch.fnmatchcase(v, i):
+                    break
             else:
-                getattr(namespace, dest).append(parse(v))
+                l.insert(0, parse(v))
+
+        return l
 
     return config_parse_list
 
 
 def config_make_path_parser(required: bool) -> ConfigParseCallback:
-    def config_parse_path(dest: str, value: Optional[str], namespace: argparse.Namespace) -> None:
-        if value is not None and required and not Path(value).exists():
+    def config_parse_path(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[Path]:
+        if dest in namespace:
+            return getattr(namespace, dest) # type: ignore
+
+        if value and required and not Path(value).exists():
             die(f"{value} does not exist")
 
-        setattr(namespace, dest, Path(value) if value else None)
+        return Path(value) if value else None
 
     return config_parse_path
index 0a53f3be96dcb21d2a0fd0e594022b3303d9f507..0fe97a2694f8178f313bd2040307ec117806359c 100644 (file)
@@ -27,7 +27,7 @@ def cd_temp_dir() -> Iterator[None]:
 
 
 def parse(argv: Optional[List[str]] = None) -> MkosiConfig:
-    return mkosi.load_args(mkosi.parse_args(argv, directory=Path(".")))
+    return mkosi.load_args(mkosi.parse_args(argv))
 
 
 def test_parse_load_verb() -> None: