Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop stripping tree artifact root name in pkg_tar and pkg_zip. #555

Merged
merged 2 commits into from
Mar 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/mappings.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ def _do_strip_prefix(path, to_strip, src_file):

if path_norm.startswith(to_strip_norm):
return path_norm[len(to_strip_norm):]
elif src_file.is_directory and (path_norm + '/') == to_strip_norm:
return ''
else:
# Avoid user surprise by failing if prefix stripping doesn't work as
# expected.
Expand Down
6 changes: 1 addition & 5 deletions pkg/private/tar/tar.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,7 @@ def _pkg_tar_impl(ctx):
for f in src_files:
d_path = dest_path(f, data_path, data_path_without_prefix)
if f.is_directory:
# Tree artifacts need a name, but the name is never really
# the important part. The likely behavior people want is
# just the content, so we strip the directory name.
dest = "/".join(d_path.split("/")[0:-1])
add_tree_artifact(content_map, dest, f, src.label)
add_tree_artifact(content_map, d_path, f, src.label)
else:
# Note: This extra remap is the bottleneck preventing this
# large block from being a utility method as shown below.
Expand Down
6 changes: 1 addition & 5 deletions pkg/private/zip/zip.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,7 @@ def _pkg_zip_impl(ctx):
for f in src.files.to_list():
d_path = dest_path(f, data_path, data_path_without_prefix)
if f.is_directory:
# Tree artifacts need a name, but the name is never really
# the important part. The likely behavior people want is
# just the content, so we strip the directory name.
dest = "/".join(d_path.split("/")[0:-1])
add_tree_artifact(content_map, dest, f, src.label)
add_tree_artifact(content_map, d_path, f, src.label)
else:
add_single_file(content_map, d_path, f, src.label)

Expand Down
30 changes: 21 additions & 9 deletions tests/tar/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"""Tests for pkg_tar."""

# buildifier: disable=bzl-visibility
load("//pkg:mappings.bzl", "pkg_files", "strip_prefix")
load("//pkg/private/tar:tar.bzl", "SUPPORTED_TAR_COMPRESSIONS", "pkg_tar")
load("//tests:my_package_name.bzl", "my_package_naming")
load("//tests/util:defs.bzl", "directory", "fake_artifact")
Expand Down Expand Up @@ -267,10 +268,29 @@ pkg_tar(
name = "test-tar-tree-artifact",
srcs = [
":generate_tree",
"//tests:loremipsum_txt",
],
package_dir = "a_tree",
)

pkg_files(
name = "tree_noroot",
srcs = [
":generate_tree",
],
strip_prefix = "generate_tree",
)

pkg_tar(
name = "test-tar-tree-artifact-noroot",
srcs = [
":tree_noroot",
"//tests:loremipsum_txt",
],
package_dir = "a_tree",
)


py_test(
name = "pkg_tar_test",
size = "medium",
Expand All @@ -291,6 +311,7 @@ py_test(
":test-tar-long-filename",
":test-tar-repackaging-long-filename.tar",
":test-tar-tree-artifact",
":test-tar-tree-artifact-noroot",
":test-tar-with-runfiles",
"//tests:testdata/tar_test.tar",
"//tests:testdata/tar_test.tar.bz2",
Expand Down Expand Up @@ -324,15 +345,6 @@ test_suite(
tests = [":windows_tests"],
)

# Used within experimental/tests/ for external repo genpkg tests
filegroup(
name = "loremipsum_txt",
srcs = [
"//tests:testdata/loremipsum.txt",
],
visibility = ["//visibility:public"],
)

pkg_tar(
name = "test-tar-compression-from-extension-targz",
srcs = [
Expand Down
36 changes: 24 additions & 12 deletions tests/tar/pkg_tar_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
class PkgTarTest(unittest.TestCase):
"""Testing for pkg_tar rule."""

def assertTarFileContent(self, file_name, content):
def assertTarFileContent(self, file_name, content, verbose=False):
"""Assert that tarfile contains exactly the entry described by `content`.

Args:
Expand All @@ -44,7 +44,8 @@ def assertTarFileContent(self, file_name, content):
with tarfile.open(file_path, 'r:*') as f:
i = 0
for info in f:
got.append(info.name)
if verbose:
print(' >> from tar file:', info.name)
error_msg = 'Extraneous file at end of archive %s: %s' % (
file_path,
info.name
Expand All @@ -62,7 +63,7 @@ def assertTarFileContent(self, file_name, content):
'%s in archive %s does' % (info.name, file_path),
'not match expected value `%s`' % v
])
# self.assertEqual(value, v, error_msg)
self.assertEqual(value, v, error_msg)
if value != v:
print(error_msg)
i += 1
Expand Down Expand Up @@ -207,18 +208,29 @@ def test_tar_with_tree_artifact(self):

content = [
{'name': 'a_tree', 'isdir': True},
{'name': 'a_tree/a', 'isdir': True},
{'name': 'a_tree/a/a'},
{'name': 'a_tree/a/b', 'isdir': True},
{'name': 'a_tree/a/b/c'},
{'name': 'a_tree/b', 'isdir': True},
{'name': 'a_tree/b/c', 'isdir': True},
{'name': 'a_tree/b/c/d'},
{'name': 'a_tree/b/d'},
{'name': 'a_tree/b/e'},
{'name': 'a_tree/generate_tree', 'isdir': True},
{'name': 'a_tree/generate_tree/a', 'isdir': True},
{'name': 'a_tree/generate_tree/a/a'},
{'name': 'a_tree/generate_tree/a/b', 'isdir': True},
{'name': 'a_tree/generate_tree/a/b/c'},
{'name': 'a_tree/generate_tree/b', 'isdir': True},
{'name': 'a_tree/generate_tree/b/c', 'isdir': True},
{'name': 'a_tree/generate_tree/b/c/d'},
{'name': 'a_tree/generate_tree/b/d'},
{'name': 'a_tree/generate_tree/b/e'},
{'name': 'a_tree/loremipsum.txt'},
]
self.assertTarFileContent('test-tar-tree-artifact.tar', content)

# Now test against the tree artifact with the dir name stripped.
noroot_content = []
for c in content[1:]: # one less level in tree. Skip first element.
nc = dict(c)
nc['name'] = c['name'].replace('/generate_tree', '')
noroot_content.append(nc)
self.assertTarFileContent('test-tar-tree-artifact-noroot.tar',
noroot_content)

def test_tar_with_runfiles(self):
content = [
{'name': 'BUILD' },
Expand Down
2 changes: 1 addition & 1 deletion version.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
# limitations under the License.
"""The version of rules_pkg."""

version = "0.6.0"
version = "0.7.0"