From: Daan De Meyer Date: Thu, 13 Feb 2025 13:52:34 +0000 (+0100) Subject: Stop using nargs="?" for options X-Git-Tag: v26~387 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4519cfd9f5f4a32c96e4d4e0f2df7005b677057f;p=thirdparty%2Fmkosi.git Stop using nargs="?" for options This allows us to get rid of the ambiguity when parsing the verb which could be interpreted as the argument of the previous option by argparse. We also have to change the argument parser plumbing to only allow specifying short options without an argument. --- diff --git a/mkosi/completion.py b/mkosi/completion.py index 9f80180c5..3f2fd3455 100644 --- a/mkosi/completion.py +++ b/mkosi/completion.py @@ -63,7 +63,6 @@ class CompletionItem: short: Optional[str] long: Optional[str] help: Optional[str] - nargs: Union[str, int] choices: list[str] compgen: CompGen @@ -76,7 +75,6 @@ def collect_completion_arguments() -> list[CompletionItem]: short=next((s for s in action.option_strings if not s.startswith("--")), None), long=next((s for s in action.option_strings if s.startswith("--")), None), help=action.help, - nargs=action.nargs or 0, choices=[str(c) for c in action.choices] if action.choices is not None else [], compgen=CompGen.from_action(action), ) @@ -93,7 +91,6 @@ def collect_completion_arguments() -> list[CompletionItem]: short=setting.short, long=setting.long, help=setting.help, - nargs=setting.nargs or 1, choices=[str(c) for c in setting.choices] if setting.choices is not None else [], compgen=CompGen.default, ) @@ -123,12 +120,6 @@ def finalize_completion_bash(options: list[CompletionItem], resources: Path) -> c.write(to_bash_array("_mkosi_options", options_by_key.keys())) c.write("\n\n") - nargs = to_bash_hasharray( - "_mkosi_nargs", {optname: v.nargs for optname, v in options_by_key.items()} - ) - c.write(nargs) - c.write("\n\n") - choices = to_bash_hasharray( "_mkosi_choices", {optname: " ".join(v.choices) for optname, v in options_by_key.items() if v.choices}, @@ -174,8 +165,7 @@ def finalize_completion_fish(options: list[CompletionItem], resources: Path) -> c.write(f"-s {option.short.lstrip('-')} ") if option.long: c.write(f"-l {option.long.lstrip('-')} ") - if isinstance(option.nargs, int) and option.nargs > 0: - c.write("-r ") + c.write("-r ") if option.choices: c.write('-a "') c.write(" ".join(option.choices)) diff --git a/mkosi/config.py b/mkosi/config.py index 23a1519d6..120292d4a 100644 --- a/mkosi/config.py +++ b/mkosi/config.py @@ -1517,7 +1517,6 @@ class ConfigSetting(Generic[T]): long: str = "" choices: Optional[list[str]] = None metavar: Optional[str] = None - nargs: Optional[str] = None const: Optional[Any] = None help: Optional[str] = None @@ -2405,7 +2404,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="repository_key_check", metavar="BOOL", - nargs="?", section="Distribution", default=True, parse=config_parse_boolean, @@ -2415,7 +2413,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="repository_key_fetch", metavar="BOOL", - nargs="?", section="Distribution", default_factory_depends=("distribution", "tools_tree", "tools_tree_distribution"), default_factory=config_default_repository_key_fetch, @@ -2481,7 +2478,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="compress_output", metavar="ALG", - nargs="?", section="Output", parse=config_parse_compression, default_factory=config_default_compression, @@ -2538,7 +2534,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ), ConfigSetting( dest="split_artifacts", - nargs="?", section="Output", parse=config_parse_artifact_output_list, default=ArtifactOutput.compat_no(), @@ -2565,7 +2560,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="overlay", metavar="BOOL", - nargs="?", section="Output", parse=config_parse_boolean, help="Only output the additions on top of the given base trees", @@ -2638,7 +2632,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="with_recommends", metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, help="Install recommended packages", @@ -2646,7 +2639,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="with_docs", metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, default=True, @@ -2779,7 +2771,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="bootable", metavar="FEATURE", - nargs="?", section="Content", parse=config_parse_feature, match=config_match_feature, @@ -2852,7 +2843,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="microcode_host", metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, default=False, @@ -2911,7 +2901,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="kernel_modules_initrd", metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, default=True, @@ -3016,8 +3005,8 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="autologin", short="-a", + const=True, metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, help="Enable root autologin", @@ -3025,7 +3014,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="make_initrd", metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, help="Make sure the image can be used as an initramfs", @@ -3033,7 +3021,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="ssh", metavar="BOOL", - nargs="?", section="Content", parse=config_parse_boolean, help="Set up SSH access from the host to the final image via 'mkosi ssh'", @@ -3050,7 +3037,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="secure_boot", metavar="BOOL", - nargs="?", section="Validation", parse=config_parse_boolean, help="Sign the resulting kernel/initrd image for UEFI SecureBoot", @@ -3207,7 +3193,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="checksum", metavar="BOOL", - nargs="?", section="Validation", parse=config_parse_boolean, help="Write SHA256SUMS file", @@ -3215,7 +3200,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="sign", metavar="BOOL", - nargs="?", section="Validation", parse=config_parse_boolean, help="Write and sign SHA256SUMS file", @@ -3240,8 +3224,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ parse=config_make_path_parser(constants=("default",)), paths=("mkosi.tools",), help="Look up programs to execute inside the given tree", - nargs="?", - const="default", scope=SettingScope.universal, ), ConfigSetting( @@ -3339,7 +3321,7 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="incremental", short="-i", - nargs="?", + const=Incremental.yes, section="Build", parse=config_make_enum_parser_with_boolean(Incremental, yes=Incremental.yes, no=Incremental.no), default=Incremental.no, @@ -3425,7 +3407,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="use_subvolumes", metavar="FEATURE", - nargs="?", section="Build", parse=config_parse_feature, help="Use btrfs subvolumes for faster directory operations where possible", @@ -3465,7 +3446,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ), ConfigSetting( dest="build_sources_ephemeral", - nargs="?", section="Build", parse=config_make_enum_parser_with_boolean( BuildSourcesEphemeral, yes=BuildSourcesEphemeral.yes, no=BuildSourcesEphemeral.no @@ -3496,9 +3476,7 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="with_tests", short="-T", - long="--without-tests", - nargs="?", - const="no", + const=False, section="Build", parse=config_parse_boolean, default=True, @@ -3508,7 +3486,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="with_network", metavar="BOOL", - nargs="?", section="Build", parse=config_parse_boolean, help="Run build and postinst scripts with network access (instead of private network)", @@ -3578,7 +3555,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ "If specified, the container/VM is run with a temporary snapshot of the output " "image that is removed immediately when the container/VM terminates" ), - nargs="?", ), ConfigSetting( dest="credentials", @@ -3700,7 +3676,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="console", metavar="MODE", - nargs="?", section="Runtime", parse=config_make_enum_parser(ConsoleMode), help="Configure the virtual machine console mode to use", @@ -3732,7 +3707,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ dest="kvm", name="KVM", metavar="FEATURE", - nargs="?", section="Runtime", parse=config_parse_feature, help="Configure whether to use KVM or not", @@ -3743,7 +3717,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ dest="vsock", name="VSock", metavar="FEATURE", - nargs="?", section="Runtime", parse=config_parse_feature, help="Configure whether to use vsock or not", @@ -3766,7 +3739,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ dest="tpm", name="TPM", metavar="FEATURE", - nargs="?", section="Runtime", parse=config_parse_feature, help="Configure whether to use a virtual tpm or not", @@ -3777,7 +3749,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ dest="cdrom", name="CDROM", metavar="BOOLEAN", - nargs="?", section="Runtime", parse=config_parse_boolean, help="Attach the image as a CD-ROM to the virtual machine", @@ -3787,7 +3758,6 @@ SETTINGS: list[ConfigSetting[Any]] = [ ConfigSetting( dest="removable", metavar="BOOLEAN", - nargs="?", section="Runtime", parse=config_parse_boolean, help="Attach the image as a removable drive to the virtual machine", @@ -4060,6 +4030,7 @@ def create_argument_parser(chdir: bool = True) -> argparse.ArgumentParser: parser.add_argument( "verb", type=Verb, + nargs="?", choices=list(Verb), default=Verb.build, help=argparse.SUPPRESS, @@ -4084,16 +4055,25 @@ def create_argument_parser(chdir: bool = True) -> argparse.ArgumentParser: group = parser.add_argument_group(f"{s.section} configuration options") last_section = s.section + if s.short and s.const is not None: + group.add_argument( # type: ignore + s.short, + metavar="", + dest=s.dest, + const=s.const, + help="", + action="store_const", + default=argparse.SUPPRESS, + ) + for long in [s.long, *s.compat_longs]: - opts = [s.short, long] if s.short and long == s.long else [long] + opts = [s.short, long] if s.short and long == s.long and s.const is None else [long] group.add_argument( # type: ignore *opts, dest=s.dest, choices=s.choices, metavar=s.metavar, - nargs=s.nargs, # type: ignore - const=s.const, help=s.help if long == s.long else argparse.SUPPRESS, action=ConfigAction, ) @@ -4137,9 +4117,6 @@ class ConfigAction(argparse.Action): ) -> None: assert option_string is not None - if values is None and self.nargs == "?": - values = self.const or "yes" - s = SETTINGS_LOOKUP_BY_DEST[self.dest] if values is None or isinstance(values, str): @@ -4548,25 +4525,6 @@ def parse_config( ) -> tuple[Args, tuple[Config, ...]]: argv = list(argv) - # 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. - for verb in Verb: - try: - v_i = argv.index(verb.value) - except ValueError: - continue - - # Hack to make sure mkosi -C build works. - if argv[v_i - 1] in ("-C", "--directory"): - continue - - if v_i > 0 and argv[v_i - 1] != "--": - argv.insert(v_i, "--") - break - else: - argv += ["--", "build"] - context = ParseContext(resources) # The "image" field does not directly map to a setting but is required to determine some default values diff --git a/mkosi/resources/completion.bash b/mkosi/resources/completion.bash index 593ec849a..8e4fb0796 100644 --- a/mkosi/resources/completion.bash +++ b/mkosi/resources/completion.bash @@ -11,7 +11,7 @@ _mkosi_compgen_dirs() { _mkosi_completion() { local -a _mkosi_options - local -A _mkosi_nargs _mkosi_choices _mkosi_compgen _mkosi_verbs + local -A _mkosi_choices _mkosi_compgen _mkosi_verbs local -i curword_idx verb_seen ##VARIABLEDEFINITIONS## @@ -27,7 +27,6 @@ _mkosi_completion() { elif [[ "$completing_word_preceding" =~ ^- ]] # the previous word was an option then current_option="${completing_word_preceding}" - current_option_nargs="${_mkosi_nargs[${current_option}]}" current_option_choices="${_mkosi_choices[${current_option}]}" current_option_compgen="${_mkosi_compgen[${current_option}]}" @@ -43,12 +42,8 @@ _mkosi_completion() { < <(compgen -W "${current_option_choices}" -- "${completing_word}") # if this (maybe) takes arguments, we'll just fall back to files - if [[ "${current_option_nargs}" == "?" ]] || ((current_option_nargs > 0)) - then - readarray -t COMPREPLY -O "${#COMPREPLY[@]}" \ - < <(_mkosi_compgen_files "${completing_word}") - return - fi + readarray -t COMPREPLY -O "${#COMPREPLY[@]}" \ + < <(_mkosi_compgen_files "${completing_word}") fi # the preceding word wasn't an option or one that doesn't take arguments, diff --git a/mkosi/resources/man/mkosi.1.md b/mkosi/resources/man/mkosi.1.md index e0e21526c..72fdb367c 100644 --- a/mkosi/resources/man/mkosi.1.md +++ b/mkosi/resources/man/mkosi.1.md @@ -1733,7 +1733,7 @@ boolean argument: either `1`, `yes`, or `true` to enable, or `0`, `no`, : Space-delimited list of additional arguments to pass when invoking **qemu**. -`Ephemeral=`, `--ephemeral` +`Ephemeral=`, `--ephemeral=` : When used with the `shell`, `boot`, or `vm` verbs, this option runs the specified verb on a temporary snapshot of the output image that is removed immediately when the container terminates. Taking the temporary snapshot is more efficient on file systems that support reflinks natively (**btrfs** or **xfs**) diff --git a/mkosi/resources/man/mkosi.news.7.md b/mkosi/resources/man/mkosi.news.7.md index 208f44d4d..96a6f967d 100644 --- a/mkosi/resources/man/mkosi.news.7.md +++ b/mkosi/resources/man/mkosi.news.7.md @@ -10,6 +10,13 @@ or `mkosi shell`) should now be delimited from regular options using `--`. Options passed after the verb without using the `--` delimiter are now interpreted as regular mkosi options. +- Boolean options specified on the command line now always expect a + boolean argument. For example, `--repository-key-check` needs to + become `--repository-key-check=yes`. The reason for this change is to + remove ambiguity when parsing e.g. `--repository-key-check build` + where `build` would be interpreted as the argument for + `--repository-key-check` whereas now it'll be properly interpreted as + the verb. - Teach `--verity` a new `hash` value, which skips the verity signature partition for extension / portable images. To align the possible values, `yes` is renamed to `signed`. diff --git a/tests/__init__.py b/tests/__init__.py index 004276b14..98a0aec29 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -122,7 +122,7 @@ class Image: "boot", [ "--runtime-build-sources=no", - "--ephemeral", + "--ephemeral=yes", "--register=no", *options, ], @@ -145,7 +145,7 @@ class Image: # TODO: Drop once both Hyper-V bugs are fixed in Github Actions. "--qemu-args=-cpu max,pcid=off", "--ram=2G", - "--ephemeral", + "--ephemeral=yes", "--register=no", *options, ], diff --git a/tests/test_extension.py b/tests/test_extension.py index 23ac0443d..59f1f1a24 100644 --- a/tests/test_extension.py +++ b/tests/test_extension.py @@ -23,7 +23,7 @@ def test_extension(config: ImageConfig, format: OutputFormat) -> None: "", "--incremental=no", "--base-tree", Path(image.output_dir) / "image", - "--overlay", + "--overlay=yes", "--package=lsof", f"--format={format}", ]