Azure CLI 2.0 on Homebrew (with latest release 2.0.17) #17344

derekbekoe · 2017-08-28T23:39:16Z

Move prev. azure cli to azure-cli@1.0.rb
Closes Support installation on OSX with Homebrew Azure/azure-cli#643
Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

tamird

Need to create a version alias for the new formula as well:

Error: 1 problem in 1 formula
azure-cli:

Formula has other versions so create a versioned alias:
cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Aliases
ln -s ../Formula/azure-cli.rb azure-cli@2.0

derekbekoe · 2017-08-29T00:42:52Z

@tamird thanks for the initial comments. I fixed that and some formatting of the formula also.

ilovezfs · 2017-08-29T01:26:49Z

This is a duplicate of #13198 and it appears to have the exact same issue that it's not building the dependencies from source.

derekbekoe · 2017-08-29T01:55:49Z

Hi @ilovezfs the main blocker with that PR was that there were dependency conflicts.
This has been fixed in the latest version of upstream.

Also, some dependencies do not have source packages (Python sdist) available.

derekbekoe · 2017-08-29T02:02:10Z

The product source code is built from source with https://azurecliprod.blob.core.windows.net/releases/azure-cli_packaged_2.0.15.tar.gz

ilovezfs · 2017-08-29T02:13:13Z

I mean the dependencies, not the product source code.

ilovezfs

All dependencies need to be built from source not just the azure wheels themselves.

ilovezfs · 2017-08-29T04:19:39Z

Hi @ilovezfs the main blocker with that PR was that there were dependency conflicts.
This has been fixed in the latest version of upstream.

Also, some dependencies do not have source packages (Python sdist) available.

Regardless of whether the conflicts have or have not been resolved, I'm afraid we'll have to pass on this upgrade until everything can build from source.

vszakats · 2017-08-29T17:07:21Z

If you agree, it'd be nice to use this language-agnostic URL in homepage and in the caveat text:
https://docs.microsoft.com/cli/azure/overview

derekbekoe · 2017-08-29T21:48:00Z

@vszakats thanks now using the language-agnostic URL.

vszakats · 2017-08-29T21:52:10Z

Thank you @derekbekoe!

derekbekoe · 2017-08-29T22:00:06Z

@ilovezfs I am taking a look at building everything (including dependencies) from source.
I will update soon.

derekbekoe · 2017-08-29T23:23:54Z

@ilovezfs I now build from source by specifying --no-binary. 😄

Now we build from source, it takes longer to build.
Can bottles be created targeting the common macOS versions (sierra, el_capitan, yosemite).

Bottles states 'Bottles are created using the Brew Test Bot.'

Is this a process I need to initiate?

helenlivsc · 2017-08-30T09:14:15Z

After the fix is ready, does installing Azure Cli through homebrew require Xcode on MacOs in advance?

derekbekoe · 2017-08-30T17:16:14Z

Once Azure CLI is available through Homebrew, brew install azure-cli is all that will be needed.

derekbekoe · 2017-08-30T18:26:05Z

@ilovezfs Can we get another look at the PR?

derekbekoe · 2017-09-01T17:10:36Z

Friendly ping on this. :)

cglong · 2017-09-03T01:01:12Z

@derekbekoe FWIW bottles (and the associated bottle stanza) are automatically generated as part of BrewTestBot and will be available once the PR has been merged 😄

.

ilovezfs · 2017-09-03T20:16:54Z

Please recast this not to use virtualenv and to use setup_install_args instead.

derekbekoe · 2017-09-05T17:30:25Z

Please recast this not to use virtualenv and to use setup_install_args instead.

@ilovezfs Can you provide an example or expand on this further?

Currently we use include Language::Python::Virtualenv then create the venv with virtualenv_create(libexec) and install with libexec/"bin/pip", "install", "--no-binary", ":all:".

This appears similar to these formulae.

ilovezfs · 2017-09-05T17:34:57Z

cython, ipython, jupyter, meson, protobuf, snakemake, etc.

derekbekoe · 2017-09-05T17:43:56Z

I will take a look at those examples though the current formula looks similar to these other formulae.

ilovezfs · 2017-09-05T17:46:19Z

Yes, I did those. They're for small, frequently updated cases where I tired of maintaining the resource blocks. It's a hack.

ilovezfs · 2017-09-12T02:21:55Z

Formula/azure-cli.rb

  depends_on "openssl"
+  depends_on "python3"


should be :python3 not "python3"

ilovezfs · 2017-09-12T04:12:11Z

Formula/azure-cli.rb

-    system libexec/"bin/pip", "install", "--no-binary", ":all:", "--force-reinstall", "-U", "azure-nspkg", "azure-mgmt-nspkg", "-f", buildpath/"dist"
+    # This replaces the `import pkg_resources` namespace imports from upstream
+    # with empty string as the import is slow and not needed in this environment.
+    File.open(site_packages/"azure/__init__.py", "w")


touch site_packages/"azure/__init__.py"

The files already exist and contain these contents:

import pkg_resources pkg_resources.declare_namespace(__name__)

I want to empty the contents of those files.
touch would create the file if it didn't exist but doesn't empty its contents.

cglong · 2017-09-14T18:30:51Z

The patch won't be merged upstream as the patch addresses an issue in Homebrew (not Azure CLI) - Azure/azure-cli#4428 (comment). In other words, the patch is specific to homebrew.

FYI, Homebrew has a policy against using patches "when patching something that will never be accepted upstream". The general recommendation is to instead use inreplace in this case, although obviously it's up to ILZ if there will be an exception in this case.

ilovezfs · 2017-09-15T13:22:43Z

Where do az extension add extensions end up getting installed?

Regarding the patch, I think we'll need to do without it if it's never going to be merged upstream.

derekbekoe · 2017-09-15T15:08:41Z

Extensions get installed into the user directory or ~/.azure/cliextensions to be specific.

The patch resolves Homebrew/brew#837 which is not an issue with upstream.

If the patch is removed, the user experience is less than ideal. Without the patch, az extension add commands fail with stacktraces like below. To clarify, this only happens due to Homebrew/brew#837 and doesn't happen with our other installers.

Exception:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/pip/basecommand.py", line 215, in main
    status = self.run(options, args)
  File "/usr/local/lib/python2.7/site-packages/pip/commands/install.py", line 317, in run
    prefix=options.prefix_path,
  File "/usr/local/lib/python2.7/site-packages/pip/req/req_set.py", line 742, in install
    **kwargs
  File "/usr/local/lib/python2.7/site-packages/pip/req/req_install.py", line 831, in install
    self.move_wheel_files(self.source_dir, root=root, prefix=prefix)
  File "/usr/local/lib/python2.7/site-packages/pip/req/req_install.py", line 1032, in move_wheel_files
    isolated=self.isolated,
  File "/usr/local/lib/python2.7/site-packages/pip/wheel.py", line 247, in move_wheel_files
    prefix=prefix,
  File "/usr/local/lib/python2.7/site-packages/pip/locations.py", line 153, in distutils_scheme
    i.finalize_options()
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/command/install.py", line 264, in finalize_options
    "must supply either home or prefix/exec-prefix -- not both"
DistutilsOptionError: must supply either home or prefix/exec-prefix -- not both

The only workaround in this case is for a user to temporarily create the ~/.pydistutils.cfg file, run az extension add, then remove the file afterwards.

The patch does this for them.

ilovezfs · 2017-09-15T15:11:00Z

Then Homebrew/brew#837 would need to be fixed because a permanent patch isn't really OK.

derekbekoe · 2017-09-15T15:13:16Z

It should be okay to leave the patch in until Homebrew/brew#837 is resolved.

ilovezfs · 2017-09-15T15:14:48Z

Given Homebrew/brew#837 is closed, my assumption at this point has to be it will never be fixed, which means the patch is permanent, and therefore not acceptable.

derekbekoe · 2017-09-15T15:17:35Z

If Homebrew/brew#837 isn't going to be fixed and a patch for that is required for this formula, it makes sense to make an exception for this patch.

ilovezfs · 2017-09-15T15:20:04Z

Whether or not it makes sense from the perspective of functionality and user experience, I'm not willing to accept a patch that won't be upstreamed.

derekbekoe · 2017-09-15T15:25:10Z

We'd like this formula in homebrew as it's a highly requested installer.

So if a patch to fix the user experience is not an option, what other option will you accept that can get this PR merged?

I can add a caveat message that explains how to manually temporarily create the ~/.pydistutils.cfg file then remove it afterwards. Would you accept that?

ilovezfs · 2017-09-15T15:28:51Z

Would it not be possible to detect the installation is Homebrew, and handle it conditionally?

derekbekoe · 2017-09-15T15:37:23Z

I have reopened Azure/azure-cli#4428.
We can look at having conditional logic in the Azure CLI code for the next release of the product.

Would like to get this PR merged. For the next release, I can include conditional logic for homebrew in Azure CLI.

Sound good?

ilovezfs · 2017-09-15T15:43:20Z

Yup, thank you. The patch url is

url "https://raw.githubusercontent.com/Homebrew/formula-patches/e600681/azure-cli/bug-4428.patch"

derekbekoe · 2017-09-15T15:51:27Z

Changed patch URL.

ilovezfs · 2017-09-15T15:54:21Z

Formula/azure-cli.rb

+      export PYTHONPATH="#{ENV["PYTHONPATH"]}"
+      python#{xy} -m azure.cli \"$@\"
+    EOS
+    (bin/"az").write az_exec


(bin/"az").write <<-EOS.undent #!/usr/bin/env bash export PYTHONPATH="#{ENV["PYTHONPATH"]}" python#{xy} -m azure.cli \"$@\" EOS

That said, it would be nice if this could use the write_env_script DSL instead.

okay changed this...

ilovezfs · 2017-09-15T15:58:17Z

Formula/azure-cli.rb

-    assert_equal azure_cloud["resourceManagerEndpointUrl"], "https://management.azure.com/"
+    version_output = shell_output("#{bin}/az --version")
+    assert_match "azure-cli", version_output
+    system bin/"az", "account", "list"


Can we assert the output matches something here? I'd like to just get rid of the version test above, and have this do something a bit more involved to make sure things are working properly. See the tests in faas-cli, jose, jupyter, and stubby for some good examples.

I've added a different test.

This is similar to https://github.com/Homebrew/homebrew-core/blob/master/Formula/azure-cli.rb#L35.

derekbekoe · 2017-09-15T16:18:47Z

@ilovezfs Anything else?

ilovezfs · 2017-09-15T16:23:12Z

Please squash it down to one commit for azure-cli 2.0.17 and one commit for azure-cli@1 1.0.10.15 (new formula)

derekbekoe · 2017-09-15T16:45:15Z

@ilovezfs done.

ilovezfs · 2017-09-15T17:05:20Z

Thanks for your first contribution to Homebrew, @derekbekoe! (And for putting up with me.)

derekbekoe · 2017-09-15T17:09:23Z

Thanks for your cooperation @ilovezfs.

antoineco · 2017-09-18T10:24:02Z

Most anticipated, thanks for merging, and thanks @derekbekoe for making it happen :)

lamdor · 2017-09-18T18:40:48Z

I ran into an issue with this formula after installing with it not finding the python image: #18240

derekbekoe mentioned this pull request Aug 28, 2017

Support installation on OSX with Homebrew Azure/azure-cli#643

Closed

tamird reviewed Aug 29, 2017

View reviewed changes

ilovezfs previously requested changes Aug 29, 2017

View reviewed changes

derekbekoe force-pushed the azure-cli branch from 7266c38 to 974e54d Compare September 5, 2017 17:32

derekbekoe changed the title ~~Azure CLI 2.0 on Homebrew (with latest release 2.0.15)~~ Azure CLI 2.0 on Homebrew (with latest release 2.0.17) Sep 11, 2017

derekbekoe force-pushed the azure-cli branch from 974e54d to 72db4b4 Compare September 11, 2017 22:03

ilovezfs reviewed Sep 12, 2017

View reviewed changes

ilovezfs suggested changes Sep 15, 2017

View reviewed changes

derekbekoe added 2 commits September 15, 2017 09:44

azure-cli@1 1.0.10.15 (new formula)

8639ad9

azure-cli 2.0.17

f65bd07

derekbekoe force-pushed the azure-cli branch from bb6f654 to f65bd07 Compare September 15, 2017 16:45

ilovezfs approved these changes Sep 15, 2017

View reviewed changes

ilovezfs closed this in fbad3a8 Sep 15, 2017

lamdor mentioned this pull request Sep 18, 2017

azure-cli: python Library not loadedReason: image not found #18240

Closed

5 tasks

derekbekoe mentioned this pull request Sep 28, 2017

Homebrew: "Python location" not pointing to a virtual environment Azure/azure-cli#4573

Closed

Homebrew locked and limited conversation to collaborators May 4, 2018

Azure CLI 2.0 on Homebrew (with latest release 2.0.17) #17344

Azure CLI 2.0 on Homebrew (with latest release 2.0.17) #17344

Conversation

derekbekoe commented Aug 28, 2017 • edited

tamird left a comment • edited

Choose a reason for hiding this comment

derekbekoe commented Aug 29, 2017

ilovezfs commented Aug 29, 2017

derekbekoe commented Aug 29, 2017 • edited

derekbekoe commented Aug 29, 2017

ilovezfs commented Aug 29, 2017

ilovezfs left a comment

Choose a reason for hiding this comment

ilovezfs commented Aug 29, 2017

vszakats commented Aug 29, 2017

derekbekoe commented Aug 29, 2017

vszakats commented Aug 29, 2017

derekbekoe commented Aug 29, 2017

derekbekoe commented Aug 29, 2017

helenlivsc commented Aug 30, 2017

derekbekoe commented Aug 30, 2017

derekbekoe commented Aug 30, 2017

derekbekoe commented Sep 1, 2017

cglong commented Sep 3, 2017

ilovezfs commented Sep 3, 2017

derekbekoe commented Sep 5, 2017

ilovezfs commented Sep 5, 2017

derekbekoe commented Sep 5, 2017

ilovezfs commented Sep 5, 2017

Choose a reason for hiding this comment

derekbekoe Sep 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cglong commented Sep 14, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

ilovezfs commented Sep 15, 2017

derekbekoe commented Sep 15, 2017

antoineco commented Sep 18, 2017

lamdor commented Sep 18, 2017

derekbekoe commented Aug 28, 2017 •

edited

tamird left a comment •

edited

derekbekoe commented Aug 29, 2017 •

edited

derekbekoe Sep 12, 2017 •

edited