Fix pkg_tar directory entries #647
Conversation
|
This PR works well, I've got it in the Selenium tree for now. |
|
Could you provide test cases for this? If this is supporting what I think it is (passing in directory labels that are not TreeArtifacts), I am hesitant to support this any further, as is not a best practice. @aiuto may have a differing opinion. Please correct me if I'm wrong, but the relevant code in the Selenium tree is a |
Hmm, is there a similar case that I should base one off of?
Well...it's apparently supported for zip files already so the inconsistency with tar files is problematic.
Yeah, I think that probably also needs to be changed, although I'm not very familiar with bazel in general so making tar work like zip files seemed easier for a short term fix. |
|
I worry about this too. It looks like it will pull in trees that might not have been staged, so it may do what you want for local builds but not remote builds. If zip does that, it is probably an accidental side effect. Both support tree artifacts as inputs. As far as tests, there already are tests for tree artifacts in tests/tar/BUILD. |
Directories appear to work for zip files but not tar files.
This should ensure we don't try to process directories as normal
files.
Fixes issues like:
rules_pkg/pkg/private/tar/tar_writer.py", line 242, in add_file
with open(file_content, 'rb') as f:
IsADirectoryError: [Errno 21] Is a directory: 'bazel-out/darwin-fastbuild/bin/py/selenium/webdriver/common/devtools/v106'
jameshilliard
force-pushed
the
fix-tar-dir-src
branch
from
c280c43
to
1848729
Compare
Test which reproduces the error is added. |
| genrule( | ||
| name = "generate_dir_file", | ||
| outs = ["lib"], | ||
| cmd = "mkdir -p $@; echo 1 >$@/nsswitch.conf", |
There was a problem hiding this comment.
This is not a case we should actively support. It doesn't work in remote build situations.
There was a problem hiding this comment.
Well the behavior is at least currently inconsistent with zip files so not sure what best option here is.
There was a problem hiding this comment.
Does this ctx.actions.declare_directory based rule look correct?
There was a problem hiding this comment.
That looks mostly right. It does a declare_directory, then passes the .path of that to some tool to fill in.
|
Maybe, but that case should be working already. |
| @@ -309,7 +309,10 @@ def add_manifest_entry(self, entry_list, file_attributes): | |||
| elif entry.entry_type == manifest.ENTRY_IS_EMPTY_FILE: | |||
| self.add_empty_file(entry.dest, **attrs) | |||
| else: | |||
| self.add_file(entry.src, entry.dest, **attrs) | |||
| if os.path.isdir(entry.src): | |||
There was a problem hiding this comment.
This is not the place we should fix it.
For correctness, it needs to be in pkg.bzl, where we test that the input file has is_directory set. But we are already doing that.
Directories appear to work for zip files but not tar files.
This should ensure we don't try to process directories as normal files.
Fixes issues like: