Skip to content

Allow the --isolated pip flag to be optionally unset#512

Merged
hrfuller merged 5 commits intobazel-contrib:mainfrom
UebelAndre:isolated
Sep 7, 2021
Merged

Allow the --isolated pip flag to be optionally unset#512
hrfuller merged 5 commits intobazel-contrib:mainfrom
UebelAndre:isolated

Conversation

@UebelAndre
Copy link
Copy Markdown
Contributor

@UebelAndre UebelAndre commented Aug 16, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

In adopting a wider use of pip_parse I've run into an issue where the pip config file which defines the cert location for my index_url is not being parsed, leading to failures when attempting to download packages.

Issue Number: N/A

What is the new behavior?

This PR introduces flags for optionally disabling the use of the --isolated flag that's currently always passed when invoking pip commands. Users can set isolated = False on uses of pip_parse and pip_install. Additionally, the environment variable RULES_PYTHON_PIP_ISOLATED can be set to a falsey value ("0", "False") to accomplish the same. The environment variable is intended to allow for this kind of configuration in .bazelrc files where users could define common --repo_env=RULES_PYTHON_PIP_ISOLATED to allow for this behavior in specific cases (like on CI machines).

I'm not sure if making --isolated optional is the best approach, but I do want to be able to rely on the machine's pip config to determine how to download things. The first alternative I reached for was to use extra_pip_args to pass --cert but next ran into an issue where --cert was different on different machines. I'm happy to hear some other alternatives though.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla
Copy link
Copy Markdown

google-cla bot commented Aug 16, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Aug 16, 2021
@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 16, 2021
@UebelAndre UebelAndre marked this pull request as ready for review August 16, 2021 17:53
@UebelAndre UebelAndre changed the title Allow the --isolated flag to be optionally unset Allow the --isolated pip flag to be optionally unset Aug 16, 2021
@UebelAndre
Copy link
Copy Markdown
Contributor Author

Another option for fixing this without a change to the repo would be #335 but for me this is undesirable since since it requires an infra change for something that seems like inconsistent behavior in pip(?)

Co-authored-by: Henry Fuller <hrofuller@gmail.com>
@UebelAndre UebelAndre requested a review from hrfuller September 1, 2021 03:40
@UebelAndre
Copy link
Copy Markdown
Contributor Author

@hrfuller Hey, anything more I need to do for this PR?

@UebelAndre
Copy link
Copy Markdown
Contributor Author

@hrfuller Hey, sorry to ping again but it'd be really great if this were to be merged 🙏 😅

@hrfuller
Copy link
Copy Markdown
Contributor

hrfuller commented Sep 7, 2021

LGTM. Merging. Thanks @UebelAndre

@hrfuller hrfuller merged commit 9f59762 into bazel-contrib:main Sep 7, 2021
@UebelAndre UebelAndre deleted the isolated branch September 7, 2021 17:13
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.

3 participants