Merge pull request #3480 from behrmann/fspath

Use `os.fspath` in more places

docs/CODING_STYLE.md

@@ -0,0 +1,40 @@
+---
+title: Coding Style
+category: Contributing
+layout: default
+SPDX-License-Identifier: LGPL-2.1-or-later
+---
+
+# Coding Style
+
+## Python Version
+
+- The lowest supported Python version is CPython 3.9.
+
+## Formatting
+
+- Use the accompanying `.editorconfig` or `.dir-locals.el`.
+- For Python files we use the style from `ruff format` with a line length of 109 characters and four spaces
+  indentation.
+- Indentation with tabs is not allowed.
+- When it improves readability, judicious use of `# noqa: E501` comments is allowed.
+- Long lists, including argument lists, should have a trailing comma to force ruff to split all elements on a
+  line of their own.
+- List of commandline arguments should not split the argument of a commandline option and the option. This
+  needs to be enforced with `# fmt: skip` comments, e.g. do
+  ```python
+  cmd = [
+      "--option", "foo",
+  ]  # fmt: skip
+  ```
+  and do NOT do
+  ```python
+  cmd = [
+      "--option",
+      "foo",
+  ]
+  ```
+- When coercing Path-like objects to strings, use `os.fspath`, since this calls the `__fspath__` protocol
+  instead of `__str__`. It also ensures more type-safety, since every Python object supports `__str__`, but
+  not all support `__fspath__` and this gives the typechecker more information what is expected at this
+  point. It also signals the intent to the reader more than a blanket `str()`.

mkosi/__init__.py

@@ -508,7 +508,7 @@ def finalize_scripts(config: Config, scripts: Mapping[str, Sequence[PathString]]
                         )
                     )
 
-                f.write(f'exec {shlex.join(str(s) for s in script)} "$@"\n')
+                f.write(f'exec {shlex.join(os.fspath(s) for s in script)} "$@"\n')
 
             make_executable(Path(d) / name)
             os.utime(Path(d) / name, (0, 0))
@@ -834,7 +834,7 @@ def run_build_scripts(context: Context) -> None:
                     "--bind", context.artifacts, "/work/artifacts",
                     "--bind", context.package_dir, "/work/packages",
                     *(
-                        ["--bind", str(context.config.build_dir), "/work/build"]
+                        ["--bind", os.fspath(context.config.build_dir), "/work/build"]
                         if context.config.build_dir
                         else []
                     ),
@@ -904,7 +904,7 @@ def run_postinst_scripts(context: Context) -> None:
                     "--bind", context.artifacts, "/work/artifacts",
                     "--bind", context.package_dir, "/work/packages",
                     *(
-                        ["--ro-bind", str(context.config.build_dir), "/work/build"]
+                        ["--ro-bind", os.fspath(context.config.build_dir), "/work/build"]
                         if context.config.build_dir
                         else []
                     ),
@@ -973,7 +973,7 @@ def run_finalize_scripts(context: Context) -> None:
                     "--bind", context.artifacts, "/work/artifacts",
                     "--bind", context.package_dir, "/work/packages",
                     *(
-                        ["--ro-bind", str(context.config.build_dir), "/work/build"]
+                        ["--ro-bind", os.fspath(context.config.build_dir), "/work/build"]
                         if context.config.build_dir
                         else []
                     ),
@@ -1290,10 +1290,10 @@ def finalize_default_initrd(
         "--compress-level", str(config.compress_level),
         "--with-network", str(config.with_network),
         "--cache-only", str(config.cacheonly),
-        *(["--output-directory", str(output_dir)] if output_dir else []),
-        *(["--workspace-directory", str(config.workspace_dir)] if config.workspace_dir else []),
-        *(["--cache-directory", str(config.cache_dir)] if config.cache_dir else []),
-        *(["--package-cache-directory", str(config.package_cache_dir)] if config.package_cache_dir else []),
+        *(["--output-directory", os.fspath(output_dir)] if output_dir else []),
+        *(["--workspace-directory", os.fspath(config.workspace_dir)] if config.workspace_dir else []),
+        *(["--cache-directory", os.fspath(config.cache_dir)] if config.cache_dir else []),
+        *(["--package-cache-directory", os.fspath(config.package_cache_dir)] if config.package_cache_dir else []),  # noqa: E501
         *(["--local-mirror", str(config.local_mirror)] if config.local_mirror else []),
         "--incremental", str(config.incremental),
         *(f"--package={package}" for package in config.initrd_packages),
@@ -1315,14 +1315,14 @@ def finalize_default_initrd(
         *(["--hostname", config.hostname] if config.hostname else []),
         *(["--root-password", rootpwopt] if rootpwopt else []),
         *([f"--environment={k}='{v}'" for k, v in config.environment.items()]),
-        *(["--tools-tree", str(config.tools_tree)] if config.tools_tree and tools else []),
+        *(["--tools-tree", os.fspath(config.tools_tree)] if config.tools_tree and tools else []),
         "--tools-tree-certificates", str(config.tools_tree_certificates),
-        *([f"--extra-search-path={p}" for p in config.extra_search_paths]),
+        *([f"--extra-search-path={os.fspath(p)}" for p in config.extra_search_paths]),
         *(["--proxy-url", config.proxy_url] if config.proxy_url else []),
         *([f"--proxy-exclude={host}" for host in config.proxy_exclude]),
-        *(["--proxy-peer-certificate", str(p)] if (p := config.proxy_peer_certificate) else []),
-        *(["--proxy-client-certificate", str(p)] if (p := config.proxy_client_certificate) else []),
-        *(["--proxy-client-key", str(p)] if (p := config.proxy_client_key) else []),
+        *(["--proxy-peer-certificate", os.fspath(p)] if (p := config.proxy_peer_certificate) else []),
+        *(["--proxy-client-certificate", os.fspath(p)] if (p := config.proxy_client_certificate) else []),
+        *(["--proxy-client-key", os.fspath(p)] if (p := config.proxy_client_key) else []),
         "--selinux-relabel", str(relabel),
         "--include=mkosi-initrd",
     ]  # fmt: skip
@@ -4332,21 +4332,21 @@ def finalize_default_tools(config: Config, *, resources: Path) -> Config:
         "--repository-key-check", str(config.repository_key_check),
         "--repository-key-fetch", str(config.repository_key_fetch),
         "--cache-only", str(config.cacheonly),
-        *(["--output-directory", str(config.output_dir)] if config.output_dir else []),
-        *(["--workspace-directory", str(config.workspace_dir)] if config.workspace_dir else []),
-        *(["--cache-directory", str(config.cache_dir)] if config.cache_dir else []),
-        *(["--package-cache-directory", str(config.package_cache_dir)] if config.package_cache_dir else []),
+        *(["--output-directory", os.fspath(config.output_dir)] if config.output_dir else []),
+        *(["--workspace-directory", os.fspath(config.workspace_dir)] if config.workspace_dir else []),
+        *(["--cache-directory", os.fspath(config.cache_dir)] if config.cache_dir else []),
+        *(["--package-cache-directory", os.fspath(config.package_cache_dir)] if config.package_cache_dir else []),  # noqa: E501
         "--incremental", str(config.incremental),
         *([f"--package={package}" for package in config.tools_tree_packages]),
-        *([f"--package-directory={directory}" for directory in config.tools_tree_package_directories]),
+        *([f"--package-directory={os.fspath(directory)}" for directory in config.tools_tree_package_directories]),  # noqa: E501
         "--output=tools",
         *(["--source-date-epoch", str(config.source_date_epoch)] if config.source_date_epoch is not None else []),  # noqa: E501
         *([f"--environment={k}='{v}'" for k, v in config.environment.items()]),
         *(["--proxy-url", config.proxy_url] if config.proxy_url else []),
         *([f"--proxy-exclude={host}" for host in config.proxy_exclude]),
-        *(["--proxy-peer-certificate", str(p)] if (p := config.proxy_peer_certificate) else []),
-        *(["--proxy-client-certificate", str(p)] if (p := config.proxy_client_certificate) else []),
-        *(["--proxy-client-key", str(p)] if (p := config.proxy_client_key) else []),
+        *(["--proxy-peer-certificate", os.fspath(p)] if (p := config.proxy_peer_certificate) else []),
+        *(["--proxy-client-certificate", os.fspath(p)] if (p := config.proxy_client_certificate) else []),
+        *(["--proxy-client-key", os.fspath(p)] if (p := config.proxy_client_key) else []),
     ]  # fmt: skip
 
     _, [tools] = parse_config(
@@ -4412,7 +4412,7 @@ def run_clean_scripts(config: Config) -> None:
                             "--dir", "/work/out",
                             "--ro-bind", script, "/work/clean",
                             "--ro-bind", json, "/work/config.json",
-                            *(["--bind", str(o), "/work/out"] if (o := config.output_dir_or_cwd()).exists() else []),  # noqa: E501
+                            *(["--bind", os.fspath(o), "/work/out"] if (o := config.output_dir_or_cwd()).exists() else []),  # noqa: E501
                             *sources,
                         ],
                     ),

mkosi/bootloader.py

@@ -1,5 +1,6 @@
 import itertools
 import logging
+import os
 import shutil
 import subprocess
 import sys
@@ -208,7 +209,7 @@ def grub_mkimage(
                 "--prefix", f"/{context.config.distribution.grub_prefix()}",
                 "--output", workdir(output) if output else "/grub/core.img",
                 "--format", target,
-                *(["--sbat", str(workdir(sbat))] if sbat else []),
+                *(["--sbat", os.fspath(workdir(sbat))] if sbat else []),
                 *(["--disable-shim-lock"] if context.config.shim_bootloader == ShimBootloader.none else []),
                 "cat",
                 "cmp",
@@ -237,8 +238,8 @@ def grub_mkimage(
                 options=[
                     "--bind", directory, "/grub",
                     "--ro-bind", earlyconfig.name, workdir(Path(earlyconfig.name)),
-                    *(["--bind", str(output.parent), str(workdir(output.parent))] if output else []),
-                    *(["--ro-bind", str(sbat), str(workdir(sbat))] if sbat else []),
+                    *(["--bind", os.fspath(output.parent), os.fspath(workdir(output.parent))] if output else []),  # noqa: E501
+                    *(["--ro-bind", os.fspath(sbat), os.fspath(workdir(sbat))] if sbat else []),
                 ],
             ),
         )  # fmt: skip

mkosi/initrd.py

@@ -129,7 +129,7 @@ def initrd_finalize(staging_dir: str, output: str, output_dir: str) -> None:
         with umask(~0o700) if os.getuid() == 0 else cast(umask, contextlib.nullcontext()):
             Path(output_dir).mkdir(parents=True, exist_ok=True)
     else:
-        output_dir = str(Path.cwd())
+        output_dir = os.fspath(Path.cwd())
 
     log_notice(f"Copying {staging_dir}/{output} to {output_dir}/{output}")
     # mkosi symlinks the expected output image, so dereference it

mkosi/mounts.py

@@ -50,7 +50,12 @@ def mount_overlay(
         )
 
         try:
-            with OverlayOperation(tuple(str(p) for p in lowerdirs), str(upperdir), str(workdir), str(dst)):
+            with OverlayOperation(
+                tuple(os.fspath(p) for p in lowerdirs),
+                os.fspath(upperdir),
+                os.fspath(workdir),
+                os.fspath(dst),
+            ):
                 yield dst
         finally:
             delete_whiteout_files(upperdir)

mkosi/run.py

@@ -492,7 +492,7 @@ def nosandbox(
 
 def workdir(path: Path, sandbox: Optional[SandboxProtocol] = None) -> str:
     subdir = "/" if sandbox and sandbox == nosandbox else "/work"
-    return joinpath(subdir, str(path))
+    return joinpath(subdir, os.fspath(path))
 
 
 def finalize_passwd_symlinks(root: PathString) -> list[PathString]:
@@ -691,7 +691,7 @@ def sandbox_cmd(
                 cmdline += [
                     "--overlay-lowerdir", overlay / d,
                     "--overlay-upperdir", tmp or "tmpfs",
-                    *(["--overlay-workdir", str(work)] if work else []),
+                    *(["--overlay-workdir", os.fspath(work)] if work else []),
                     "--overlay", Path("/") / d,
                 ]  # fmt: skip
             elif not relaxed:

mkosi/tree.py

@@ -19,7 +19,7 @@
 
 
 def is_subvolume(path: Path) -> bool:
-    return path.is_dir() and path.stat().st_ino == 256 and statfs(str(path)) == BTRFS_SUPER_MAGIC
+    return path.is_dir() and path.stat().st_ino == 256 and statfs(os.fspath(path)) == BTRFS_SUPER_MAGIC
 
 
 def cp_version(*, sandbox: SandboxProtocol = nosandbox) -> GenericVersion:
@@ -42,7 +42,7 @@ def make_tree(
 ) -> Path:
     path = path.absolute()
 
-    if statfs(str(path.parent)) != BTRFS_SUPER_MAGIC:
+    if statfs(os.fspath(path.parent)) != BTRFS_SUPER_MAGIC:
         if use_subvolumes == ConfigFeature.enabled:
             die(f"Subvolumes requested but {path} is not located on a btrfs filesystem")
 
@@ -134,7 +134,7 @@ def copy() -> None:
         use_subvolumes == ConfigFeature.disabled
         or not preserve
         or not is_subvolume(src)
-        or statfs(str(dst.parent)) != BTRFS_SUPER_MAGIC
+        or statfs(os.fspath(dst.parent)) != BTRFS_SUPER_MAGIC
         or (dst.exists() and (not dst.is_dir() or any(dst.iterdir())))
     ):
         with preserve_target_directories_stat(src, dst) if not preserve else contextlib.nullcontext():

mkosi/user.py

@@ -182,7 +182,7 @@ def become_root_in_subuid_range_cmd() -> list[str]:
         "--keep-caps",
     ]  # fmt: skip
 
-    return [str(x) for x in cmd]
+    return cmd
 
 
 def become_root_cmd() -> list[str]:

tests/test_config.py

@@ -1188,7 +1188,7 @@ def test_specifiers(tmp_path: Path) -> None:
             "Image": "",
             "ImageId": "my-image-id",
             "ImageVersion": "1.2.3",
-            "OutputDirectory": str(Path.cwd() / "abcde"),
+            "OutputDirectory": os.fspath(Path.cwd() / "abcde"),
             "Output": "test",
             "ConfigRootDirectory": os.fspath(d),
             "ConfigRootConfdir": os.fspath(d),

tests/test_signing.py

@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 
 
+import os
 import tempfile
 from pathlib import Path
 
@@ -50,7 +51,7 @@ def test_signing_checksums_with_gpg(config: ImageConfig) -> None:
         signing_cert = tmp_path / "signing-cert.pgp"
         gnupghome = tmp_path / ".gnupg"
         gnupghome.mkdir()
-        env = dict(GNUPGHOME=str(gnupghome))
+        env = dict(GNUPGHOME=os.fspath(gnupghome))
 
         # create a brand new signing key
         run(