Merge pull request #24 from samsrabin/podman-ubuntu-fix

Fix Podman builds on Ubuntu

Author: samsrabin

doc_builder/build_commands.py

@@ -9,7 +9,7 @@
 
 DEFAULT_IMAGE = "ghcr.io/escomp/ctsm/ctsm-docs:v1.0.1"
 
-# The path in the container's filesystem where the user's home directory is mounted
+# The path in the container's filesystem where doc-builder's parent repo checkout is mounted
 _CONTAINER_HOME = "/home/user/mounted_home"
 
 # CLI tools we can try to use to run our container, in decreasing order of preference
@@ -122,16 +122,13 @@ def get_build_command(
 
     # But if we're using a container, we have more work to do to create the command....
 
-    # Mount the user's home directory in the container; this assumes that both
-    # run_from_dir and build_dir reside somewhere under the user's home directory (we
-    # check this assumption below).
-    container_mountpoint = os.path.expanduser("~")
-
-    errmsg_if_not_under_mountpoint = "build_docs must be run from somewhere in your home directory"
+    # Mount the parent directory of doc-builder in the container; this assumes that both
+    # run_from_dir and build_dir are in this parent directory.
+    container_mountpoint = sys_utils.get_toplevel_of_doc_builder_parent()
     container_workdir = _container_path_from_local_path(
         local_path=run_from_dir,
         container_mountpoint=container_mountpoint,
-        errmsg_if_not_under_mountpoint=errmsg_if_not_under_mountpoint,
+        msg_start="build_docs must be run from",
     )
 
     if os.path.isabs(build_dir):
@@ -141,7 +138,7 @@ def get_build_command(
     container_build_dir = _container_path_from_local_path(
         local_path=build_dir_abs,
         container_mountpoint=container_mountpoint,
-        errmsg_if_not_under_mountpoint="build directory must reside under your home directory",
+        msg_start="build directory must be",
     )
 
     # Get current user's UID and GID
@@ -167,8 +164,8 @@ def get_build_command(
         container_name,
         "--user",
         f"{uid}:{gid}",
-        "--mount",
-        f"type=bind,source={container_mountpoint},target={_CONTAINER_HOME}",
+        "-v",
+        f"{container_mountpoint}:{_CONTAINER_HOME}:U",
         "--workdir",
         container_workdir,
         "-t",  # "-t" is needed for colorful output
@@ -202,21 +199,20 @@ def _get_make_command(build_dir, build_target, num_make_jobs, warnings_as_warnin
     return ["make", sphinxopts, builddir_arg, "-j", str(num_make_jobs), build_target]
 
 
-def _container_path_from_local_path(
-    local_path, container_mountpoint, errmsg_if_not_under_mountpoint
-):
+def _container_path_from_local_path(local_path, container_mountpoint, msg_start):
     """Given a path on the local file system, return the equivalent path in container space
 
     Args:
     - local_path: string: absolute path on local file system; this must reside under
         container_mountpoint
     - container_mountpoint: string: path on local file system that is mounted to _CONTAINER_HOME
-    - errmsg_if_not_under_mountpoint: string: message to print if local_path does not
-        reside under container_mountpoint
+    - msg_start: string: start of the error message
     """
     if not os.path.isabs(local_path):
         raise RuntimeError(f"Expect absolute path; got {local_path}")
 
+    errmsg_if_not_under_mountpoint = f"{msg_start} somewhere in {container_mountpoint}"
+
     local_pathobj = pathlib.Path(local_path)
     try:
         relpath = local_pathobj.relative_to(container_mountpoint)

doc_builder/build_docs_shared_args.py

@@ -28,10 +28,10 @@ def bd_parser(parser, site_root_required=False):
         "rather than relying on locally-installed versions of Sphinx, etc.\n"
         "This checks that a compatible container tool is installed and running on your system.\n"
         "\n"
-        "NOTE: This mounts your home directory in the container.\n"
+        "NOTE: This mounts your CTSM checkout in the container.\n"
         "Therefore, both the current directory (containing the Makefile for\n"
         "building the documentation) and the documentation build directory\n"
-        "must reside somewhere within your home directory."
+        "must reside somewhere within your CTSM checkout."
         "\n"
         f"Default image: {DEFAULT_IMAGE}\n"
         "This can be changed with -i/--container-image.",

doc_builder/sys_utils.py

@@ -102,6 +102,13 @@ def get_toplevel_git_dir(file_or_dir):
     return toplevel
 
 
+def get_toplevel_of_doc_builder_parent():
+    """Get the top level of the repo of which doc-builder is a submodule"""
+    topdir_doc_builder = get_toplevel_git_dir(__file__)
+    topdir_parent = get_toplevel_git_dir(os.path.join(topdir_doc_builder, os.pardir))
+    return topdir_parent
+
+
 def git_current_branch():
     """Determines the name of the current git branch
 
@@ -124,3 +131,15 @@ def git_current_branch():
             branch_name = branch_name.strip()
 
     return branch_found, branch_name
+
+
+def is_under(child, parent_dir):
+    """Check whether child is in parent_dir
+
+    Args:
+        child (str): Path of child directory or file
+        parent_dir (str): Path of parent directory
+    """
+    if parent_dir[-1] != os.sep:
+        parent_dir += os.sep
+    return child.startswith(parent_dir)

test/test_unit_get_build_command.py

@@ -103,10 +103,10 @@ def test_custom_conf_py_path_dir(self):
         ]
         self.assertEqual(expected, build_command)
 
-    @patch("os.path.expanduser")
-    def test_container(self, mock_expanduser):
+    @patch("doc_builder.sys_utils.get_toplevel_of_doc_builder_parent")
+    def test_container(self, mock_get_toplevel_of_doc_builder_parent):
         """Tests usage with container"""
-        mock_expanduser.return_value = "/path/to/username"
+        mock_get_toplevel_of_doc_builder_parent.return_value = "/path/to/username"
         conf_py_path = os.path.join(os.path.dirname(__file__), "conf.py")
         build_command = get_build_command(
             build_dir="/path/to/username/foorepos/foodocs/versions/main",
@@ -125,8 +125,8 @@ def test_container(self, mock_expanduser):
             "foo",
             "--user",
             self.uid_gid,
-            "--mount",
-            "type=bind,source=/path/to/username,target=/home/user/mounted_home",
+            "-v",
+            "/path/to/username:/home/user/mounted_home:U",
             "--workdir",
             "/home/user/mounted_home/foorepos/foocode/doc",
             "-t",
@@ -144,10 +144,10 @@ def test_container(self, mock_expanduser):
         print("build_command: +", " ".join(build_command))
         self.assertEqual(expected, build_command)
 
-    @patch("os.path.expanduser")
-    def test_container_relpath(self, mock_expanduser):
+    @patch("doc_builder.sys_utils.get_toplevel_of_doc_builder_parent")
+    def test_container_relpath(self, mock_get_toplevel_of_doc_builder_parent):
         """Tests usage with container, with a relative path to build_dir"""
-        mock_expanduser.return_value = "/path/to/username"
+        mock_get_toplevel_of_doc_builder_parent.return_value = "/path/to/username"
         build_command = get_build_command(
             build_dir="../../foodocs/versions/main",
             run_from_dir="/path/to/username/foorepos/foocode/doc",
@@ -164,8 +164,8 @@ def test_container_relpath(self, mock_expanduser):
             "foo",
             "--user",
             self.uid_gid,
-            "--mount",
-            "type=bind,source=/path/to/username,target=/home/user/mounted_home",
+            "-v",
+            "/path/to/username:/home/user/mounted_home:U",
             "--workdir",
             "/home/user/mounted_home/foorepos/foocode/doc",
             "-t",
@@ -182,10 +182,10 @@ def test_container_relpath(self, mock_expanduser):
         ]
         self.assertEqual(expected, build_command)
 
-    @patch("os.path.expanduser")
-    def test_container_no_clitool_given(self, mock_expanduser):
+    @patch("doc_builder.sys_utils.get_toplevel_of_doc_builder_parent")
+    def test_container_no_clitool_given(self, mock_get_toplevel_of_doc_builder_parent):
         """Tests usage with container"""
-        mock_expanduser.return_value = "/path/to/username"
+        mock_get_toplevel_of_doc_builder_parent.return_value = "/path/to/username"
         conf_py_path = os.path.join(os.path.dirname(__file__), "conf.py")
         build_command = get_build_command(
             build_dir="/path/to/username/foorepos/foodocs/versions/main",
@@ -203,8 +203,8 @@ def test_container_no_clitool_given(self, mock_expanduser):
             "foo",
             "--user",
             self.uid_gid,
-            "--mount",
-            "type=bind,source=/path/to/username,target=/home/user/mounted_home",
+            "-v",
+            "/path/to/username:/home/user/mounted_home:U",
             "--workdir",
             "/home/user/mounted_home/foorepos/foocode/doc",
             "-t",
@@ -224,13 +224,11 @@ def test_container_no_clitool_given(self, mock_expanduser):
             build_command in [["podman"] + expected, build_command == ["docker"] + expected]
         )
 
-    @patch("os.path.expanduser")
-    def test_container_builddir_not_in_home(self, mock_expanduser):
-        """If build_dir is not in the user's home directory, should raise an exception"""
-        mock_expanduser.return_value = "/path/to/username"
-        with self.assertRaisesRegex(
-            RuntimeError, "build directory must reside under your home directory"
-        ):
+    @patch("doc_builder.sys_utils.get_toplevel_of_doc_builder_parent")
+    def test_container_builddir_not_in_db_checkout(self, mock_get_toplevel_of_doc_builder_parent):
+        """If build_dir is not in parent of doc-builder, should raise an exception"""
+        mock_get_toplevel_of_doc_builder_parent.return_value = "/path/to/username"
+        with self.assertRaisesRegex(RuntimeError, "build directory must be somewhere in"):
             _ = get_build_command(
                 build_dir="/path/to/other/foorepos/foodocs/versions/main",
                 run_from_dir="/path/to/username/foorepos/foocode/doc",
@@ -240,14 +238,11 @@ def test_container_builddir_not_in_home(self, mock_expanduser):
                 version="None",
             )
 
-    @patch("os.path.expanduser")
-    def test_container_runfromdir_not_in_home(self, mock_expanduser):
-        """If run_from_dir is not in the user's home directory, should raise an exception"""
-        mock_expanduser.return_value = "/path/to/username"
-        with self.assertRaisesRegex(
-            RuntimeError,
-            "build_docs must be run from somewhere in your home directory",
-        ):
+    @patch("doc_builder.sys_utils.get_toplevel_of_doc_builder_parent")
+    def test_container_runfromdir_not_in_db_parent(self, mock_get_toplevel_of_doc_builder_parent):
+        """If run_from_dir is not in parent of doc-builder, should raise an exception"""
+        mock_get_toplevel_of_doc_builder_parent.return_value = "/path/to/username"
+        with self.assertRaisesRegex(RuntimeError, "build_docs must be run from somewhere in"):
             _ = get_build_command(
                 build_dir="/path/to/username/foorepos/foodocs/versions/main",
                 run_from_dir="/path/to/other/foorepos/foocode/doc",
@@ -257,10 +252,10 @@ def test_container_runfromdir_not_in_home(self, mock_expanduser):
                 version="None",
             )
 
-    @patch("os.path.expanduser")
-    def test_container_runfromdir_relative(self, mock_expanduser):
+    @patch("doc_builder.sys_utils.get_toplevel_of_doc_builder_parent")
+    def test_container_runfromdir_relative(self, mock_get_toplevel_of_doc_builder_parent):
         """If run_from_dir is relative, should raise an exception"""
-        mock_expanduser.return_value = "/path/to/username"
+        mock_get_toplevel_of_doc_builder_parent.return_value = "/path/to/username"
         with self.assertRaisesRegex(
             RuntimeError,
             "Expect absolute path; got",

test/test_unit_is_under.py

@@ -0,0 +1,74 @@
+#!/usr/bin/env python3
+"""Tests of check_permanent_file
+
+These are integration tests, since they interact with the OS and git,
+and so are slower than typical unit tests.
+"""
+
+import unittest
+import tempfile
+import shutil
+import os
+
+# pylint: disable=import-error,no-name-in-module
+from doc_builder.sys_utils import is_under
+
+
+class TestIsUnder(unittest.TestCase):
+    """Test the is_under function"""
+
+    # ------------------------------------------------------------------------
+    # Helper methods
+    # ------------------------------------------------------------------------
+
+    def setUp(self):
+        self._return_dir = os.getcwd()
+        self._tempdir = os.path.realpath(tempfile.mkdtemp())
+        os.chdir(self._tempdir)
+
+    def tearDown(self):
+        os.chdir(self._return_dir)
+        shutil.rmtree(self._tempdir, ignore_errors=True)
+
+    # ------------------------------------------------------------------------
+    # Begin tests
+    # ------------------------------------------------------------------------
+
+    def test_isunder_true(self):
+        """Test if child is under parent"""
+        my_dir = "some_dir"
+        os.makedirs(my_dir)
+        filename = os.path.join(my_dir, "some_file")
+        with open(filename, "a", encoding="utf8") as myfile:
+            myfile.write("ietbeconae0nr0einr")
+        self.assertTrue(is_under(filename, my_dir))
+        self.assertFalse(is_under(my_dir, filename))
+
+    def test_isunder_true_extraslash(self):
+        """Test if child is under parent and parent has a path separator at the end"""
+        my_dir = "some_dir" + os.sep
+        os.makedirs(my_dir)
+        filename = os.path.join(my_dir, "some_file")
+        with open(filename, "a", encoding="utf8") as myfile:
+            myfile.write("ietbeconae0nr0einr")
+        self.assertTrue(is_under(filename, my_dir))
+        self.assertFalse(is_under(my_dir, filename))
+
+    def test_isunder_false(self):
+        """Test if true"""
+        my_dir = "some_dir"
+        os.makedirs(my_dir)
+        filename = "some_file"
+        with open(filename, "a", encoding="utf8") as myfile:
+            myfile.write("ietbeconae0nr0einr")
+        self.assertFalse(is_under(filename, my_dir))
+
+    def test_isunder_true_neither_exist(self):
+        """Test true even if neither exists"""
+        my_dir = "some_dir"
+        filename = "some_file"
+        self.assertFalse(is_under(filename, my_dir))
+
+
+if __name__ == "__main__":
+    unittest.main()