-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
bazel: Bump rules_python version to 0.1.0 #15236
Conversation
29f91ab
to
9ac2820
Compare
my understading of https://github.com/bazelbuild/rules_python#importing-pip-dependencies-with-pip_import-legacy is that we need to shift the pattern we currently use of doing so (with the updated rules_python) does seem to fix the problem i had with namespace packages, but it has caused a different error re test/use_category labels - investigating further ... |
4b7e988
to
dca34c5
Compare
Signed-off-by: Ryan Northey <[email protected]>
73dd502
to
263e34f
Compare
afaict here https://github.com/bazelbuild/rules_python#importing-pip-dependencies it suggests that the python interpreter is set in the edit: ignore - this is the interpreter for the py_binary not the deps |
Signed-off-by: Ryan Northey <[email protected]>
hmm - this is hitting what looks like a windows bug here: i think the issue is this one: |
@wrowe @davinci26 any thoughts on the Windows issue? Thanks. |
I think this is the same issue I think I had solved for #14329 but was never added to the PR branch See this comment thread: #14329 (comment) |
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
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.
@moderation can you take a look as dep shepherd? I seem to remember a previous patch like this..
Signed-off-by: Ryan Northey <[email protected]>
@phlax This was my prior PR where I couldn't get around the Windows issues - #14329. I think I missed @sunjayBhatia going into the holidays and like @htuch dislike carrying patches as they add friction to future upgrades. With this change it is possible to consolidate |
yep me too - i have opened the ticket upstream to address im just testing with version 44.1 - if that works we can avoid the patch by using |
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.
Change to using developer provided tarball. Consider consolidating dependency locations.
unfortunately the version of setuptools on current rules_python master doesnt fix the problem (ie setuptools==44.1) i have raised a PR upstrem bazelbuild/rules_python#422 to address in line with your comment here @moderation - #15236 (comment) - i will switch to use the release tarball this still means using the patch for now - if my PR is landed upstream we probs want to revert to using non-release tarball again - but we will be able to eliminate the patch that way ill update this pr now... |
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
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.
Looks good, just a couple of minor things.
/wait
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
as an update to the upstream resolution - it seems it wont land too quickly bazelbuild/rules_python#422 (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.
LGTM, thanks!
/lgtm deps |
Signed-off-by: Ryan Northey [email protected]
Commit Message: bazel: Bump rules_python version to 0.1.0
Additional Description:
afaict the current
rules_python
has quite a few things that dont work correctly - namespaced packages, entrypoints and wheels egversion 0.1.0 at least seems to fix the issue of namespaced packages, and possibly other problems too
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]