Skip to content

Add python_interpreter attr to pip rules #252

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 11, 2019
Merged

Add python_interpreter attr to pip rules #252

merged 4 commits into from
Nov 11, 2019

Conversation

brandjon
Copy link
Contributor

@brandjon brandjon commented Nov 7, 2019 •
edited
Loading

This is a rebase of @acumon's #158. It adds a python_interpreter attribute to pip_import and whl_library to allow customizing the command used to launch the Python interpreter. We'll follow this up with creating pip2_import and pip3_import as wrappers around this feature.

Diffbased against #251, which fixes documentation for these rules.

Work toward #249. Closes #158.

@brandjon brandjon requested a review from aiuto November 7, 2019 00:35
@brandjon brandjon requested a review from lberki as a code owner November 7, 2019 00:35
@brandjon brandjon self-assigned this Nov 7, 2019
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@brandjon brandjon mentioned this pull request Nov 7, 2019
Closed
@keith
Copy link
Member

keith commented Nov 7, 2019

I can't tell from this but is there a way to use this with your custom defined python toolchains that already encode the specific paths to interpreters? Or would this work by duplicating those paths?

@thundergolfer
Copy link

It doesn't look like it. python_interpreter is a string not a label. We run within Nix, so I think for us it'd suffice to put the string "python3" and that would resolve to our Nix environment Python3.6, but even that isn't ideal.

@brandjon
Copy link
Contributor Author

brandjon commented Nov 7, 2019

This won't interact with toolchains at all because the pip rules run at WORKSPACE evaluation time. It's just directly running the given string as a command name using repository_ctx.execute. The underlying mechanism hasn't changed, it's just allowing that string to now be customized.

This has the downside that a given repo definition has no way to run different Python commands on different systems.

@brandjon
Copy link
Contributor Author

brandjon commented Nov 7, 2019

Note that 2571dcf is a rebased and slightly modified (by me) version of @acumon's 3e1f35c. The googlebot doesn't detect this because the email address on the commit metadata doesn't match, but it's the same email as on the original PR #158. Overriding the googlebot check since consent to contribute was already established in #158.

@brandjon brandjon added cla: yes and removed cla: no labels Nov 7, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 7, 2019
@brandjon brandjon added cla: yes and removed cla: no labels Nov 7, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

aiuto
aiuto approved these changes Nov 8, 2019
View reviewed changes
Copy link

@aiuto aiuto left a comment

Choose a reason for hiding this comment

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

The code looks reasonable. But, honestly, it is impossible to tell if attribute doc specs are formatted correctly. But that is not your problem.

XMC and others added 4 commits November 11, 2019 12:18
@brandjon
Add attr python_interpreter to pip.bzl/whl.bzl
43ac269 
This commit also added --python_interpreter argument to piptool.py to
support this attribute.

Example usage:
pip_import(
    name = 'pip_deps',
    requirements = '//:requirements.txt',
    python_interpreter = 'python3.7',
)
@brandjon
Regenerate tools for previous update to add python_interpreter attr
da61864 
This required some bootstrapping using a partially reverted version of the
previous commit. The regeneration needs to happen after piptool.py is modified
to accept the flag, but before pip.bzl is modified to pass it.
@brandjon
Fix docstrings, reorder attr, run buildifier, regen docs and tools
7ed1256
@brandjon
Reorder attrs again to satisfy linter
220b41c
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@brandjon
Copy link
Contributor Author

Force-pushed a rebase now that #251 is merged. Overriding the CLA for the same reasons, hopefully for the last time on this PR.

@brandjon brandjon removed the cla: no label Nov 11, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@brandjon brandjon merged commit 7b222cf into bazel-contrib:master Nov 11, 2019
@brandjon brandjon deleted the pyattr branch November 11, 2019 17:32
This was referenced Nov 12, 2019
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aiuto aiuto aiuto approved these changes

@lberki lberki Awaiting requested review from lberki

Assignees

@brandjon brandjon

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brandjon @googlebot @keith @thundergolfer @aiuto