From 1d416c84f31b35c33c267bea53ae430201478580 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Fri, 12 Sep 2025 13:52:25 -0400 Subject: [PATCH] build-with-container: add argument groups to organize options Use the argparse add_argument_group feature to organize the mass of arguments into more sensible categories. Hopefully, someone reading over the `--help` output can now more easily see options that are useful rather than being overwhelmed by a wall of text. Signed-off-by: John Mulligan (cherry picked from commit 71a1be4dd0aea004da56c2f518ee70a281a3f7d3) (cherry picked from commit 321859b6afabc80b214fb9eff17ab5f1810a09e3) --- src/script/build-with-container.py | 291 +++++++++++++++++++---------- 1 file changed, 197 insertions(+), 94 deletions(-) diff --git a/src/script/build-with-container.py b/src/script/build-with-container.py index 1124645520f..23a23954e2c 100755 --- a/src/script/build-with-container.py +++ b/src/script/build-with-container.py @@ -105,10 +105,15 @@ class DistroKind(StrEnum): CENTOS8 = "centos8" CENTOS9 = "centos9" FEDORA41 = "fedora41" + FEDORA42 = "fedora42" + FEDORA43 = "fedora43" ROCKY9 = "rocky9" ROCKY10 = "rocky10" + UBUNTU2004 = "ubuntu20.04" UBUNTU2204 = "ubuntu22.04" UBUNTU2404 = "ubuntu24.04" + DEBIAN12 = "debian12" + DEBIAN13 = "debian13" @classmethod def uses_dnf(cls): @@ -130,39 +135,70 @@ class DistroKind(StrEnum): @classmethod def aliases(cls): return { + # EL distros str(cls.CENTOS10): cls.CENTOS10, "centos10stream": cls.CENTOS10, str(cls.CENTOS8): cls.CENTOS8, str(cls.CENTOS9): cls.CENTOS9, "centos9stream": cls.CENTOS9, - str(cls.FEDORA41): cls.FEDORA41, - "fc41": cls.FEDORA41, str(cls.ROCKY9): cls.ROCKY9, 'rockylinux9': cls.ROCKY9, str(cls.ROCKY10): cls.ROCKY10, 'rockylinux10': cls.ROCKY10, + # fedora + str(cls.FEDORA41): cls.FEDORA41, + "fc41": cls.FEDORA41, + str(cls.FEDORA42): cls.FEDORA42, + "fc42": cls.FEDORA42, + str(cls.FEDORA43): cls.FEDORA43, + "fc43": cls.FEDORA43, + # ubuntu + str(cls.UBUNTU2004): cls.UBUNTU2004, + "ubuntu-focal": cls.UBUNTU2004, + "focal": cls.UBUNTU2004, str(cls.UBUNTU2204): cls.UBUNTU2204, "ubuntu-jammy": cls.UBUNTU2204, "jammy": cls.UBUNTU2204, str(cls.UBUNTU2404): cls.UBUNTU2404, "ubuntu-noble": cls.UBUNTU2404, "noble": cls.UBUNTU2404, + # debian + str(cls.DEBIAN12): cls.DEBIAN12, + "debian-bookworm": cls.DEBIAN12, + "bookworm": cls.DEBIAN12, + str(cls.DEBIAN13): cls.DEBIAN13, + "debian-trixie": cls.DEBIAN13, + "trixie": cls.DEBIAN13, } @classmethod def from_alias(cls, value): - return cls.aliases()[value] + try: + return cls.aliases()[value] + except KeyError: + valid = ", ".join(sorted(cls.aliases())) + msg = f"unknown distro: {value!r} not in {valid}" + raise argparse.ArgumentTypeError(msg) class DefaultImage(StrEnum): + # EL distros CENTOS10 = "quay.io/centos/centos:stream10" CENTOS8 = "quay.io/centos/centos:stream8" CENTOS9 = "quay.io/centos/centos:stream9" - FEDORA41 = "registry.fedoraproject.org/fedora:41" ROCKY9 = "docker.io/rockylinux/rockylinux:9" ROCKY10 = "docker.io/rockylinux/rockylinux:10" + # fedora + FEDORA41 = "registry.fedoraproject.org/fedora:41" + FEDORA42 = "registry.fedoraproject.org/fedora:42" + FEDORA43 = "registry.fedoraproject.org/fedora:43" + # ubuntu + UBUNTU2004 = "docker.io/ubuntu:20.04" UBUNTU2204 = "docker.io/ubuntu:22.04" UBUNTU2404 = "docker.io/ubuntu:24.04" + # debian + DEBIAN12 = "docker.io/debian:bookworm" + DEBIAN13 = "docker.io/debian:trixie" class CommandFailed(Exception): @@ -243,6 +279,12 @@ def _container_cmd(ctx, args, *, workdir=None, interactive=False): cmd.append(f"-eCCACHE_BASEDIR={ctx.cli.homedir}") for extra_arg in ctx.cli.extra or []: cmd.append(extra_arg) + if ctx.npm_cache_dir: + # use :z so that other builds can use the cache + cmd.extend([ + f'--volume={ctx.npm_cache_dir}:/npmcache:z', + '--env=NPM_CACHEDIR=/npmcache' + ]) cmd.append(ctx.image_name) cmd.extend(args) return cmd @@ -292,6 +334,7 @@ class Steps(StrEnum): BUILD_CONTAINER = "build-container" CONTAINER = "container" CONFIGURE = "configure" + NPM_CACHE = "npmcache" BUILD = "build" BUILD_TESTS = "buildtests" TESTS = "tests" @@ -397,6 +440,14 @@ class Context: return path.resolve() return None + @property + def npm_cache_dir(self): + if self.cli.npm_cache_path: + path = pathlib.Path(self.cli.npm_cache_path) + path = path.expanduser() + return path.resolve() + return None + @property def map_user(self): # TODO: detect if uid mapping is needed @@ -530,6 +581,14 @@ def dnf_cache_dir(ctx): (cache_dir / ".DNF_CACHE").touch(exist_ok=True) +@Builder.set(Steps.NPM_CACHE) +def npm_cache_dir(ctx): + """Set up an NPM cache directory for reuse across container builds.""" + if not ctx.cli.npm_cache_path: + return + ctx.npm_cache_dir.mkdir(parents=True, exist_ok=True) + + @Builder.set(Steps.BUILD_CONTAINER) def build_container(ctx): """Generate a build environment container image.""" @@ -547,7 +606,7 @@ def build_container(ctx): cmd.append(f"--build-arg=DISTRO={ctx.from_image}") if ctx.dnf_cache_dir and "docker" in ctx.container_engine: log.warning( - "The --volume option is not supported by docker. Skipping dnf cache dir mounts" + "The --volume option is not supported by docker build/buildx. Skipping dnf cache dir mounts" ) elif ctx.dnf_cache_dir: cmd += [ @@ -625,6 +684,7 @@ def get_container(ctx): def bc_configure(ctx): """Configure the build""" ctx.build.wants(Steps.CONTAINER, ctx) + ctx.build.wants(Steps.NPM_CACHE, ctx) cmd = _container_cmd( ctx, [ @@ -640,6 +700,7 @@ def bc_configure(ctx): @Builder.set(Steps.BUILD) def bc_build(ctx): """Execute a standard build.""" + ctx.build.wants(Steps.NPM_CACHE, ctx) ctx.build.wants(Steps.CONFIGURE, ctx) cmd = _container_cmd( ctx, @@ -656,6 +717,7 @@ def bc_build(ctx): @Builder.set(Steps.BUILD_TESTS) def bc_build_tests(ctx): """Build the tests.""" + ctx.build.wants(Steps.NPM_CACHE, ctx) ctx.build.wants(Steps.CONFIGURE, ctx) cmd = _container_cmd( ctx, @@ -672,6 +734,7 @@ def bc_build_tests(ctx): @Builder.set(Steps.TESTS) def bc_run_tests(ctx): """Execute the tests.""" + ctx.build.wants(Steps.NPM_CACHE, ctx) ctx.build.wants(Steps.BUILD_TESTS, ctx) cmd = _container_cmd( ctx, @@ -687,7 +750,8 @@ def bc_run_tests(ctx): @Builder.set(Steps.SOURCE_RPM) def bc_make_source_rpm(ctx): - """Build SPRMs.""" + """Build SRPMs.""" + ctx.build.wants(Steps.NPM_CACHE, ctx) ctx.build.wants(Steps.CONTAINER, ctx) make_srpm_cmd = f"cd {ctx.cli.homedir} && ./make-srpm.sh" if ctx.cli.ceph_version: @@ -920,19 +984,15 @@ def parse_cli(build_step_names): formatter_class=argparse.RawDescriptionHelpFormatter, ) parser.add_argument( - "--debug", + "--help-build-steps", action="store_true", - help="Emit debugging level logging and tracebacks", - ) - parser.add_argument( - "--container-engine", - help="Select container engine to use (eg. podman, docker)", + help="Print executable build steps and brief descriptions", ) - parser.add_argument( - "--cwd", - help="Change working directory before executing commands", + + g_basic = parser.add_argument_group( + title="Basic options", ) - parser.add_argument( + g_basic.add_argument( "--distro", "-d", choices=DistroKind.aliases().keys(), @@ -940,56 +1000,97 @@ def parse_cli(build_step_names): default=str(DistroKind.CENTOS9), help="Specify a distro short name", ) - parser.add_argument( + g_basic.add_argument( + "--execute", + "-e", + dest="steps", + action="append", + choices=build_step_names, + help="Execute the target build step(s)", + ) + g_basic.add_argument( + "--cwd", + help="Change working directory before executing commands", + ) + g_basic.add_argument( + "--build-dir", + "-b", + help=( + "Specify a build directory relative to the home dir" + " (the ceph source root)" + ), + ) + g_basic.add_argument( + "--env-file", + type=pathlib.Path, + help="Use this environment file when building", + ) + + g_debug = parser.add_argument_group( + title="Debugging options", + ) + g_debug.add_argument( + "--debug", + action="store_true", + help="Emit debugging level logging and tracebacks", + ) + g_debug.add_argument( + "--dry-run", + action="store_true", + help="Do not execute key commands, print and continue if possible", + ) + g_debug.add_argument( + "--no-prereqs", + "-P", + action="store_true", + help="Do not execute any prerequisite steps. Only execute specified steps", + ) + + g_image = parser.add_argument_group( + title="Build image configuration", + description=( + 'These options customize what and how the "Build Image" is' + " constructed" + ), + ) + g_image.add_argument( "--tag", "-t", help="Specify a container tag. Append to the auto generated tag" " by prefixing the supplied value with the plus (+) character", ) - parser.add_argument( + g_image.add_argument( "--base-branch", help="Specify a base branch name", ) - parser.add_argument( + g_image.add_argument( "--current-branch", help="Manually specify the current branch name", ) - parser.add_argument( + g_image.add_argument( "--image-repo", help="Specify a container image repository", ) - parser.add_argument( + g_image.add_argument( "--image-sources", "-I", type=ImageSource.argument, help="Specify a set of valid image sources. " f"May be a comma separated list of {ImageSource.hint()}", ) - parser.add_argument( + g_image.add_argument( "--base-image", help=( "Supply a custom base image to use instead of the default" " image for the source distro." ), ) - parser.add_argument( + g_image.add_argument( "--homedir", default="/ceph", help="Container image home/build dir", ) - parser.add_argument( - "--dnf-cache-path", - help="DNF caching using provided base dir", - ) - parser.add_argument( - "--build-dir", - "-b", - help=( - "Specify a build directory relative to the home dir" - " (the ceph source root)" - ), - ) - parser.add_argument( + g_image.add_argument( "--build-arg", dest="build_args", action="append", @@ -998,7 +1099,37 @@ def parse_cli(build_step_names): " Can be used to override default build image behavior." ), ) - parser.add_argument( + g_image.add_argument( + "--containerfile", + default="Dockerfile.build", + help="Specify the path to a (build) container file", + ) + g_image.add_argument( + "--containerdir", + default=".", + help="Specify the path to container context dir", + ) + + g_container = parser.add_argument_group( + title="Container options", + description="Options to control how the containers are run", + ) + g_container.add_argument( + "--container-engine", + help="Select container engine to use (eg. podman, docker)", + ) + g_container.add_argument( + "--extra", + "-x", + action="append", + help="Specify an extra argument to pass to container command", + ) + g_container.add_argument( + "--keep-container", + action="store_true", + help="Skip removing container after executing command", + ) + g_container.add_argument( "--overlay-dir", "-l", help=( @@ -1007,52 +1138,46 @@ def parse_cli(build_step_names): "use a temporary overlay (discarding writes on container exit)" ), ) - parser.add_argument( + + g_caching = parser.add_argument_group( + title="Persistent cache options", + description=( + "Options to control caches that persist after the containers" + " have exited" + ), + ) + g_caching.add_argument( + "--dnf-cache-path", + help="DNF caching using provided base dir (during build-container build)", + ) + g_caching.add_argument( + "--npm-cache-path", + help="NPM caching using provided base dir (during build)", + ) + g_caching.add_argument( "--ccache-dir", help=( "Specify a directory (within the container) to save ccache" " output" ), ) - parser.add_argument( - "--extra", - "-x", - action="append", - help="Specify an extra argument to pass to container command", - ) - parser.add_argument( - "--keep-container", - action="store_true", - help="Skip removing container after executing command", - ) - parser.add_argument( - "--containerfile", - default="Dockerfile.build", - help="Specify the path to a (build) container file", - ) - parser.add_argument( - "--containerdir", - default=".", - help="Specify the path to container context dir", - ) - parser.add_argument( - "--no-prereqs", - "-P", - action="store_true", - help="Do not execute any prerequisite steps. Only execute specified steps", + + g_pkg = parser.add_argument_group( + title="RPM & DEB package build options", + description="Options specific to building packages", ) - parser.add_argument( + g_pkg.add_argument( "--rpm-no-match-sha", dest="srpm_match", action="store_const", - const='any', + const="any", help=( "Do not try to build RPM packages that match the SHA of the current" " git checkout. Use any source RPM available." " [DEPRECATED] Use --rpm-match=any" ), ) - parser.add_argument( + g_pkg.add_argument( "--srpm-match", dest="srpm_match", choices=("any", "versionglob", "auto"), @@ -1064,39 +1189,17 @@ def parse_cli(build_step_names): " 'auto' (the default) uses a version derived from ceph.spec." ), ) - parser.add_argument( + g_pkg.add_argument( "--rpmbuild-arg", - '-R', + "-R", action="append", help="Pass this extra argument to rpmbuild", ) - parser.add_argument( + g_pkg.add_argument( "--ceph-version", help="Rather than infer the Ceph version, use this value", ) - parser.add_argument( - "--execute", - "-e", - dest="steps", - action="append", - choices=build_step_names, - help="Execute the target build step(s)", - ) - parser.add_argument( - "--env-file", - type=pathlib.Path, - help="Use this environment file when building", - ) - parser.add_argument( - "--dry-run", - action="store_true", - help="Do not execute key commands, print and continue if possible", - ) - parser.add_argument( - "--help-build-steps", - action="store_true", - help="Print executable build steps and brief descriptions", - ) + cli, rest = parser.parse_my_args() if cli.help_build_steps: print("Executable Build Steps") @@ -1176,4 +1279,4 @@ def main(): if __name__ == "__main__": - main() + main() \ No newline at end of file -- 2.39.5