-
-
Notifications
You must be signed in to change notification settings - Fork 598
Exclude autogenerated BUILD file from wheel library runfiles #441
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
86ffdba
to
6b492b7
Compare
@hrfuller a polite ping (don't have permission to request you as a reviewer). |
@@ -29,7 +29,10 @@ def generate_build_file_contents( | ||
there may be no Python sources whatsoever (e.g. packages written in Cython: like `pymssql`). | ||
""" | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should WORKSPACE.Bazel
be a part of this list too?
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.
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:
.par
files. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What is the current behavior?
Bazel appears to inject a
BUILD
file into the root of an external repository. For the non-incrementalpip_install
the external repository root ends up looking something like this:Since
pip_parse
doesn'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
BUILD
to theexclude
so 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 theBUILD
files?Does this PR introduce a breaking change?
Other information
None