pkg_tar: load attributes for TreeArtifacts #789
Conversation
novas0x2a
force-pushed
the
tar-expose-tree
branch
2 times, most recently
from
e0b8c7c
to
eb6ba24
Compare
This change teaches pkg_tar to reinvoke file_attributes for each individual file in a TreeArtifact; this function is used to load the attributes passed via pkg_tar arguments (like `mode`). Before this change, there's no way to set individual attributes for files in a TreeArtifact. This attempts to maintain the previous behavior of intermediate dirs being 0o775, unless a custom mode is passed to override it.
novas0x2a
force-pushed
the
tar-expose-tree
branch
from
eb6ba24
to
ed48f1c
Compare
| @@ -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: | |||
There was a problem hiding this comment.
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.
| @@ -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): | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| 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: |
There was a problem hiding this comment.
It seems like just having 0o755 be the default mode would eliminate the need for the test here.
There was a problem hiding this comment.
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).
|
Any updates? |
|
Sorry for the last response. I was unavailable from Jan 10 until about now. |
This change teaches pkg_tar to reinvoke file_attributes for each individual file in a TreeArtifact; this function is used to load the attributes passed via pkg_tar arguments (like
mode). Before this change, there's no way to set individual attributes for files in a TreeArtifact.This attempts to maintain the previous behavior of intermediate dirs being 0o775, unless a custom mode is passed to override it.