Repository: osbuild/osbuild
Pull request #1881 (2024-09-11 09:14:38)

util: use non-absolute paths for chroot/mount/umount and add test
[draft as this is a bit of an RFC]

We currently use the absolute path of these binaries in the
helper. This has some advantages but given that we control the
inputs for PATH in general it seems unnecessary.

We are also slightly inconsistent about this in the codebase but
favor the non absolute path version. A quick count:
```
$ git grep '"chroot"'|wc -l
13
$ git grep '"/usr/sbin/chroot"'|grep -v test_|wc -l
8
```
for `mount` and `umount` it seems this is the only place that uses
the absolute path.

It's not an important change but it has the nice property that it
allows us to use e.g. `testutil.mock_command()` in our tests and
it would be nice to be consistent.




github-actions[bot]: This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.
mvo5: This is very minor and I'm fine closing it but it would be nice to get a quick :+1: or :-1: from e.g. @achilleas-k 
achilleas-k: Took the liberty of marking it as ready myself :)
mvo5: > There's a failing test that checks for the abs path in the call to chroot:
> 
> ```
>  >       assert run_argv[0] == "/usr/sbin/chroot"
> E       AssertionError: assert 'chroot' == '/usr/sbin/chroot'
> E         - /usr/sbin/chroot
> E         + chroot
> ```

Thanks, silly me! Should be fixed now.

Patch:
diff --git a/osbuild.spec b/osbuild.spec
index 539bf204..728a5e0f 100644
--- a/osbuild.spec
+++ b/osbuild.spec
@@ -1,7 +1,7 @@
 %global         forgeurl https://github.com/osbuild/osbuild
 %global         selinuxtype targeted
 
-Version:        132
+Version:        133
 
 %forgemeta
 
diff --git a/osbuild/__init__.py b/osbuild/__init__.py
index d986e2a8..8c5123b3 100644
--- a/osbuild/__init__.py
+++ b/osbuild/__init__.py
@@ -10,7 +10,7 @@ independent of osbuild but used across the osbuild codebase.
 
 from .pipeline import Manifest, Pipeline, Stage
 
-__version__ = "132"
+__version__ = "133"
 
 __all__ = [
     "Manifest",
diff --git a/osbuild/util/chroot.py b/osbuild/util/chroot.py
index 3686b7d1..da14bf44 100644
--- a/osbuild/util/chroot.py
+++ b/osbuild/util/chroot.py
@@ -21,15 +21,15 @@ class Chroot:
                 print(f"Making missing chroot directory: {d}")
                 os.makedirs(self.root + d)
 
-        subprocess.run(["/usr/bin/mount", "-t", "proc", "-o", "nosuid,noexec,nodev",
+        subprocess.run(["mount", "-t", "proc", "-o", "nosuid,noexec,nodev",
                         "proc", f"{self.root}/proc"],
                        check=True)
 
-        subprocess.run(["/usr/bin/mount", "-t", "devtmpfs", "-o", "mode=0755,noexec,nosuid,strictatime",
+        subprocess.run(["mount", "-t", "devtmpfs", "-o", "mode=0755,noexec,nosuid,strictatime",
                         "devtmpfs", f"{self.root}/dev"],
                        check=True)
 
-        subprocess.run(["/usr/bin/mount", "-t", "sysfs", "-o", "nosuid,noexec,nodev",
+        subprocess.run(["mount", "-t", "sysfs", "-o", "nosuid,noexec,nodev",
                         "sysfs", f"{self.root}/sys"],
                        check=True)
 
@@ -38,12 +38,12 @@ class Chroot:
     def __exit__(self, exc_type, exc_value, tracebk):
         failed_umounts = []
         for d in ["/proc", "/dev", "/sys"]:
-            if subprocess.run(["/usr/bin/umount", "--lazy", self.root + d], check=False).returncode != 0:
+            if subprocess.run(["umount", "--lazy", self.root + d], check=False).returncode != 0:
                 failed_umounts.append(d)
         if failed_umounts:
             print(f"Error unmounting paths from chroot: {failed_umounts}")
 
     def run(self, cmd, **kwargs):
-        cmd = ["/usr/sbin/chroot", self.root] + cmd
+        cmd = ["chroot", self.root] + cmd
         # pylint: disable=subprocess-run-check
         return subprocess.run(cmd, **kwargs)  # noqa: PLW1510
diff --git a/setup.py b/setup.py
index 22096f7f..c8f8f5ff 100644
--- a/setup.py
+++ b/setup.py
@@ -2,7 +2,7 @@ import setuptools
 
 setuptools.setup(
     name="osbuild",
-    version="132",
+    version="133",
     description="A build system for OS images",
     packages=[
         "osbuild",


Test patch:
diff --git a/osbuild/testutil/__init__.py b/osbuild/testutil/__init__.py
index 0681ff9b..5e937494 100644
--- a/osbuild/testutil/__init__.py
+++ b/osbuild/testutil/__init__.py
@@ -107,7 +107,7 @@ def mock_command(cmd_name: str, script: str):
         # will break. easy enough to fix by using \0 as the separator but
         # then \n in args is kinda rare
         fake_cmd_content = textwrap.dedent(f"""\
-        #!/bin/sh -e
+        #!/bin/bash -e
 
         for arg in "$@"; do
            echo "$arg" >> {cmd_calllog_path}
diff --git a/stages/test/test_dracut.py b/stages/test/test_dracut.py
index c8c1d7ed..99530589 100644
--- a/stages/test/test_dracut.py
+++ b/stages/test/test_dracut.py
@@ -27,5 +27,5 @@ def test_dracut_with_initoverlayfs(mocked_run, tmp_path, stage_module, with_init
     args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
     assert kwargs.get("check") is True
     run_argv = args[0]
-    assert run_argv[0] == "/usr/sbin/chroot"
+    assert run_argv[0] == "chroot"
     assert run_argv[2] == expected_argv2
diff --git a/test/mod/test_util_chroot.py b/test/mod/test_util_chroot.py
index 061f9388..4b6812c9 100644
--- a/test/mod/test_util_chroot.py
+++ b/test/mod/test_util_chroot.py
@@ -5,6 +5,7 @@
 import os
 from unittest.mock import call, patch
 
+from osbuild.testutil import mock_command
 from osbuild.util.chroot import Chroot
 
 
@@ -27,17 +28,29 @@ def test_chroot_context(mocked_run, tmp_path):
         chroot.run(["/bin/false"], check=False)
 
     assert mocked_run.call_args_list == [
-        call(["/usr/bin/mount", "-t", "proc", "-o", "nosuid,noexec,nodev",
+        call(["mount", "-t", "proc", "-o", "nosuid,noexec,nodev",
               "proc", os.fspath(tmp_path / "proc")], check=True),
-        call(["/usr/bin/mount", "-t", "devtmpfs", "-o", "mode=0755,noexec,nosuid,strictatime",
+        call(["mount", "-t", "devtmpfs", "-o", "mode=0755,noexec,nosuid,strictatime",
               "devtmpfs", os.fspath(tmp_path / "dev")], check=True),
-        call(["/usr/bin/mount", "-t", "sysfs", "-o", "nosuid,noexec,nodev",
+        call(["mount", "-t", "sysfs", "-o", "nosuid,noexec,nodev",
               "sysfs", os.fspath(tmp_path / "sys")], check=True),
 
-        call(["/usr/sbin/chroot", os.fspath(tmp_path), "/bin/true"], check=True),
-        call(["/usr/sbin/chroot", os.fspath(tmp_path), "/bin/false"], check=False),
+        call(["chroot", os.fspath(tmp_path), "/bin/true"], check=True),
+        call(["chroot", os.fspath(tmp_path), "/bin/false"], check=False),
 
-        call(["/usr/bin/umount", "--lazy", os.fspath(tmp_path / "proc")], check=False),
-        call(["/usr/bin/umount", "--lazy", os.fspath(tmp_path / "dev")], check=False),
-        call(["/usr/bin/umount", "--lazy", os.fspath(tmp_path / "sys")], check=False),
+        call(["umount", "--lazy", os.fspath(tmp_path / "proc")], check=False),
+        call(["umount", "--lazy", os.fspath(tmp_path / "dev")], check=False),
+        call(["umount", "--lazy", os.fspath(tmp_path / "sys")], check=False),
     ]
+
+
+def test_chroot_integration(tmp_path):
+    # drop the first two arguments ("chroot", "target-dir") from our fake
+    # chroot
+    fake_chroot = r'exec "${@:2}"'
+    with mock_command("mount", ""), mock_command("umount", ""), mock_command("chroot", fake_chroot):
+        with Chroot(os.fspath(tmp_path)) as chroot:
+            ret = chroot.run(["/bin/true"], check=True)
+            assert ret.returncode == 0
+            ret = chroot.run(["/bin/false"], check=False)
+            assert ret.returncode == 1