Merge pull request #1402 from yuvipanda/docker-buildx

Shell out to `docker buildx build` to build images

Author: yuvipanda

repo2docker/__main__.py

@@ -140,7 +140,8 @@ def get_argparser():
 
     argparser.add_argument(
         "--build-memory-limit",
-        help="Total Memory that can be used by the docker build process",
+        # Removed argument, but we still want to support printing an error message if this is passed
+        help=argparse.SUPPRESS,
     )
 
     argparser.add_argument(
@@ -434,12 +435,13 @@ def make_r2d(argv=None):
         sys.exit(1)
 
     if args.build_memory_limit:
-        # if the string only contains numerals we assume it should be an int
-        # and specifies a size in bytes
-        if args.build_memory_limit.isnumeric():
-            r2d.build_memory_limit = int(args.build_memory_limit)
-        else:
-            r2d.build_memory_limit = args.build_memory_limit
+        # We no longer support build_memory_limit, it must be set in the builder instance
+        print("--build-memory-limit is no longer supported", file=sys.stderr)
+        print(
+            "Check your build engine documentation to set memory limits. Use `docker buildx create` if using the default builder to create a custom builder with appropriate memory limits",
+            file=sys.stderr,
+        )
+        sys.exit(-1)
 
     if args.environment and not r2d.run:
         print("To specify environment variables, you also need to run " "the container")

repo2docker/app.py

@@ -165,13 +165,23 @@ def _default_log_level(self):
     build_memory_limit = ByteSpecification(
         0,
         help="""
-        Total memory that can be used by the docker image building process.
+        Unsupported.
 
-        Set to 0 for no limits.
+        When using docker, please use `docker buildx create` to create a new buildkit
+        builder with appropriate limits instead.
         """,
         config=True,
     )
 
+    @observe("build_memory_limit")
+    def build_memory_limit_changed(self, change):
+        print("Setting build_memory_limit is not supported", file=sys.stderr)
+        print(
+            "Check your build engine documentation to set memory limits. Use `docker buildx create` if using the default builder to create a custom builder with appropriate memory limits",
+            file=sys.stderr,
+        )
+        sys.exit(-1)
+
     volumes = Dict(
         {},
         help="""
@@ -856,6 +866,7 @@ def build(self):
                     for l in picked_buildpack.build(
                         docker_client,
                         self.output_image_spec,
+                        # This is deprecated, but passing it anyway to not break backwards compatibility
                         self.build_memory_limit,
                         build_args,
                         self.cache_from,

repo2docker/docker.py

@@ -2,12 +2,17 @@
 Docker container engine for repo2docker
 """
 
+import shutil
+import tarfile
+import tempfile
+
 from iso8601 import parse_date
-from traitlets import Dict
+from traitlets import Dict, List, Unicode
 
 import docker
 
 from .engine import Container, ContainerEngine, ContainerEngineException, Image
+from .utils import execute_cmd
 
 
 class DockerContainer(Container):
@@ -53,7 +58,7 @@ class DockerEngine(ContainerEngine):
     https://docker-py.readthedocs.io/en/4.2.0/api.html#module-docker.api.build
     """
 
-    string_output = False
+    string_output = True
 
     extra_init_args = Dict(
         {},
@@ -69,6 +74,14 @@ class DockerEngine(ContainerEngine):
         config=True,
     )
 
+    extra_buildx_build_args = List(
+        Unicode(),
+        help="""
+        Extra commandline arguments to pass to `docker buildx build` when building the image.
+        """,
+        config=True,
+    )
+
     def __init__(self, *, parent):
         super().__init__(parent=parent)
         try:
@@ -94,22 +107,46 @@ def build(
         platform=None,
         **kwargs,
     ):
-        return self._apiclient.build(
-            buildargs=buildargs,
-            cache_from=cache_from,
-            container_limits=container_limits,
-            forcerm=True,
-            rm=True,
-            tag=tag,
-            custom_context=custom_context,
-            decode=True,
-            dockerfile=dockerfile,
-            fileobj=fileobj,
-            path=path,
-            labels=labels,
-            platform=platform,
-            **kwargs,
-        )
+        if not shutil.which("docker"):
+            raise RuntimeError("The docker commandline client must be installed")
+        args = ["docker", "buildx", "build", "--progress", "plain", "--load"]
+        if buildargs:
+            for k, v in buildargs.items():
+                args += ["--build-arg", f"{k}={v}"]
+
+        if cache_from:
+            for cf in cache_from:
+                args += ["--cache-from", cf]
+
+        if dockerfile:
+            args += ["--file", dockerfile]
+
+        if tag:
+            args += ["--tag", tag]
+
+        if labels:
+            for k, v in labels.items():
+                args += ["--label", f"{k}={v}"]
+
+        if platform:
+            args += ["--platform", platform]
+
+        # place extra args right *before* the path
+        args += self.extra_buildx_build_args
+
+        if fileobj:
+            with tempfile.TemporaryDirectory() as d:
+                tarf = tarfile.open(fileobj=fileobj)
+                tarf.extractall(d)
+
+                args += [d]
+
+                yield from execute_cmd(args, True)
+        else:
+            # Assume 'path' is passed in
+            args += [path]
+
+            yield from execute_cmd(args, True)
 
     def images(self):
         images = self._apiclient.images()

tests/unit/test_app.py

@@ -78,19 +78,18 @@ def test_local_dir_image_name(repo_with_content):
     )
 
 
-def test_build_kwargs(repo_with_content):
+def test_extra_buildx_build_args(repo_with_content):
     upstream, sha1 = repo_with_content
-    argv = [upstream]
+    argv = ["--DockerEngine.extra_buildx_build_args=--check", upstream]
     app = make_r2d(argv)
-    app.extra_build_kwargs = {"somekey": "somevalue"}
-
-    with patch.object(docker.APIClient, "build") as builds:
-        builds.return_value = []
+    with patch("repo2docker.docker.execute_cmd") as execute_cmd:
         app.build()
-    builds.assert_called_once()
-    args, kwargs = builds.call_args
-    assert "somekey" in kwargs
-    assert kwargs["somekey"] == "somevalue"
+
+    args, kwargs = execute_cmd.call_args
+    cmd = args[0]
+    assert cmd[:3] == ["docker", "buildx", "build"]
+    # make sure it's inserted before the end
+    assert "--check" in cmd[:-1]
 
 
 def test_run_kwargs(repo_with_content):
@@ -107,26 +106,6 @@ def test_run_kwargs(repo_with_content):
     assert kwargs["somekey"] == "somevalue"
 
 
-def test_root_not_allowed():
-    with TemporaryDirectory() as src, patch("os.geteuid") as geteuid:
-        geteuid.return_value = 0
-        argv = [src]
-        with pytest.raises(SystemExit) as exc:
-            app = make_r2d(argv)
-            assert exc.code == 1
-
-        with pytest.raises(ValueError):
-            app = Repo2Docker(repo=src, run=False)
-            app.build()
-
-        app = Repo2Docker(repo=src, user_id=1000, user_name="jovyan", run=False)
-        app.initialize()
-        with patch.object(docker.APIClient, "build") as builds:
-            builds.return_value = []
-            app.build()
-        builds.assert_called_once()
-
-
 def test_dryrun_works_without_docker(tmpdir, capsys):
     with chdir(tmpdir):
         with patch.object(docker, "APIClient") as client:

tests/unit/test_args.py

@@ -50,11 +50,11 @@ def test_mem_limit():
     """
     Test various ways of passing --build-memory-limit
     """
-    r2d = make_r2d(["--build-memory-limit", "1024", "."])
-    assert int(r2d.build_memory_limit) == 1024
+    with pytest.raises(SystemExit):
+        r2d = make_r2d(["--build-memory-limit", "1024", "."])
 
-    r2d = make_r2d(["--build-memory-limit", "3K", "."])
-    assert int(r2d.build_memory_limit) == 1024 * 3
+    with pytest.raises(SystemExit):
+        r2d = make_r2d(["--Repo2Docker.build_memory_limit", "1024", "."])
 
 
 def test_run_required():