Bazel: address review comments

Author: redsun82

misc/bazel/internal/install.py

@@ -19,20 +19,20 @@
 parser = argparse.ArgumentParser(description=__doc__)
 parser.add_argument("--destdir", type=pathlib.Path, required=True,
                     help="Desination directory, relative to `--build-file`")
-parser.add_argument("--script", required=True,
+parser.add_argument("--pkg-install-script", required=True,
                     help="The wrapped `pkg_install` installation script rlocation")
 parser.add_argument("--build-file", required=True,
                     help="BUILD.bazel rlocation relative to which the installation should take place")
 parser.add_argument("--ripunzip", required=True,
                     help="ripunzip executable rlocation")
-parser.add_argument("--zip-manifest", action="append", default=[], dest="zip_manifests",
+parser.add_argument("--zip-manifest", required=True,
                     help="The rlocation of a file containing newline-separated `prefix:zip_file` entries")
 opts = parser.parse_args()
 
 build_file = runfiles.Rlocation(opts.build_file)
-script = runfiles.Rlocation(opts.script)
+script = runfiles.Rlocation(opts.pkg_install_script)
 ripunzip = runfiles.Rlocation(opts.ripunzip)
-zip_manifests = [runfiles.Rlocation(z) for z in opts.zip_manifests]
+zip_manifest = runfiles.Rlocation(opts.zip_manifest)
 destdir = pathlib.Path(build_file).resolve().parent / opts.destdir
 
 if destdir.exists():
@@ -41,11 +41,10 @@
 destdir.mkdir(parents=True)
 subprocess.run([script, "--destdir", destdir], check=True)
 
-for zip_manifest in zip_manifests:
-    with open(zip_manifest) as manifest:
-        for line in manifest:
-            prefix, _, zip = line.partition(":")
-            assert zip, f"missing prefix for {prefix}, you should use prefix:zip format"
-            dest = destdir / prefix
-            dest.mkdir(parents=True, exist_ok=True)
-            subprocess.run([ripunzip, "unzip-file", zip, "-d", dest], check=True)
+with open(zip_manifest) as manifest:
+    for line in manifest:
+        prefix, _, zip = line.partition(":")
+        assert zip, f"missing prefix for {prefix}, you should use prefix:zip format"
+        dest = destdir / prefix
+        dest.mkdir(parents=True, exist_ok=True)
+        subprocess.run([ripunzip, "unzip-file", zip, "-d", dest], check=True)

misc/bazel/pkg.bzl

@@ -21,15 +21,15 @@ _PLAT_DETECTION_ATTRS = {
 
 _PLAT_PLACEHOLDER = "{CODEQL_PLATFORM}"
 
-def _process_path(path, plat):
+def _process_path(path, platform):
     if _PLAT_PLACEHOLDER in path:
-        path = path.replace(_PLAT_PLACEHOLDER, plat)
+        path = path.replace(_PLAT_PLACEHOLDER, platform)
         return ("arch", path)
     return ("generic", path)
 
-def _detect_plat(ctx):
+def _detect_platform(ctx):
     if ctx.target_platform_has_constraint(ctx.attr._windows[platform_common.ConstraintValueInfo]):
-        return "windows64"
+        return "win64"
     elif ctx.target_platform_has_constraint(ctx.attr._macos[platform_common.ConstraintValueInfo]):
         return "osx64"
     else:
@@ -80,7 +80,7 @@ def codeql_pkg_files(
 
 def _extract_pkg_filegroup_impl(ctx):
     src = ctx.attr.src[PackageFilegroupInfo]
-    plat = _detect_plat(ctx)
+    platform = _detect_platform(ctx)
 
     if src.pkg_dirs or src.pkg_symlinks:
         fail("`pkg_dirs` and `pkg_symlinks` are not supported for codeql packaging rules")
@@ -89,7 +89,7 @@ def _extract_pkg_filegroup_impl(ctx):
     for pfi, origin in src.pkg_files:
         dest_src_map = {}
         for dest, file in pfi.dest_src_map.items():
-            file_kind, dest = _process_path(dest, plat)
+            file_kind, dest = _process_path(dest, platform)
             if file_kind == ctx.attr.kind:
                 dest_src_map[dest] = file
         if dest_src_map:
@@ -101,25 +101,33 @@ def _extract_pkg_filegroup_impl(ctx):
         DefaultInfo(files = depset(transitive = files)),
     ]
 
-_extrac_pkg_filegroup = rule(
+_extract_pkg_filegroup = rule(
     implementation = _extract_pkg_filegroup_impl,
+    doc = """
+        This internal rule extracts the arch or generic part of a `PackageFilegroupInfo` source, returning a
+        `PackageFilegroupInfo` that is a subset of the provided `src`, while expanding `{CODEQL_PLATFORM}` in
+        destination paths to the relevant codeql platform (linux64, win64 or osx64).
+        The distinction between generic and arch contents is given on a per-file basis depending on the install path
+        containing {CODEQL_PLATFORM}, which will typically have been added by a `prefix` attribute to a `pkg_*` rule.
+        No `pkg_dirs` or `pkg_symlink` must have been used for assembling the source mapping information: we could
+        easily add support for that, but we don't require it for now.
+    """,
     attrs = {
         "src": attr.label(providers = [PackageFilegroupInfo, DefaultInfo]),
-        "kind": attr.string(doc = "generic or arch", values = ["generic", "arch"]),
+        "kind": attr.string(doc = "What part to extract", values = ["generic", "arch"]),
     } | _PLAT_DETECTION_ATTRS,
 )
 
 def _imported_zips_manifest_impl(ctx):
-    plat = _detect_plat(ctx)
+    platform = _detect_platform(ctx)
 
     manifest = []
     files = []
     for zip, prefix in ctx.attr.zips.items():
-        zip_kind, prefix = _process_path(prefix, plat)
-        if zip_kind == ctx.attr.kind:
-            zip_files = zip.files.to_list()
-            manifest += ["%s:%s" % (prefix, f.short_path) for f in zip_files]
-            files += zip_files
+        _, prefix = _process_path(prefix, platform)
+        zip_files = zip.files.to_list()
+        manifest += ["%s:%s" % (prefix, f.short_path) for f in zip_files]
+        files += zip_files
 
     output = ctx.actions.declare_file(ctx.label.name + ".params")
     ctx.actions.write(
@@ -133,21 +141,24 @@ def _imported_zips_manifest_impl(ctx):
 
 _imported_zips_manifest = rule(
     implementation = _imported_zips_manifest_impl,
+    doc = """
+        This internal rule prints a zip manifest file that `misc/bazel/internal/install.py` understands.
+        {CODEQL_PLATFORM} can be used as zip prefixes and will be expanded to the relevant codeql platform.
+    """,
     attrs = {
-        "zips": attr.label_keyed_string_dict(allow_files = True),
-        "kind": attr.string(doc = "generic or arch", values = ["generic", "arch"]),
+        "zips": attr.label_keyed_string_dict(doc = "mapping from zip files to install prefixes", allow_files = True),
     } | _PLAT_DETECTION_ATTRS,
 )
 
 def _zipmerge_impl(ctx):
     zips = []
     filename = ctx.attr.zip_name + "-"
-    plat = _detect_plat(ctx)
-    filename = "%s-%s.zip" % (ctx.attr.zip_name, plat if ctx.attr.kind == "arch" else "generic")
+    platform = _detect_platform(ctx)
+    filename = "%s-%s.zip" % (ctx.attr.zip_name, platform if ctx.attr.kind == "arch" else "generic")
     output = ctx.actions.declare_file(filename)
     args = [output.path, "--prefix=%s" % ctx.attr.zip_prefix, ctx.file.base.path]
     for zip, prefix in ctx.attr.zips.items():
-        zip_kind, prefix = _process_path(prefix, plat)
+        zip_kind, prefix = _process_path(prefix, platform)
         if zip_kind == ctx.attr.kind:
             args.append("--prefix=%s/%s" % (ctx.attr.zip_prefix, prefix.rstrip("/")))
             args += [f.path for f in zip.files.to_list()]
@@ -165,12 +176,24 @@ def _zipmerge_impl(ctx):
 
 _zipmerge = rule(
     implementation = _zipmerge_impl,
+    doc = """
+        This internal rule merges a `base` zip file with the ones indicated by the `zips` mapping where the prefix
+        indicates a matching kind between arch and generic. An imported zip file will be considered arch-specific
+        if its prefix contains `{CODEQL_PLATFORM}` (and this prefix will have that expanded to the appropriate
+        platform).
+
+        The output filename will be either `{zip_name}-generic.zip` or `{zip_name}-{CODEQL_PLATFORM}.zip`, depending on
+        the requested `kind`.
+    """,
     attrs = {
-        "base": attr.label(allow_single_file = True),
-        "zips": attr.label_keyed_string_dict(allow_files = True),
-        "zip_name": attr.string(),
-        "kind": attr.string(doc = "generic or arch", values = ["generic", "arch"]),
-        "zip_prefix": attr.string(),
+        "base": attr.label(
+            doc = "Base zip file to which zips from `zips` will be merged with",
+            allow_single_file = True,
+        ),
+        "zips": attr.label_keyed_string_dict(doc = "mapping from zip files to install prefixes", allow_files = True),
+        "zip_name": attr.string(doc = "Prefix to use for the output file name"),
+        "kind": attr.string(doc = "Which zip kind to consider", values = ["generic", "arch"]),
+        "zip_prefix": attr.string(doc = "Prefix posix path to add to the zip contents in the archive"),
         "_zipmerge": attr.label(default = "//misc/bazel/internal/zipmerge", executable = True, cfg = "exec"),
     } | _PLAT_DETECTION_ATTRS,
 )
@@ -190,14 +213,14 @@ def codeql_pack(
     * defines a `<name>-generic-zip` target creating a `<zip_filename>-generic.zip` archive with the generic bits,
       prefixed with `name`
     * defines a `<name>-arch-zip` target creating a `<zip_filename>-<codeql_platform>.zip` archive with the
-      arch-specific bits, prefixed with `zip_prefix` (`name` by default)
+      arch-specific bits, prefixed with `name`
     * defines a runnable `<name>-installer` target that will install the pack in `install_dest`, relative to where the
       rule is used. The install destination can be overridden appending `-- --destdir=...` to the `bazel run`
-      invocation. This installation does not use the `zip_prefix`.
+      invocation. This installation _does not_ prefix the contents with `name`.
 
     The distinction between arch-specific and generic contents is made based on whether the paths (including possible
     prefixes added by rules) contain the special `{CODEQL_PLATFORM}` placeholder, which in case it is present will also
-    be replaced by the appropriate platform (`linux64`, `windows64` or `osx64`).
+    be replaced by the appropriate platform (`linux64`, `win64` or `osx64`).
     """
     internal = _make_internal(name)
     zip_filename = zip_filename or name
@@ -209,7 +232,7 @@ def codeql_pack(
         **kwargs
     )
     for kind in ("generic", "arch"):
-        _extrac_pkg_filegroup(
+        _extract_pkg_filegroup(
             name = internal(kind),
             src = internal("base"),
             kind = kind,
@@ -229,12 +252,11 @@ def codeql_pack(
             kind = kind,
             visibility = visibility,
         )
-        _imported_zips_manifest(
-            name = internal(kind + "-zip-manifest"),
-            zips = zips,
-            kind = kind,
-            visibility = ["//visibility:private"],
-        )
+    _imported_zips_manifest(
+        name = internal("zip-manifest"),
+        zips = zips,
+        visibility = ["//visibility:private"],
+    )
 
     pkg_install(
         name = internal("script"),
@@ -254,19 +276,17 @@ def codeql_pack(
         data = [
             internal("build-file"),
             internal("script"),
-            internal("generic-zip-manifest"),
-            internal("arch-zip-manifest"),
+            internal("zip-manifest"),
             "//misc/bazel/internal/ripunzip",
         ],
         deps = ["@rules_python//python/runfiles"],
         args = [
             "--build-file=$(rlocationpath %s)" % internal("build-file"),
-            "--script=$(rlocationpath %s)" % internal("script"),
+            "--pkg-install-script=$(rlocationpath %s)" % internal("script"),
             "--destdir",
             install_dest,
             "--ripunzip=$(rlocationpath //misc/bazel/internal/ripunzip)",
-            "--zip-manifest=$(rlocationpath %s)" % internal("generic-zip-manifest"),
-            "--zip-manifest=$(rlocationpath %s)" % internal("arch-zip-manifest"),
+            "--zip-manifest=$(rlocationpath %s)" % internal("zip-manifest"),
         ],
         visibility = visibility,
     )