From: Daan De Meyer Date: Thu, 24 Aug 2023 10:25:47 +0000 (+0200) Subject: Drop "first assignment wins" logic X-Git-Tag: v16~40^2 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F1830%2Fhead;p=thirdparty%2Fmkosi.git 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 ad29df6e5..f72d4d4cb 100644 --- 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 b5e899f42..5ee5492f6 100644 --- 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 2b956b86e..bfb23765d 100644 --- 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