Skip to content
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

Load @rules_python in six.BUILD #6795

Merged
merged 4 commits into from Oct 28, 2019

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Oct 24, 2019

This PR adds a load statement for py_library to make @six compatible with bazelbuild/bazel#9006.

//cc @aaliddell @gnossen

This way, we can avoid an unnecessary copy.
@aaliddell
Copy link
Contributor

If I remember correctly, there was a reason it was done this way. I'll have a check

@aaliddell
Copy link
Contributor

aaliddell commented Oct 24, 2019

See #6391

Prior to that PR, it was done effectively as you are now in this PR (i.e the only file in the library is six.py). However, we can't know at the @six//:six py_library creation time what the legacy_create_init flag might be set to when anyone creates a py_binary that depends on six. Since a number of other repos depend on @six, we can't just fix all the py_binary definitions in protobuf. Therefore, by moving six.py to __init__.py, we force it to work regardless of the legacy_create_init flag, as it won't overwrite an existing __init__.py file.

At some point legacy_create_init is getting flipped and removed, at which point this bodge can be fixed.

Edit: In hindsight, I should have annotated the code with this detail at the time...

@Yannic Yannic changed the title [bazel] Add strip_prefix to download of @six Load @rules_python in six.BUILD Oct 25, 2019
@Yannic
Copy link
Contributor Author

Yannic commented Oct 25, 2019

@aaliddell Thank you! Also fixed this in my PR for grpc.

Updated the PR to add the missing load for @rules_python. @acozzette can you review?

@acozzette acozzette self-assigned this Oct 28, 2019
@acozzette acozzette merged commit fc7c65a into protocolbuffers:master Oct 28, 2019
@acozzette
Copy link
Member

Thanks, @Yannic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants