From 4afd0b3ccd8379af11f67f7ddff525fd1a9c5834 Mon Sep 17 00:00:00 2001 From: elsk Date: Wed, 28 Aug 2024 12:14:02 -0700 Subject: [PATCH] pkg_install: Add destdir attr & read rel paths. (#886) Implementation notes: Relative paths are interpreted against BUILD_WORKSPACE_DIRECTORY, not BUILD_WORKING_DIRECTORY. This is for the following reasons: The TODO tag explicitly convey the intention of using BUILD_WORKSPACE_DIRECTORY for relative paths. If destdir is specified in the attribute of the pkg_install() target, interpreting it against BUILD_WORKSPACE_DIRECTORY is much stabler. That is, no matter where your current cwd is, the destdir attribute always refers to a path relative to the workspace root. For example: ``` pkg_install(name = "my_pkg_install", destdir = "out/dest") ``` ``` cd / bazel run //:my_pkg_install ``` This clearly conveys that the default destdir is /out/dest regardless of where the user runs the command. The cost is that the --destdir command line argument becomes trickier to understand. For example, if one is not familiar with pkg_install, and below the workspace root they run: ``` cd /out bazel run //:my_pkg_install -- --destdir dest ``` They may expect the destdir to be set to /out/dest; but it is in fact `/dest`. We could also interpret the target attribute & the command line argument separately (e.g. pkg_install(destdir_against_workspace)), but honestly I think that's even more confusing when they interpret relative paths differently. Please let me know if this is preferred by the maintainers. Co-authored-by: HONG Yifan --- pkg/install.bzl | 24 +++++++++++++++---- pkg/private/install.py.tpl | 47 +++++++++++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/pkg/install.bzl b/pkg/install.bzl index e7b0e424..7b415aba 100644 --- a/pkg/install.bzl +++ b/pkg/install.bzl @@ -62,6 +62,7 @@ def _pkg_install_script_impl(ctx): "{WORKSPACE_NAME}": ctx.workspace_name, # Used to annotate --help with "bazel run //path/to/your:installer" "{TARGET_LABEL}": label_str, + "{DEFAULT_DESTDIR}": ctx.attr.destdir, }, is_executable = True, ) @@ -98,6 +99,7 @@ _pkg_install_script = rule( ], doc = "Source mapping/grouping targets", ), + "destdir": attr.string(), # This is private for now -- one could perhaps imagine making this # public, but that would require more documentation of the underlying # scripts and expected interfaces. @@ -109,7 +111,7 @@ _pkg_install_script = rule( executable = True, ) -def pkg_install(name, srcs, **kwargs): +def pkg_install(name, srcs, destdir = None, **kwargs): """Create an installer script from pkg_filegroups and friends. This macro allows users to create `bazel run`nable installation scripts @@ -123,6 +125,7 @@ def pkg_install(name, srcs, **kwargs): srcs = [ # mapping/grouping targets here ], + destdir = "out/install", ) ``` @@ -153,15 +156,28 @@ def pkg_install(name, srcs, **kwargs): https://github.com/bazelbuild/rules_pkg/issues/388. Args: - name: rule name - srcs: pkg_filegroup framework mapping or grouping targets - **kwargs: common rule attributes + name: rule name + srcs: pkg_filegroup framework mapping or grouping targets + destdir: The default destination directory. + + If it is specified, this is the default destination to install + the files. It is overridable by explicitly specifying `--destdir` + in the command line or specifying the `DESTDIR` environment + variable. + + If it is not specified, `--destdir` must be set on the command line, + or the `DESTDIR` environment variable must be set. + + If this is an absolute path, it is used as-is. If this is a relative + path, it is interpreted against `BUILD_WORKSPACE_DIRECTORY`. + **kwargs: common rule attributes """ _pkg_install_script( name = name + "_install_script", srcs = srcs, + destdir = destdir, **kwargs ) diff --git a/pkg/private/install.py.tpl b/pkg/private/install.py.tpl index 004049fb..3a29f05f 100644 --- a/pkg/private/install.py.tpl +++ b/pkg/private/install.py.tpl @@ -20,6 +20,7 @@ import argparse import logging import os +import pathlib import shutil import sys @@ -182,6 +183,36 @@ class NativeInstaller(object): raise ValueError("Unrecognized entry type '{}'".format(entry.type)) +def _default_destdir(): + # If --destdir is not specified, use these values, in this order + # Use env var if specified and non-empty + env = os.getenv("DESTDIR") + if env: + return env + + # Checks if DEFAULT_DESTDIR is an empty string + target_attr = "{DEFAULT_DESTDIR}" + if target_attr: + return target_attr + + return None + + +def _resolve_destdir(path_s): + if not path_s: + raise argparse.ArgumentTypeError("destdir is not set!") + path = pathlib.Path(path_s) + if path.is_absolute(): + return path_s + build_workspace_directory = os.getenv("BUILD_WORKSPACE_DIRECTORY") + if not build_workspace_directory: + raise argparse.ArgumentTypeError(f"BUILD_WORKSPACE_DIRECTORY is not set" + f" and destdir {path} is relative. " + f"Unable to infer an absolute path.") + ret = str(pathlib.Path(build_workspace_directory) / path) + return ret + + def main(args): parser = argparse.ArgumentParser( prog="bazel run -- {TARGET_LABEL}", @@ -194,12 +225,16 @@ def main(args): help="Be silent, except for errors") # TODO(nacl): consider supporting DESTDIR=/whatever syntax, like "make # install". - # - # TODO(nacl): consider removing absolute path restriction, perhaps using - # BUILD_WORKING_DIRECTORY. - parser.add_argument('--destdir', action='store', default=os.getenv("DESTDIR"), - help="Installation root directory (defaults to DESTDIR " - "environment variable). Must be an absolute path.") + default_destdir = _default_destdir() + default_destdir_text = f" or {default_destdir}" if default_destdir else "" + parser.add_argument('--destdir', action='store', default=default_destdir, + required=default_destdir is None, + type=_resolve_destdir, + help=f"Installation root directory (defaults to DESTDIR" + f" environment variable{default_destdir_text}). " + f"Relative paths are interpreted against " + f"BUILD_WORKSPACE_DIRECTORY " + f"({os.getenv('BUILD_WORKSPACE_DIRECTORY')})") args = parser.parse_args()