Exclude autogenerated BUILD file from wheel library runfiles#441
Exclude autogenerated BUILD file from wheel library runfiles#441person142 wants to merge 1 commit intobazel-contrib:masterfrom
Conversation
86ffdba to
6b492b7
Compare
|
@hrfuller a polite ping (don't have permission to request you as a reviewer). |
|
|
||
| data_exclude = ["*.whl", "**/*.py", "**/* *", "BUILD.bazel", "WORKSPACE"] + pip_data_exclude | ||
| data_exclude = ( | ||
| ["*.whl", "**/*.py", "**/* *", "BUILD", "BUILD.bazel", "WORKSPACE"] |
There was a problem hiding this comment.
Should WORKSPACE.Bazel be a part of this list too?
There was a problem hiding this comment.
https://docs.bazel.build/versions/master/external.html#working-with-external-dependencies
Looks like WORKSPACE.bazel is an allowed value. We don't use it in our repository rules though.
|
I believe this would cause a regression to #427 in pip_parse. I think the correct way to do this is probably to extract the wheels into a subdirectory instead of at the root of the individual repos. |
Hasn’t the regression already happened? An empty |
|
Ah, or maybe “this” -> “the current state of affairs” and not “the changes in this PR”, in which case yes I think so. |
|
My concern is that excluding "BUILD" would exclude a valid build directory on case insensitive file systems. There wont be a regression ala #427 because we write build file contents into BUILD.bazel now. Bazel "glob" ignores directories by default so if you tried to install the keyring wheel used as example in $427 the build directory would be excluded. As an aside if you need an immediate workaround you can use the pip_data_excludes As I mention above, I think the correct way to fix this issue is to create the wheel package in a sub directory. |
Right, but when using Certainly this PR isn't the right fix, and I'm happy to close it-but I believe the regression is already present. Using a sub-directory would be another work around-but at the end of the day this appears to be a Bazel bug-it seems to be adding an automatic empty |
|
A concrete example of the regression ( |
|
Ah, I was mistaken; it's not a Bazel thing; looks like it's this line that inserts the empty build file: https://github.com/bazelbuild/rules_python/blob/master/python/pip_install/pip_repository.bzl#L15 I'll open a new PR with the proper fix... |
|
#457 should be a better fix. |
PR Checklist
Please check if your PR fulfills the following requirements:
.parfiles. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What is the current behavior?
Bazel appears to inject a
BUILDfile into the root of an external repository. For the non-incrementalpip_installthe external repository root ends up looking something like this:Since
pip_parsedoesn't have the extra layers of directories you end up with the external repository looking something like this:The build file is then picked up in the data glob and injected into the runfiles.
Issue Number: #440
What is the new behavior?
This adds the autogenerated
BUILDto theexcludeso that it doesn't end up in the runfiles. It still won't fix the issue identified in #427 however, but AFAICT that will require changes to Bazel itself to not autogenerate theBUILDfiles?Does this PR introduce a breaking change?
Other information
None