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

Add pip3_import #256

Merged
merged 4 commits into from
Nov 15, 2019
Merged

Add pip3_import #256

merged 4 commits into from
Nov 15, 2019

Conversation

brandjon
Copy link
Member

@brandjon brandjon commented Nov 12, 2019

Add pip3_import as a macro built on the new python_interpreter attribute to pip_import.

Also change canonical wheel repo naming to include the pip_import name, so the same wheel can be named by multiple pip_imports.

Update README, regenerate docs and tools.

Thanks go out to the authors of similar PRs #179, #85, and #216.

Fixes #33 (yaml import problem).
Fixes #165 (documentation request).
Closes #85 (similar PR).
Closes #179 (similar PR).
Fixes #249 (feature tracking bug).

This adds two repo rule macros, `pip2_import` and `pip3_import`, as wrappers
around `pip_import` that set `python_interpreter` to "python2" and "python3"
respectively. The latter is important for requesting the Python 3 version of a
package, since the `pip_import` rule invokes "python", which is Python 2 on
most systems.

piptool.py is updated to prefix the names of the wheel repos (an implementation
detail of rules_python) with the name given to `pip_import`. This is needed to
avoid shadowing wheel repos when the same wheel name is used by separate
`pip_import` invocations -- in particular when the same wheel is used for both
PY2 and PY3. (Thanks to @joshclimacell for pointing this detail out in
his prototype 90a70d5.)

Regenerated the .par files and docs.
python/pip.bzl Outdated
# We don't advertise "Use this for PY2 programs" because it's not clear
# whether this alias is always available, and on most (but not all) systems
# `python` is Python 2 anyway.
pip_import(python_interpreter = "python2", **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work on macOS I guess

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC macOS has python and python2.7 but not python2, right?

I was ambivalent about whether to add pip2_import at all, or just leave that use case to pip_import (with or without a custom python_interpreter attr). I guess it's not ideal to encourage people to switch Python 2 deps to use pip2_import and thereby break mac users by default.

I suppose we could make this macro smarter to detect what platform it's running on and choose a different default. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Yea I think related to bazelbuild/bazel#9402 it would be safe to default this to python2.7 on macOS, but I agree that this isn't a huge issue since this still provides complete customizability for users

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to revert the addition of pip2_import for the purposes of this PR. Filed #258 to track the feature of pip_import reliably detecting a suitable Python interpreter.

Better explain the structure of the packaging rules. Mention pip3_import and
concerns around versioning and hermeticity. Clarify that users shouldn't
depend on wheel repo names and recommend requirement() syntax for extras. Add
an announcement about `pip3_import` support.

Also add backticks to filenames.
@brandjon brandjon requested a review from aiuto November 13, 2019 17:40
@brandjon
Copy link
Member Author

@aiuto This adds a wrapper around the existing pip_import and adds a bunch of README text.

It also changes repo naming behavior, which breaks users who depended directly on wheel repos rather than going through the helper function as an intermediary.

I think we'd like to release this functionality in a 0.0.2 release for inclusion in the federation for Bazel 2.0.

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