When pkg_tar.prefix_dir == base of symlink path, don't double-dip. (#749)

* Correct for case where tar has prefix_dir and files/symlinks, but the
user has already mapped the prefix_dir into those paths. Under the old
behavior, prefix_dir was (incorrectly) not added to symlinks, so some
users made the desired prefix part of the link.  Protecting them
against the double inclusion of the prefix is probably the least
surprising behavior.

Add tests for this, which required improving verify_archive_test.

* make buildifier happy

* do not run the symlink test on windows

* huh? why are symlinks in a tree failing on CI?

Author: aiuto

.bazelci/tests.yml

@@ -33,6 +33,10 @@ default_tests_with_rpm: &default_tests_with_rpm
     - "//toolchains/..."
 
 win_tests: &win_tests
+  test_flags:
+    - "--test_tag_filters=-nowindows"
+  build_flags:
+    - "--build_tag_filters=-nowindows"
   test_targets:
     - "//pkg/..."
     - "//tests/..."

docs/latest.md

@@ -259,8 +259,8 @@ Rules for making .tar files.
 ## pkg_tar
 
 <pre>
-pkg_tar(<a href="#pkg_tar-name">name</a>, <a href="#pkg_tar-build_tar">build_tar</a>, <a href="#pkg_tar-compressor">compressor</a>, <a href="#pkg_tar-compressor_args">compressor_args</a>, <a href="#pkg_tar-deps">deps</a>, <a href="#pkg_tar-empty_dirs">empty_dirs</a>, <a href="#pkg_tar-empty_files">empty_files</a>, <a href="#pkg_tar-extension">extension</a>,
-             <a href="#pkg_tar-files">files</a>, <a href="#pkg_tar-include_runfiles">include_runfiles</a>, <a href="#pkg_tar-mode">mode</a>, <a href="#pkg_tar-modes">modes</a>, <a href="#pkg_tar-mtime">mtime</a>, <a href="#pkg_tar-out">out</a>, <a href="#pkg_tar-owner">owner</a>, <a href="#pkg_tar-ownername">ownername</a>, <a href="#pkg_tar-ownernames">ownernames</a>, <a href="#pkg_tar-owners">owners</a>,
+pkg_tar(<a href="#pkg_tar-name">name</a>, <a href="#pkg_tar-compressor">compressor</a>, <a href="#pkg_tar-compressor_args">compressor_args</a>, <a href="#pkg_tar-deps">deps</a>, <a href="#pkg_tar-empty_dirs">empty_dirs</a>, <a href="#pkg_tar-empty_files">empty_files</a>, <a href="#pkg_tar-extension">extension</a>, <a href="#pkg_tar-files">files</a>,
+             <a href="#pkg_tar-include_runfiles">include_runfiles</a>, <a href="#pkg_tar-mode">mode</a>, <a href="#pkg_tar-modes">modes</a>, <a href="#pkg_tar-mtime">mtime</a>, <a href="#pkg_tar-out">out</a>, <a href="#pkg_tar-owner">owner</a>, <a href="#pkg_tar-ownername">ownername</a>, <a href="#pkg_tar-ownernames">ownernames</a>, <a href="#pkg_tar-owners">owners</a>,
              <a href="#pkg_tar-package_dir">package_dir</a>, <a href="#pkg_tar-package_dir_file">package_dir_file</a>, <a href="#pkg_tar-package_file_name">package_file_name</a>, <a href="#pkg_tar-package_variables">package_variables</a>, <a href="#pkg_tar-portable_mtime">portable_mtime</a>,
              <a href="#pkg_tar-private_stamp_detect">private_stamp_detect</a>, <a href="#pkg_tar-remap_paths">remap_paths</a>, <a href="#pkg_tar-srcs">srcs</a>, <a href="#pkg_tar-stamp">stamp</a>, <a href="#pkg_tar-strip_prefix">strip_prefix</a>, <a href="#pkg_tar-symlinks">symlinks</a>)
 </pre>
@@ -273,31 +273,30 @@ pkg_tar(<a href="#pkg_tar-name">name</a>, <a href="#pkg_tar-build_tar">build_tar
 | Name  | Description | Type | Mandatory | Default |
 | :------------- | :------------- | :------------- | :------------- | :------------- |
 | <a id="pkg_tar-name"></a>name |  A unique name for this target.   | <a href="https://bazel.build/docs/build-ref.html#name">Name</a> | required |  |
-| <a id="pkg_tar-build_tar"></a>build_tar |  -   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | //pkg/private/tar:build_tar |
-| <a id="pkg_tar-compressor"></a>compressor |  -   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
-| <a id="pkg_tar-compressor_args"></a>compressor_args |  -   | String | optional | "" |
-| <a id="pkg_tar-deps"></a>deps |  -   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
+| <a id="pkg_tar-compressor"></a>compressor |  External tool which can compress the archive.   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
+| <a id="pkg_tar-compressor_args"></a>compressor_args |  Arg list for <code>compressor</code>.   | String | optional | "" |
+| <a id="pkg_tar-deps"></a>deps |  tar files which will be unpacked and repacked into the archive.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="pkg_tar-empty_dirs"></a>empty_dirs |  -   | List of strings | optional | [] |
 | <a id="pkg_tar-empty_files"></a>empty_files |  -   | List of strings | optional | [] |
 | <a id="pkg_tar-extension"></a>extension |  -   | String | optional | "tar" |
-| <a id="pkg_tar-files"></a>files |  -   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: Label -> String</a> | optional | {} |
+| <a id="pkg_tar-files"></a>files |  Obsolete. Do not use.   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: Label -> String</a> | optional | {} |
 | <a id="pkg_tar-include_runfiles"></a>include_runfiles |  -   | Boolean | optional | False |
 | <a id="pkg_tar-mode"></a>mode |  -   | String | optional | "0555" |
 | <a id="pkg_tar-modes"></a>modes |  -   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
 | <a id="pkg_tar-mtime"></a>mtime |  -   | Integer | optional | -1 |
 | <a id="pkg_tar-out"></a>out |  -   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | required |  |
-| <a id="pkg_tar-owner"></a>owner |  -   | String | optional | "0.0" |
+| <a id="pkg_tar-owner"></a>owner |  Default numeric owner.group to apply to files when not set via pkg_attribures.   | String | optional | "0.0" |
 | <a id="pkg_tar-ownername"></a>ownername |  -   | String | optional | "." |
 | <a id="pkg_tar-ownernames"></a>ownernames |  -   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
 | <a id="pkg_tar-owners"></a>owners |  -   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
-| <a id="pkg_tar-package_dir"></a>package_dir |  Prefix to be prepend to all paths written. The name may contain variables, same as [package_file_name](#package_file_name)   | String | optional | "" |
+| <a id="pkg_tar-package_dir"></a>package_dir |  Prefix to be prepend to all paths written.<br><br>            This is applied as a final step, while writing to the archive.             Any other attributes (e.g. symlinks) which specify a path, must do so relative to package_dir.             The value may contain variables. See [package_file_name](#package_file_name) for examples.   | String | optional | "" |
 | <a id="pkg_tar-package_dir_file"></a>package_dir_file |  -   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
 | <a id="pkg_tar-package_file_name"></a>package_file_name |  See [Common Attributes](#package_file_name)   | String | optional | "" |
 | <a id="pkg_tar-package_variables"></a>package_variables |  See [Common Attributes](#package_variables)   | <a href="https://bazel.build/docs/build-ref.html#labels">Label</a> | optional | None |
 | <a id="pkg_tar-portable_mtime"></a>portable_mtime |  -   | Boolean | optional | True |
 | <a id="pkg_tar-private_stamp_detect"></a>private_stamp_detect |  -   | Boolean | optional | False |
 | <a id="pkg_tar-remap_paths"></a>remap_paths |  -   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
-| <a id="pkg_tar-srcs"></a>srcs |  -   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
+| <a id="pkg_tar-srcs"></a>srcs |  Inputs which will become part of the tar archive.   | <a href="https://bazel.build/docs/build-ref.html#labels">List of labels</a> | optional | [] |
 | <a id="pkg_tar-stamp"></a>stamp |  Enable file time stamping.  Possible values: <li>stamp = 1: Use the time of the build as the modification time of each file in the archive. <li>stamp = 0: Use an "epoch" time for the modification time of each file. This gives good build result caching. <li>stamp = -1: Control the chosen modification time using the --[no]stamp flag. <div class="since"><i>Since 0.5.0</i></div>   | Integer | optional | 0 |
 | <a id="pkg_tar-strip_prefix"></a>strip_prefix |  (note: Use strip_prefix = "." to strip path to the package but preserve relative paths of sub directories beneath the package.)   | String | optional | "" |
 | <a id="pkg_tar-symlinks"></a>symlinks |  -   | <a href="https://bazel.build/docs/skylark/lib/dict.html">Dictionary: String -> String</a> | optional | {} |
@@ -583,6 +582,9 @@ pkg_mklink(<a href="#pkg_mklink-name">name</a>, <a href="#pkg_mklink-link_name">
 
 Create a symlink.
 
+Wraps [pkg_mklink_impl](#pkg_mklink_impl)
+
+
 **PARAMETERS**
 
 

pkg/private/tar/build_tar.py

@@ -70,7 +70,14 @@ def normalize_path(self, path: str) -> str:
     # No path should ever come in with slashs on either end, but protect
     # against that anyway.
     dest = dest.strip('/')
-    if self.directory:
+    # This prevents a potential problem for users with both a prefix_dir and
+    # symlinks that also repeat the prefix_dir. The old behavior was that we
+    # would get just the symlink path. Now we are prefixing with the prefix,
+    # so you get the file in the wrong place.
+    # We silently de-dup that. If people come up with a real use case for
+    # the /a/b/a/b/rest... output we can start an issue and come up with a
+    # solution at that time.
+    if self.directory and not dest.startswith(self.directory):
       dest = self.directory + dest
     return dest
 

pkg/private/tar/tar.bzl

@@ -244,7 +244,7 @@ def _pkg_tar_impl(ctx):
         # or this OutputGroup might be totally removed.
         # Depend on it at your own risk!
         OutputGroupInfo(
-            manifest = [manifest_file], 
+            manifest = [manifest_file],
         ),
     ]
 
@@ -253,21 +253,37 @@ pkg_tar_impl = rule(
     implementation = _pkg_tar_impl,
     attrs = {
         "strip_prefix": attr.string(
-            doc = """(note: Use strip_prefix = "." to strip path to the package but preserve relative paths of sub directories beneath the package.)"""
+            doc = """(note: Use strip_prefix = "." to strip path to the package but preserve relative paths of sub directories beneath the package.)""",
         ),
         "package_dir": attr.string(
             doc = """Prefix to be prepend to all paths written.
-The name may contain variables, same as [package_file_name](#package_file_name)""",
+
+            This is applied as a final step, while writing to the archive.
+            Any other attributes (e.g. symlinks) which specify a path, must do so relative to package_dir.
+            The value may contain variables. See [package_file_name](#package_file_name) for examples.
+            """,
         ),
         "package_dir_file": attr.label(allow_single_file = True),
-        "deps": attr.label_list(allow_files = tar_filetype),
-        "srcs": attr.label_list(allow_files = True),
-        "files": attr.label_keyed_string_dict(allow_files = True),
+        "deps": attr.label_list(
+            doc = """tar files which will be unpacked and repacked into the archive.""",
+            allow_files = tar_filetype,
+        ),
+        "srcs": attr.label_list(
+            doc = """Inputs which will become part of the tar archive.""",
+            allow_files = True,
+        ),
+        "files": attr.label_keyed_string_dict(
+            doc = """Obsolete. Do not use.""",
+            allow_files = True,
+        ),
         "mode": attr.string(default = "0555"),
         "modes": attr.string_dict(),
         "mtime": attr.int(default = _DEFAULT_MTIME),
         "portable_mtime": attr.bool(default = True),
-        "owner": attr.string(default = "0.0"),
+        "owner": attr.string(
+            doc = """Default numeric owner.group to apply to files when not set via pkg_attribures.""",
+            default = "0.0",
+        ),
         "ownername": attr.string(default = "."),
         "owners": attr.string_dict(),
         "ownernames": attr.string_dict(),
@@ -277,8 +293,14 @@ The name may contain variables, same as [package_file_name](#package_file_name)"
         "include_runfiles": attr.bool(),
         "empty_dirs": attr.string_list(),
         "remap_paths": attr.string_dict(),
-        "compressor": attr.label(executable = True, cfg = "exec"),
-        "compressor_args": attr.string(),
+        "compressor": attr.label(
+            doc = """External tool which can compress the archive.""",
+            executable = True,
+            cfg = "exec",
+        ),
+        "compressor_args": attr.string(
+            doc = """Arg list for `compressor`.""",
+        ),
 
         # Common attributes
         "out": attr.output(mandatory = True),
@@ -323,6 +345,7 @@ def pkg_tar(name, **kwargs):
 
     @wraps(pkg_tar_impl)
     """
+
     # Compatibility with older versions of pkg_tar that define files as
     # a flat list of labels.
     if "srcs" not in kwargs:

pkg/verify_archive.bzl

@@ -23,7 +23,6 @@ The execution time is O(# expected patterns * size of archive).
 
 load("@rules_python//python:defs.bzl", "py_test")
 
-
 def _gen_verify_archive_test_main_impl(ctx):
     ctx.actions.expand_template(
         template = ctx.file._template,
@@ -38,6 +37,7 @@ def _gen_verify_archive_test_main_impl(ctx):
             "${MUST_NOT_CONTAIN_REGEX}": str(ctx.attr.must_not_contain_regex),
             "${MIN_SIZE}": str(ctx.attr.min_size),
             "${MAX_SIZE}": str(ctx.attr.max_size),
+            "${VERIFY_LINKS}": str(ctx.attr.verify_links),
         },
     )
     return [
@@ -55,7 +55,6 @@ _gen_verify_archive_test_main = rule(
             allow_single_file = True,
             mandatory = True,
         ),
-
         "must_contain": attr.string_list(
             doc = "List of paths which all must appear in the archive.",
         ),
@@ -69,10 +68,13 @@ _gen_verify_archive_test_main = rule(
             doc = """List of regexes that must not be in the archive.""",
         ),
         "min_size": attr.int(
-            doc = """Minimum number of entries in the archive."""
+            doc = """Minimum number of entries in the archive.""",
         ),
         "max_size": attr.int(
-            doc = """Maximum number of entries in the archive."""
+            doc = """Maximum number of entries in the archive.""",
+        ),
+        "verify_links": attr.string_dict(
+            doc = """Dict keyed by paths which must appear, and be symlinks to their values.""",
         ),
 
         # Implicit dependencies.
@@ -83,10 +85,17 @@ _gen_verify_archive_test_main = rule(
     },
 )
 
-def verify_archive_test(name, target,
-                        must_contain=None, must_contain_regex=None,
-                        must_not_contain=None, must_not_contain_regex=None,
-                        min_size=1, max_size=-1):
+def verify_archive_test(
+        name,
+        target,
+        must_contain = None,
+        must_contain_regex = None,
+        must_not_contain = None,
+        must_not_contain_regex = None,
+        min_size = 1,
+        max_size = -1,
+        tags = None,
+        verify_links = None):
     """Tests that an archive contains specific file patterns.
 
     This test is used to verify that an archive contains the expected content.
@@ -99,19 +108,23 @@ def verify_archive_test(name, target,
       must_not_contain_regex: A list of path regexes which must not appear in the archive.
       min_size: The minimum number of entries which must be in the archive.
       max_size: The maximum number of entries which must be in the archive.
+      tags: standard meaning
+      verify_links: Dict keyed by paths which must appear, and be symlinks to their values.
     """
-    test_src = name + "__internal_main.py" 
+    test_src = name + "__internal_main.py"
     _gen_verify_archive_test_main(
         name = name + "_internal_main",
         target = target,
-        test_name = name.replace('-', '_') + "Test",
+        test_name = name.replace("-", "_") + "Test",
         out = test_src,
         must_contain = must_contain,
         must_contain_regex = must_contain_regex,
         must_not_contain = must_not_contain,
         must_not_contain_regex = must_not_contain_regex,
         min_size = min_size,
         max_size = max_size,
+        tags = tags,
+        verify_links = verify_links,
     )
     py_test(
         name = name,

pkg/verify_archive_test_main.py.tpl

@@ -36,10 +36,13 @@ class VerifyArchiveTest(unittest.TestCase):
 
   def load_tar(self, path):
     self.paths = []
+    self.links = {}
     with tarfile.open(path, 'r:*') as f:
       i = 0
       for info in f:
         self.paths.append(info.name)
+        if info.linkname:
+          self.links[info.name] = info.linkname
 
   def assertMinSize(self, min_size):
     """Check that the archive contains at least min_size entries.
@@ -60,6 +63,8 @@ class VerifyArchiveTest(unittest.TestCase):
     Args:
         max_size: The maximum number of targets we expect.
     """
+    if max_size < 0:
+      return
     actual_size = len(self.paths)
     self.assertLessEqual(
         len(self.paths),
@@ -100,6 +105,14 @@ class VerifyArchiveTest(unittest.TestCase):
         if r_comp.match(path):
           self.fail('Found disallowed pattern (%s) in the archive' % pattern)
 
+  def verify_links(self, verify_links):
+    for link, target in verify_links.items():
+      if link not in self.paths:
+        self.fail('Required link (%s) is not in the archive' % link)
+      if self.links[link] != target:
+        self.fail('link (%s) points to the wrong place. Expected (%s) got (%s)' %
+            (link, target, self.links[link]))
+
 
 class ${TEST_NAME}(VerifyArchiveTest):
 
@@ -125,6 +138,9 @@ class ${TEST_NAME}(VerifyArchiveTest):
   def test_must_not_contain(self):
     self.check_must_not_contain_regex(${MUST_NOT_CONTAIN_REGEX})
 
+  def test_verify_links(self):
+    self.verify_links(${VERIFY_LINKS})
+
 
 if __name__ == '__main__':
   unittest.main()