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 <workspace_root>/<some_subdir>
bazel run //:my_pkg_install
```
This clearly conveys that the default destdir is
<workspace_root>/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 <workspace_root>/out
bazel run //:my_pkg_install -- --destdir dest
```

They may expect the destdir to be set to <workspace_root>/out/dest; but
it is in fact `<workspace_root>/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 <elsk@google.com>

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
     )
 

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()