-
Notifications
You must be signed in to change notification settings - Fork 539
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
Add a pip3_import target for Python 3 compatibility. #85
Conversation
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.
:)
python/pip.bzl
Outdated
@@ -34,6 +34,14 @@ def _pip_import_impl(repository_ctx): | |||
if result.return_code: | |||
fail("pip_import failed: %s (%s)" % (result.stdout, result.stderr)) | |||
|
|||
def _pip2_import_impl(repository_ctx): |
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.
Not sure, but maybe there should be three options? pip_import
which uses system python
, pip2_import
which explicitly uses python2
, and pip3_import
which explicitly uses python3
?
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.
From what I can tell, the explicit Python 2 binaries always include a minor version (e.g. python2.7
). I think I'll update the commend and name to reflect that it's the system version.
A pip27
or similar could be added if needed...
@@ -13,7 +13,7 @@ | |||
# limitations under the License. |
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.
Would probably be nice to add documentation of this functionality to a README.md
?
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.
Docs updated.
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.
You also want to forward the configured binary to whl
: https://github.com/bazelbuild/rules_python/pull/82/files#diff-fecf9113b31102d557ae0e414f90d6e0
Otherwise you end up calling pip
with python3
and whl
with python
. This can cause build failures in certain libraries, though other libraries seem to be fine with it (in my case lxml
worked but PyYAML
failed).
"--name", repository_ctx.attr.name, | ||
"--input", repository_ctx.path(repository_ctx.attr.requirements), | ||
"--output", repository_ctx.path("requirements.bzl"), | ||
"--directory", repository_ctx.path(""), | ||
]) | ||
], quiet=False) |
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.
I think you want to return it to quiet.
any timeline for this pr? really need this feature bring in!!! |
I could really use this. Any updates? |
Not that I know of... I've been using this alternative Python packaging mechanism lately, which I like a lot: https://github.com/TriggerMail/rules_pyz |
+1. I cherry-picked this and it works like a charm! |
Can we please get this in? |
@brandjon who is looking at PRs now? |
I should have some time for PR reviews after the cut for Bazel 0.25, which is April 1st. There's a lot of backlog in rules_python I'd like to work down starting then. We've been prioritizing cleanups to the native rules but this issue is certainly highly requested. |
Any update on this? |
Fix / workaround for #33 .
Much thanks to @joshclimacell for doing essentially this.