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

brew install into a virtualenv #8689

Closed
marstr opened this issue Mar 5, 2019 · 5 comments
Closed

brew install into a virtualenv #8689

marstr opened this issue Mar 5, 2019 · 5 comments

Comments

@marstr
Copy link
Member

marstr commented Mar 5, 2019

Our homebrew formula installs az using system python without a virtual environment. This is causing issues for folks who have existing non-hombrew python installs, or use system Python for their projects.

We should isolate the CLI into its own virtual environment, the way the bash install script does. It'll also allow us to fix this logic:

(bin/"az").write <<~EOS
#!/usr/bin/env bash
export PYTHONPATH="#{ENV["PYTHONPATH"]}"
if command -v python#{xy} >/dev/null 2>&1; then
python#{xy} -m azure.cli \"$@\"
else
python3 -m azure.cli \"$@\"
fi
EOS

Which is getting confused on systems where python3.x is available, but not where the azure-cli was installed. (Folks setting it up in a virtualenv by themselves.)

@marstr marstr added this to Triage in CodeGen Features via automation Mar 5, 2019
@marstr marstr moved this from Triage to High Priority in CodeGen Features Mar 5, 2019
@marstr marstr self-assigned this Mar 5, 2019
@marstr
Copy link
Member Author

marstr commented Mar 8, 2019

I started chasing this down, and made some good progress after following guidance here. However, for reasons I do not yet understand, when pip is invoked by homebrew's virtualenv code, installing resources entrypoints and jeepney both fail with a very generic error message:

pip._internal.exceptions.InstallationError: Command "/usr/local/Cellar/azure-cli/2.0.60/libexec/bin/python3.7 /usr/local/Cellar/azure-cli/2.0.60/libexec/lib/python3.7/site-packages/pip install --ignore-installed --no-user --prefix /private/tmp/pip-build-env-2szx5z77/overlay --no-warn-script-location -v --no-binary :all: --only-binary :none: -i https://pypi.org/simple -- flit" failed with error code 1 in None

I'm still investigating several ways forward. The happiest case would getting to the bottom of why exactly these dependencies are different than all the rest, and installing them appropriately before invoking Homebrew's code. Alternatively, I could try to cut these dependencies out of our product. Obviously, that could prove to be challenging in its own right.

I partnered with @devigned earlier today to dig, and we didn't make a lot of headway. I'm going to give it another look, and may recruit more help from @lmazuel.

@marstr
Copy link
Member Author

marstr commented Mar 8, 2019

Further investigation makes it look like our problems are related to: pypa/flit#245 and pypa/pip#6222

@marstr
Copy link
Member Author

marstr commented Mar 8, 2019

/CC @tjprescott @yugangw-msft

Alright, I've spent the better part of the day investigating our options here.

I've confirmed with @lmazuel that at the end of the day, we are not able to install all of our declared resources because of pypa/pip#6222. However, the good news is that the two packages involved (entrypoints and jeepney) only seem to be included because of keyring, which is no longer a requirement of Azure/msrestazure-for-python. Bad news, we can't adopt new versions of msrestazure-for-python because of a couple of delinquent command_modules that are still relying on relatively old versions of management libraries. Assuming that pypa/pip#6222 will not be fixed soon enough for us to just kick the can down the road, the right fix is to rewrite those command modules to use newer management library packages. However, it sounds like as a team we had previously decided it was too expensive to do this.

What that leaves us with, is the ability to add a hack where we simply just don't declare the two packages involved as dependencies anymore, on @lmazuel word that we don't need them.

@lmazuel
Copy link
Member

lmazuel commented Mar 8, 2019

To emphasize, keyring was only used in msrestazure authentication, which is not even use by the CLI (use it's own ADAL wrapper). And because keyring was a problem, I catch any failure and just disable the feature (that again the CLI doesn't use):
https://github.com/Azure/msrestazure-for-python/blob/e347fc59d323edccf04ce9a043768ce4d36b6271/msrestazure/azure_active_directory.py#L49-L53

So in an ideal world, "updating these command modules" + "pip has not bugs" = the world is all rainbow :). In practical terms, removing keyring bypass the issue before it even happens, with no known drawbacks.
My two cents

@yugangw-msft
Copy link
Contributor

I support the idea of removing keyring to bypass the issue.
In future on potential secure token persistence implementation, we just remember this when consider to using keyring, but that is still too far away to worthwhile think that much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants