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.

Sign up for GitHub

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

pkg_tar: load attributes for TreeArtifacts #789

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

pkg_tar: load attributes for TreeArtifacts #789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 34 additions & 17 deletions pkg/private/tar/build_tar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ def add_deb(self, deb):
self.add_tar(tmpfile[1])
os.remove(tmpfile[1])

def add_tree(self, tree_top, destpath, mode=None, ids=None, names=None):
def add_tree(self, tree_top, destpath, file_attributes, mode=None, ids=None, names=None):
"""Add a tree artifact to the tar file.

Args:
Expand Down Expand Up @@ -273,31 +273,48 @@ def add_tree(self, tree_top, destpath, mode=None, ids=None, names=None):
to_write[dest_dir + file] = content_path

for path in sorted(to_write.keys()):
if file_attributes:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this always true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, at the moment; I copied how file_attributes was used elsewhere in the script because I didn't understand the context about why the script was already checking for it, and I figured it was safer to do it the same way.

attrs = file_attributes(path, with_default_mode=False)
use_ids = attrs.get("ids")
use_names = attrs.get("names")
# in this case, use_mode will be None and mode (as passed into this
# function) will be the default_mode.
use_mode = attrs.get("mode")
else:
use_ids = ids
use_names = names
use_mode = mode

content_path = to_write[path]
if not content_path:
# This is an intermediate directory. Bazel has no API to specify modes
# for this, so the least surprising thing we can do is make it the
# canonical rwxr-xr-x
# canonical rwxr-xr-x unless we've gotten a custom override
if use_mode is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like just having 0o755 be the default mode would eliminate the need for the test here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this function now uses the same global default that every other file uses, the default without the conditional block would actually be 555 which would be a breaking change.

Before my change, intermediate dirs would always be 0755; after my change, intermediate dirs would default to 0755 (regardless of the global default) unless the user passed modes: {"the/intermediate/dir": othermode} to pkg_tar in which case it would be honored. (I updated the test to reflect this ability).

# No custom override, so fall back to the old behavior of 0o755
use_mode = 0o755

self.add_empty_file(
path,
mode=0o755,
ids=ids,
names=names,
mode=use_mode,
ids=use_ids,
names=use_names,
kind=tarfile.DIRTYPE)
else:
# If mode is unspecified, derive the mode from the file's mode.
if mode is None:
f_mode = 0o755 if os.access(content_path, os.X_OK) else 0o644
else:
f_mode = mode
if use_mode is None:
use_mode = mode
if use_mode is None:
use_mode = 0o755 if os.access(content_path, os.X_OK) else 0o644

self.tarfile.add_file(
path,
file_content=content_path,
mode=f_mode,
uid=ids[0],
gid=ids[1],
uname=names[0],
gname=names[1])
mode=use_mode,
uid=use_ids[0],
gid=use_ids[1],
uname=use_names[0],
gname=use_names[1])

def add_manifest_entry(self, entry, file_attributes):
# Use the pkg_tar mode/owner remapping as a fallback
Expand Down Expand Up @@ -325,7 +342,7 @@ def add_manifest_entry(self, entry, file_attributes):
elif entry.type == manifest.ENTRY_IS_DIR:
self.add_empty_dir(self.normalize_path(entry.dest), **attrs)
elif entry.type == manifest.ENTRY_IS_TREE:
self.add_tree(entry.src, entry.dest, **attrs)
self.add_tree(entry.src, entry.dest, file_attributes, **attrs)
elif entry.type == manifest.ENTRY_IS_EMPTY_FILE:
self.add_empty_file(self.normalize_path(entry.dest), **attrs)
else:
Expand Down Expand Up @@ -434,11 +451,11 @@ def main():
compressor = options.compressor,
default_mtime=default_mtime) as output:

def file_attributes(filename):
def file_attributes(filename, with_default_mode=True):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is with_default_mode needed? Having it, and only for mode rather than the rest makes this method a little more complex to understand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without with_default_mode, I can't tell the difference between a value that's intentionally set and a value that's falling back to the default. This is important because file_attributes falls back to the default mode (as passed by the end-user caller); it's pretty common to set the default mode to something like 0644, which is a fine default for files but a terrible default for directories. with_default_mode allows me to have different defaults for directories and files (without a breaking change)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said "without a breaking change" but I guess I should note that there's a non-zero chance of a breaking change in this; before, if someone set modes to a file or a directory in a TreeArtifact, it would have been silently ignored before and now it would be honored. It's somewhat unlikely that anyone would actually be bitten by this (I noticed the bug when I tried to do exactly this, and noticed it didn't work) but it'd probably need to be called out.

if filename.startswith('/'):
filename = filename[1:]
return {
'mode': mode_map.get(filename, default_mode),
'mode': mode_map.get(filename, default_mode if with_default_mode else None),
'ids': ids_map.get(filename, default_ids),
'names': names_map.get(filename, default_ownername),
}
Expand Down
6 changes: 6 additions & 0 deletions tests/tar/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,9 @@ pkg_tar(
":generate_tree",
"//tests:loremipsum_txt",
],
modes = {
"a_tree/generate_tree/b/c": "0777",
},
package_dir = "a_tree",
)

Expand All @@ -318,6 +321,9 @@ pkg_tar(
":tree_noroot",
"//tests:loremipsum_txt",
],
modes = {
"a_tree/b/c": "0777",
},
package_dir = "a_tree",
)

Expand Down
2 changes: 1 addition & 1 deletion tests/tar/pkg_tar_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ def test_tar_with_tree_artifact(self):
{'name': 'a_tree/generate_tree/a/b', 'isdir': True, 'mode': 0o755},
{'name': 'a_tree/generate_tree/a/b/c'},
{'name': 'a_tree/generate_tree/b', 'isdir': True, 'mode': 0o755},
{'name': 'a_tree/generate_tree/b/c', 'isdir': True, 'mode': 0o755},
{'name': 'a_tree/generate_tree/b/c', 'isdir': True, 'mode': 0o777},
{'name': 'a_tree/generate_tree/b/c/d'},
{'name': 'a_tree/generate_tree/b/d'},
{'name': 'a_tree/generate_tree/b/e'},
Expand Down