Drop "first assignment wins" logic

From experience in the systemd repository's usage of presets, we've
learned that we want to have fixed values for certain settings for
presets that cannot be overridden by either the CLI options or by
global config. Good examples are that the Format= of a base image
should always be "directory" and the Format= of an initrd image
should always be "cpio" or that Bootable= should always be "no" for
both the base image and the initrd image and their list of packages
should not be affected by any packages specified on the CLI.

The issue is that with "first assignment wins" logic, we need to add
an explicit "override" mechanism which almost all settings in these
presets would then use to make sure they can't be changed by CLI
options. This seems rather backwards, and is a good indication that
any settings configured in config should not be overridden by settings
set on the CLI.

Even disregarding usage of presets, any existing mkosi config will almost
certainly not be written to expect arbitrary changes in the config due
to options set on the CLI.

Also, it's currently not entirely trivial to set default values for
presets from the global config, because any values set in the global
config cannot be overridden anymore by presets. By not doing "first
assignment wins" logic, this becomes trivial as the global config can
simply set a default value and it can be overridden by presets.

Of course by removing "first assignment wins" logic, we do introduce
the issue again that "first assignment wins" solves in the first place,
which is that it becomes possible to assign a value to a setting, match
on that value and then change the setting later. We acknowledge this by
documenting it in the manual. Also, in some cases, this is exactly what
you want. For example, if you want to use a Fedora rawhide tools tree to
build CentOS 8 images, you have to first match on distribution == centos
and then set Distribution=fedora afterwards for the tools tree preset, so
this actually makes perfect sense in some cases.

While this is technically a compat break, it will only be noticed by users
doing advanced stuff with mkosi, which AFAIK does not exist yet outside of
the systemd repo. In fact even the systemd repo was not broken by this change,
so we should be OK with making it, given the large benefits we get out of it.

This commit also simplifies the interfaces of the parser and matching callbacks
to not take the namespace as an argument anymore, but to simply take the existing
value as an argument instead.

diff --git a/NEWS.md b/NEWS.md
index ad29df6e587bf5b6b57342fa6776e453eb100102..f72d4d4cb6b88aa3b078fd7063b1587b8f2ceee8 100644 (file)
--- a/NEWS.md
+++ b/NEWS.md
@@ -4,6 +4,10 @@
 
 - `mkosi.version` is now picked up from preset and dropin directories as
   well following the usual config precedence logic
+- The "first assignment wins" logic was dropped from configuration
+  parsing. Settings parsed later will now override earlier values and
+  the `!` exclusion logic for lists was removed. Assigning the empty
+  string to a list can be used to clear previously assigned values.
 
 ## v15.1
 

diff --git a/mkosi/config.py b/mkosi/config.py
index b5e899f429d5d25fca27bc4e56623a96c6e1e5d1..5ee5492f677ec551ef7e709c745a744f9daf5422 100644 (file)
--- a/mkosi/config.py
+++ b/mkosi/config.py
@@ -41,8 +41,8 @@ from mkosi.versioncomp import GenericVersion
 
 __version__ = "15.1"
 
-ConfigParseCallback = Callable[[str, Optional[str], argparse.Namespace], Any]
-ConfigMatchCallback = Callable[[str, str, argparse.Namespace], bool]
+ConfigParseCallback = Callable[[Optional[str], Optional[Any]], Any]
+ConfigMatchCallback = Callable[[str, Optional[Any]], bool]
 ConfigDefaultCallback = Callable[[argparse.Namespace], Any]
 
 
@@ -193,30 +193,24 @@ def make_source_target_paths_parser(absolute: bool = True) -> Callable[[str], tu
     return parse_source_target_paths
 
 
-def config_parse_string(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[str]:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
-
+def config_parse_string(value: Optional[str], old: Optional[str]) -> Optional[str]:
     return value if value else None
 
 
 def config_make_string_matcher(allow_globs: bool = False) -> ConfigMatchCallback:
-    def config_match_string(dest: str, value: str, namespace: argparse.Namespace) -> bool:
-        if getattr(namespace, dest, None) is None:
+    def config_match_string(match: str, value: Optional[str]) -> bool:
+        if value is None:
             return False
 
         if allow_globs:
-            return fnmatch.fnmatchcase(getattr(namespace, dest), value)
+            return fnmatch.fnmatchcase(value, match)
         else:
-            return cast(bool, value == getattr(namespace, dest))
+            return match == value
 
     return config_match_string
 
 
-def config_parse_script(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[Path]:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
-
+def config_parse_script(value: Optional[str], old: Optional[Path]) -> Optional[Path]:
     if value:
         path = parse_path(value)
         if not os.access(path, os.X_OK):
@@ -226,17 +220,10 @@ def config_parse_script(dest: str, value: Optional[str], namespace: argparse.Nam
     return None
 
 
-def config_parse_boolean(dest: str, value: Optional[str], namespace: argparse.Namespace) -> bool:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
-
+def config_parse_boolean(value: Optional[str], old: Optional[bool]) -> bool:
     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 parse_feature(value: Optional[str]) -> ConfigFeature:
     if not value or value == ConfigFeature.auto.name:
         return ConfigFeature.auto
@@ -244,21 +231,15 @@ def parse_feature(value: Optional[str]) -> ConfigFeature:
     return ConfigFeature.enabled if parse_boolean(value) else ConfigFeature.disabled
 
 
-def config_parse_feature(dest: str, value: Optional[str], namespace: argparse.Namespace) -> ConfigFeature:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
-
+def config_parse_feature(value: Optional[str], old: Optional[ConfigFeature]) -> ConfigFeature:
     return parse_feature(value)
 
 
-def config_match_feature(dest: str, value: Optional[str], namespace: argparse.Namespace) -> bool:
-    return cast(bool, getattr(namespace, dest) == parse_feature(value))
-
+def config_match_feature(match: Optional[str], value: Optional[ConfigFeature]) -> bool:
+    return value == parse_feature(match)
 
-def config_parse_compression(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[Compression]:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
 
+def config_parse_compression(value: Optional[str], old: Optional[Compression]) -> Optional[Compression]:
     if not value:
         return None
 
@@ -342,18 +323,15 @@ def make_enum_parser(type: Type[enum.Enum]) -> Callable[[str], enum.Enum]:
 
 
 def config_make_enum_parser(type: Type[enum.Enum]) -> ConfigParseCallback:
-    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
-
+    def config_parse_enum(value: Optional[str], old: Optional[enum.Enum]) -> Optional[enum.Enum]:
         return make_enum_parser(type)(value) if value else None
 
     return config_parse_enum
 
 
 def config_make_enum_matcher(type: Type[enum.Enum]) -> ConfigMatchCallback:
-    def config_match_enum(dest: str, value: str, namespace: argparse.Namespace) -> bool:
-        return cast(bool, make_enum_parser(type)(value) == getattr(namespace, dest))
+    def config_match_enum(match: str, value: Optional[enum.Enum]) -> bool:
+        return make_enum_parser(type)(match) == value
 
     return config_match_enum
 
@@ -362,17 +340,12 @@ def config_make_list_parser(delimiter: str,
                             *,
                             parse: Callable[[str], Any] = str,
                             unescape: bool = False) -> ConfigParseCallback:
-    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()
+    def config_parse_list(value: Optional[str], old: Optional[list[Any]]) -> list[Any]:
+        new = old.copy() if old else []
 
+        # Empty strings reset the list.
         if not value:
-            return l # type: ignore
+            return []
 
         if unescape:
             lex = shlex.shlex(value, posix=True)
@@ -383,33 +356,24 @@ def config_make_list_parser(delimiter: str,
         else:
             values = value.replace(delimiter, "\n").split("\n")
 
-        new = []
-
         for v in values:
             if not v:
+                new = []
                 continue
 
-            if v.startswith("!"):
-                ignore.add(v[1:])
-                continue
+            new.append(parse(v))
 
-            for i in ignore:
-                if fnmatch.fnmatchcase(v, i):
-                    break
-            else:
-                new.append(parse(v))
-
-        return new + l
+        return new
 
     return config_parse_list
 
 
-def config_match_image_version(dest: str, value: str, namespace: argparse.Namespace) -> bool:
-    image_version = getattr(namespace, dest)
+def config_match_image_version(match: str, value: Optional[str]) -> bool:
     # If the version is not set it cannot positively compare to anything
-    if image_version is None:
+    if value is None:
         return False
-    image_version = GenericVersion(image_version)
+
+    image_version = GenericVersion(value)
 
     for sigil, opfunc in {
         "==": operator.eq,
@@ -419,14 +383,14 @@ def config_match_image_version(dest: str, value: str, namespace: argparse.Namesp
         ">": operator.gt,
         "<": operator.lt,
     }.items():
-        if value.startswith(sigil):
+        if match.startswith(sigil):
             op = opfunc
-            comp_version = GenericVersion(value[len(sigil):])
+            comp_version = GenericVersion(match[len(sigil):])
             break
     else:
         # default to equality if no operation is specified
         op = operator.eq
-        comp_version = GenericVersion(value)
+        comp_version = GenericVersion(match)
 
     # all constraints must be fulfilled
     if not op(image_version, comp_version):
@@ -458,10 +422,7 @@ def config_make_path_parser(*,
                             expanduser: bool = True,
                             expandvars: bool = True,
                             secret: bool = False) -> ConfigParseCallback:
-    def config_parse_path(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[Path]:
-        if dest in namespace:
-            return getattr(namespace, dest) # type: ignore
-
+    def config_parse_path(value: Optional[str], old: Optional[Path]) -> Optional[Path]:
         if value:
             return parse_path(
                 value,
@@ -478,10 +439,7 @@ def config_make_path_parser(*,
     return config_parse_path
 
 
-def config_parse_filename(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[str]:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
-
+def config_parse_filename(value: Optional[str], old: Optional[str]) -> Optional[str]:
     if not value:
         return None
 
@@ -501,10 +459,7 @@ def match_path_exists(value: str) -> bool:
     return Path(value).exists()
 
 
-def config_parse_root_password(dest: str, value: Optional[str], namespace: argparse.Namespace) -> Optional[tuple[str, bool]]:
-    if dest in namespace:
-        return getattr(namespace, dest) # type: ignore
-
+def config_parse_root_password(value: Optional[str], old: Optional[tuple[str, bool]]) -> Optional[tuple[str, bool]]:
     if not value:
         return None
 
@@ -598,11 +553,11 @@ def config_make_action(settings: Sequence[MkosiConfigSetting]) -> Type[argparse.
                 die(f"Unknown setting {option_string}")
 
             if values is None or isinstance(values, str):
-                setattr(namespace, s.dest, s.parse(self.dest, values, namespace))
+                setattr(namespace, s.dest, s.parse(values, getattr(namespace, self.dest, None)))
             else:
                 for v in values:
                     assert isinstance(v, str)
-                    setattr(namespace, s.dest, s.parse(self.dest, v, namespace))
+                    setattr(namespace, s.dest, s.parse(v, getattr(namespace, self.dest, None)))
 
     return MkosiAction
 
@@ -1627,13 +1582,13 @@ class MkosiConfigParser:
                             if s.default_factory:
                                 default = s.default_factory(namespace)
                             elif s.default is None:
-                                default = s.parse(s.dest, None, namespace)
+                                default = s.parse(None, None)
                             else:
                                 default = s.default
 
                             setattr(namespace, s.dest, default)
 
-                        result = match(s.dest, v, namespace)
+                        result = match(v, getattr(namespace, s.dest))
 
                     elif (m := self.match_lookup.get(k)):
                         result = m.match(v)
@@ -1657,7 +1612,7 @@ class MkosiConfigParser:
                 if not (s := self.settings_lookup.get(k)):
                     die(f"Unknown setting {k}")
 
-                setattr(namespace, s.dest, s.parse(s.dest, v, namespace))
+                setattr(namespace, s.dest, s.parse(v, getattr(namespace, s.dest, None)))
 
         if extras:
             # Dropin configuration has priority over any default paths.
@@ -1669,6 +1624,9 @@ class MkosiConfigParser:
                             self.parse_config(p if p.is_file() else Path("."), namespace)
 
             for s in self.SETTINGS:
+                if s.dest in namespace:
+                    continue
+
                 for f in s.paths:
                     p = parse_path(
                         f,
@@ -1681,7 +1639,7 @@ class MkosiConfigParser:
                     )
                     if p.exists():
                         setattr(namespace, s.dest,
-                                s.parse(s.dest, p.read_text() if s.path_read_text else f, namespace))
+                                s.parse(p.read_text() if s.path_read_text else f, None))
 
         return True
 
@@ -1963,7 +1921,7 @@ class MkosiConfigParser:
                 if s.default_factory:
                     default = s.default_factory(ns)
                 elif s.default is None:
-                    default = s.parse(s.dest, None, ns)
+                    default = s.parse(None, None)
                 else:
                     default = s.default
 

diff --git a/mkosi/resources/mkosi.md b/mkosi/resources/mkosi.md
index 2b956b86ea598fd40093cb8ce793d266ef722f2f..bfb23765d509bf15e5165942bbcb751dacd1105f 100644 (file)
--- a/mkosi/resources/mkosi.md
+++ b/mkosi/resources/mkosi.md
@@ -266,28 +266,28 @@ Configuration is parsed in the following order:
 * 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. Matches can use a pipe
-symbol ("|") after the equals sign ("…=|…"), which causes the match to become a triggering match. The config
-file will be included if the logical AND of all non-triggering matches and the logical OR of all triggering
-matches is satisfied. To negate the result of a match, prefix the argument with an exclamation mark. If an
-argument is prefixed with the pipe symbol and an exclamation mark, the pipe symbol must be passed first, and
-the exclamation second.
-
-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",
+Settings that take a list of values are merged by appending the new
+values to the previously configured values. If a list setting is set to
+the empty list, all previously assigned values are cleared.
+
+To conditionally include configuration files, the `[Match]` section can
+be used. Matches can use a pipe symbol ("|") after the equals sign
+("…=|…"), which causes the match to become a triggering match. The
+config file will be included if the logical AND of all non-triggering
+matches and the logical OR of all triggering matches is satisfied. To
+negate the result of a match, prefix the argument with an exclamation
+mark. If an argument is prefixed with the pipe symbol and an exclamation
+mark, the pipe symbol must be passed first, and the exclamation second.
+
+Note that `[Match]` settings match against the current values of
+specific settings, and do not take into account changes made to the
+setting in configuration files that have not been parsed yet. Also note
+that matching against a setting and then changing its value afterwards
+in a different config file may lead to unexpected results.
+
+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.
@@ -543,12 +543,13 @@ they should be specified with a boolean argument: either "1", "yes", or "true" t
 
 `RepartDirectories=`, `--repart-dir=`
 
-: Paths to directories containing systemd-repart partition definition files that
-  are used when mkosi invokes systemd-repart when building a disk image. If not
-  specified and `mkosi.repart/` exists in the local directory, it will be used
-  instead. Note that mkosi invokes repart with `--root=` set to the root of the
-  image root, so any `CopyFiles=` source paths in partition definition files will
-  be relative to the image root directory.
+: Paths to directories containing systemd-repart partition definition
+  files that are used when mkosi invokes systemd-repart when building a
+  disk image. If `mkosi.repart/` exists in the local directory, it will
+  be used for this purpose as well. Note that mkosi invokes repart with
+  `--root=` set to the root of the image root, so any `CopyFiles=`
+  source paths in partition definition files will be relative to the
+  image root directory.
 
 `SectorSize=`, `--sector-size=`
 
@@ -658,10 +659,9 @@ they should be specified with a boolean argument: either "1", "yes", or "true" t
   image. If the second path is not provided, the directory is copied
   on top of the root directory of the image. Use this to insert files
   and directories into the OS tree before the package manager installs
-  any packages. If this option is not used, but the `mkosi.skeleton/`
-  directory is found in the local directory it is automatically used
-  for this purpose with the root directory as target (also see the
-  "Files" section below).
+  any packages. If the `mkosi.skeleton/` directory is found in the local
+  directory it is also used for this purpose with the root directory as
+  target (also see the "Files" section below).
 
 : As with the base tree logic above, instead of a directory, a tar
   file may be provided too. `mkosi.skeleton.tar` will be automatically
@@ -687,10 +687,9 @@ they should be specified with a boolean argument: either "1", "yes", or "true" t
   to the target directory inside the image. If the second path is not
   provided, the directory is copied on top of the root directory of the
   image. Use this to override any default configuration files shipped
-  with the distribution. If this option is not used, but the
-  `mkosi.extra/` directory is found in the local directory it is
-  automatically used for this purpose with the root directory as target.
-  (also see the "Files" section below).
+  with the distribution. If the `mkosi.extra/` directory is found in the
+  local directory it is also used for this purpose with the root
+  directory as target. (also see the "Files" section below).
 
 : As with the base tree logic above, instead of a directory, a tar
   file may be provided too. `mkosi.extra.tar` will be automatically