Codify how to remove TreeArtifact base directories in pkg_files (#330)

We often have TreeArtifacts (directory artifacts) that are built to be installed
directly onto the filesystem in packages, but they sometimes need to be renamed.
As of #319, there are two ways to accomplish this:

1) Rename the TreeArtifact to the destination
2) Rename the TreeArtifact to nothing

I believe that (2) is clearer, as the `prefix` provided to `pkg_files` stands
out more than an entry in `renames`.  This change codifies a workaround we have
tried out internally -- renaming to "nothing".  While "nothing" itself could be
a bit misleading, we can instead represent the desire for this via the constant
`REMOVE_BASE_DIRECTORY`, represented as a non-printable string. (1) is still
allowed.

This change does not attempt to do anything related to updating paths inside
TreeArtifacts.  This will be attempted in a follow-on change.

Tests and documentation updates are also provided.

This change also moves the common analysis test boilerplate to its own location
in `pkg/tests/util/defs.bzl`, where it will be more generally accessible in
future changes.

Partially resolves #269.

pkg/experimental/augment_rpm_files_install.py

@@ -56,17 +56,14 @@
 
     for d in dir_data:
         # d is a dict, d["src"] is the TreeArtifact directory to walk.
-        print(d["src"])
         for root, dirs, files in os.walk(d["src"]):
             # "root" is the current directory we're walking through.  This
             # computes the path the source location (the TreeArtifact root) --
             # the desired install location relative to the user-provided install
             # destination.
             path_relative_to_install_dest = os.path.relpath(root, start=d["src"])
-            print(path_relative_to_install_dest)
 
             for f in files:
-                print(" ", d["dest"], path_relative_to_install_dest, f)
                 full_dest = os.path.join(d["dest"], path_relative_to_install_dest, f)
                 dir_install_script_segments.append(_INSTALL_FILE_STANZA_FMT.format(
                     os.path.join(root, f),

pkg/experimental/tests/rpm/analysis_tests.bzl

@@ -24,39 +24,7 @@ load(
     "pkg_mklink",
 )
 load("//:providers.bzl", "PackageArtifactInfo", "PackageVariablesInfo")
-
-# Generic negative test boilerplate
-#
-# TODO: create an internal test library containing this function, and maybe the second one too
-def _generic_neg_test_impl(ctx):
-    env = analysistest.begin(ctx)
-
-    asserts.expect_failure(env, ctx.attr.reason)
-
-    return analysistest.end(env)
-
-generic_neg_test = analysistest.make(
-    _generic_neg_test_impl,
-    attrs = {
-        "reason": attr.string(
-            default = "",
-        ),
-    },
-    expect_failure = True,
-)
-
-def _generic_base_case_test_impl(ctx):
-    env = analysistest.begin(ctx)
-
-    # Nothing here intentionally, this is simply an attempt to verify successful
-    # analysis.
-
-    return analysistest.end(env)
-
-generic_base_case_test = analysistest.make(
-    _generic_base_case_test_impl,
-    attrs = {},
-)
+load("//tests/util:defs.bzl", "directory", "generic_base_case_test", "generic_negative_test")
 
 def _declare_pkg_rpm(name, srcs_ungrouped, tags = None, **kwargs):
     pfg_name = "{}_pfg".format(name)
@@ -89,7 +57,7 @@ def _declare_conflicts_test(name, srcs, **kwargs):
         tags = ["manual"],
     )
 
-    generic_neg_test(
+    generic_negative_test(
         name = name,
         target_under_test = ":" + rpm_name,
     )

pkg/experimental/tests/rpm/tree_artifacts/BUILD

@@ -12,15 +12,16 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+load("//tests/util:defs.bzl", "directory")
 load(
     "@rules_pkg//:mappings.bzl",
+    "REMOVE_BASE_DIRECTORY",
     "pkg_filegroup",
     "pkg_files",
     "strip_prefix",
 )
 load("@rules_pkg//experimental:rpm.bzl", "pkg_rpm")
-load("@rules_python//python:defs.bzl", "py_binary", "py_test")
-load(":rules.bzl", "directory")
+load("@rules_python//python:defs.bzl", "py_test")
 
 ############################################################################
 # Test handling of directory outputs
@@ -183,10 +184,54 @@ pkg_filegroup(
 )
 
 ############################################################################
-# Utility (used by the "directory" rule)
+# Test that renaming TreeArtifacts to nothing results in the files being dropped
+# in their prefix.
 ############################################################################
+py_test(
+    name = "treeartifact_ops",
+    srcs = ["rpm_treeartifact_ops_test.py"],
+    data = [":treeartifact_ops_rpm_test_data"],
+    main = "rpm_treeartifact_ops_test.py",
+    tags = [
+        "no_windows",  # Windows doesn't have rpm(8) or rpmbuild(8)
+    ],
+    deps = [
+        "//experimental/tests/rpm:rpm_util",
+        "@rules_python//python/runfiles",
+    ],
+)
 
-py_binary(
-    name = "create_directory_with_contents",
-    srcs = ["create_directory_with_contents.py"],
+sh_library(
+    name = "treeartifact_ops_rpm_test_data",
+    testonly = True,
+    srcs = [":treeartifact_ops_rpm"],
+)
+
+pkg_rpm(
+    name = "treeartifact_ops_rpm",
+    srcs = [
+        ":treeartifact_ops_pfg",
+    ],
+    architecture = "noarch",
+    description = """pkg_rpm test rpm description""",
+    license = "Apache 2.0",
+    release = "2222",
+    spec_template = "//experimental/tests/rpm:template-test.spec.in",
+    summary = "pkg_rpm test rpm summary",
+    version = "1.1.1",
+)
+
+pkg_filegroup(
+    name = "treeartifact_ops_pfg",
+    srcs = [
+        ":stripped_treeartifact_pf",
+    ],
+)
+
+pkg_files(
+    name = "stripped_treeartifact_pf",
+    srcs = [
+        ":test_dir1",
+    ],
+    renames = {":test_dir1": REMOVE_BASE_DIRECTORY},
 )

pkg/experimental/tests/rpm/tree_artifacts/rpm_treeartifact_ops_test.py

@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+
+# Copyright 2021 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest
+import csv
+import io
+import os
+import rpm_util
+from rules_python.python.runfiles import runfiles
+
+EXPECTED_RPM_MANIFEST_CSV = """
+path,digest,user,group,mode,fflags,symlink
+/a,dc35e5df50b75e25610e1aaaa29edaa4,root,root,100644,,
+/b,dc35e5df50b75e25610e1aaaa29edaa4,root,root,100644,,
+/subdir/c,dc35e5df50b75e25610e1aaaa29edaa4,root,root,100644,,
+/subdir/d,dc35e5df50b75e25610e1aaaa29edaa4,root,root,100644,,
+""".strip()
+
+
+class PkgRpmCompManifest(unittest.TestCase):
+    def setUp(self):
+        self.runfiles = runfiles.Create()
+        self.maxDiff = None
+        # Allow for parameterization of this test based on the desired RPM to
+        # test.
+        self.rpm_path = self.runfiles.Rlocation(os.path.join(
+            os.environ["TEST_WORKSPACE"],
+            "experimental", "tests", "rpm", "tree_artifacts",
+            # The object behind os.environ is not a dict, and thus doesn't have
+            # the "getdefault()" we'd otherwise use here.
+            os.environ["TEST_RPM"] if "TEST_RPM" in os.environ else "treeartifact_ops_rpm.rpm",
+        ))
+
+    def test_contents_match(self):
+        sio = io.StringIO(EXPECTED_RPM_MANIFEST_CSV)
+        manifest_reader = csv.DictReader(sio)
+        manifest_specs = {r['path']: r for r in manifest_reader}
+
+        rpm_specs = rpm_util.read_rpm_filedata(self.rpm_path)
+
+        self.assertDictEqual(manifest_specs, rpm_specs)
+
+
+if __name__ == "__main__":
+    unittest.main()

pkg/mappings.bzl

@@ -24,14 +24,18 @@ the following:
 - `pkg_filegroup` creates groupings of above to add to packages
 
 Rules that actually make use of the outputs of the above rules are not specified
-here.  TODO(nacl): implement one.
+here.
 """
 
 load("@bazel_skylib//lib:paths.bzl", "paths")
 load("//:providers.bzl", "PackageDirsInfo", "PackageFilegroupInfo", "PackageFilesInfo", "PackageSymlinkInfo")
 
+# TODO(#333): strip_prefix module functions should produce unique outputs.  In
+# particular, this one and `_sp_from_pkg` can overlap.
 _PKGFILEGROUP_STRIP_ALL = "."
 
+REMOVE_BASE_DIRECTORY = "\0"
+
 def _sp_files_only():
     return _PKGFILEGROUP_STRIP_ALL
 
@@ -124,7 +128,13 @@ def _do_strip_prefix(path, to_strip, src_file):
         #
         # We already leave enough breadcrumbs, so if File.owner() returns None,
         # this won't be a problem.
-        fail("Could not strip prefix '{}' from file {} ({})".format(to_strip, str(src_file), str(src_file.owner)))
+        failmsg = "Could not strip prefix '{}' from file {} ({})".format(to_strip, str(src_file), str(src_file.owner))
+        if src_file.is_directory:
+            failmsg += """\n\nNOTE: prefix stripping does not operate within TreeArtifacts (directory outputs)
+
+To strip the directory named by the TreeArtifact itself, see documentation for the `renames` attribute.
+"""
+        fail(failmsg)
 
 # The below routines make use of some path checking magic that may difficult to
 # understand out of the box.  This following table may be helpful to demonstrate
@@ -233,7 +243,19 @@ def _pkg_files_impl(ctx):
                 "File remapping from {0} to {1} is invalid: {0} is not provided to this rule or was excluded".format(rename_src, rename_dest),
                 "renames",
             )
-        src_dest_paths_map[src_file] = paths.join(ctx.attr.prefix, rename_dest)
+
+        if rename_dest == REMOVE_BASE_DIRECTORY:
+            if not src_file.is_directory:
+                fail(
+                    "REMOVE_BASE_DIRECTORY as a renaming target for non-directories is disallowed.",
+                    "renames",
+                )
+            # REMOVE_BASE_DIRECTORY results in the contents being dropped into
+            # place directly in the prefix path.
+            src_dest_paths_map[src_file] = ctx.attr.prefix
+
+        else:
+            src_dest_paths_map[src_file] = paths.join(ctx.attr.prefix, rename_dest)
 
     # At this point, we have a fully valid src -> dest mapping in src_dest_paths_map.
     #
@@ -354,13 +376,28 @@ pkg_files = rule(
             the `prefix`, ignoring whatever value was provided for
             `strip_prefix`.
 
+            If the key refers to a TreeArtifact (directory output), you may
+            specify the constant `REMOVE_BASE_DIRECTORY` as the value, which
+            will result in all containing files and directories being installed
+            relative to the otherwise install prefix (via the `prefix` and
+            `strip_prefix` attributes), not the directory name.
+            
             The following keys are rejected:
 
             - Any label that expands to more than one file (mappings must be
               one-to-one).
 
             - Any label or file that was either not provided or explicitly
               `exclude`d.
+            
+            The following values result in undefined behavior:
+
+            - "" (the empty string)
+
+            - "."
+            
+            - Anything containing ".."
+
             """,
             default = {},
             allow_files = True,