- Notifications
You must be signed in to change notification settings - Fork202
Fix pkg_tar directory entries#647
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
AutomatedTester commentedDec 7, 2022
This PR works well, I've got it in the Selenium tree for now. |
nacl commentedDec 7, 2022
Could you provide test cases for this? If this is supporting what I think it is (passing in directory labels that are notTreeArtifacts), I am hesitant to support this any further, as isnot a best practice.@aiuto may have a differing opinion. Please correct me if I'm wrong, but the relevantcode in the Selenium tree is a |
jameshilliard commentedDec 7, 2022
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. |
aiuto commentedDec 7, 2022
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 normalfiles.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'
c280c43 to1848729Comparejameshilliard commentedDec 8, 2022
Test which reproduces the error is added. |
aiuto left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This PR is trying to add a feature that will not work all the time. If you are building a tree of files you can't explicitly name or enumerate, the only bazel supported way is to use ctx.actions.declare_directory to make a tree artifact.
| genrule( | ||
| name="generate_dir_file", | ||
| outs= ["lib"], | ||
| cmd="mkdir -p $@; echo 1 >$@/nsswitch.conf", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not a case we should actively support. It doesn't work in remote build situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does thisctx.actions.declare_directory based rule look correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That looks mostly right. It does a declare_directory, then passes the .path of that to some tool to fill in.
aiuto commentedDec 8, 2022
Maybe, but that case should be working already. |
| self.add_empty_file(entry.dest,**attrs) | ||
| else: | ||
| self.add_file(entry.src,entry.dest,**attrs) | ||
| ifos.path.isdir(entry.src): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 hasis_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: