homebrew_cask: unhelpful error when become_user set wrong #4708

mitchblank · 2022-05-20T23:23:34Z

Summary

I was trying to migrate management of homebrew casks to ansible, but I got this very confusing error from inside the _brew_cask_command_is_deprecated() function (added by @jaanhio as part of #1481)

File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_cask.py\", line 884, in <module>
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_cask.py\", line 876, in main
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_cask.py\", line 440, in run
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_cask.py\", line 509, in _run
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_cask.py\", line 575, in _upgrade_all
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/modules/homebrew_cask.py\", line 503, in _brew_cask_command_is_deprecated
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/module_utils/_version.py\", line 78, in __ge__
  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/module_utils/_version.py\", line 338, in _cmp
TypeError: '<' not supported between instances of 'str' and 'int'

Well that's weird! I spent way too long staring at the code, but I couldn't figure out what had gone wrong.

As mentioned in the title, it was ultimately a configuration error which was causing the step to be executed as the wrong user. This meant that when brew --version was run it produced stdout...

Homebrew >=2.5.0 (shallow or no git repository)
Homebrew/homebrew-core (no Git repository)
Homebrew/homebrew-cask (no Git repository)

...and stderr...

fatal: unsafe repository ('/opt/homebrew/Library/Taps/homebrew/homebrew-core' is owned by someone else)
To add an exception for this directory, call:

git config --global --add safe.directory /opt/homebrew/Library/Taps/homebrew/homebrew-core

...and exited with status 1. I guess because of the context that it's being called from, the check_rc=True in _get_brew_version() didn't have much effect. This is a shame, since if I had been given an error containing that stderr output I would have diagnosed the problem much quicker.

Anyway, instead of failing it parsed the stdout as it found it and set self.brew_version to >=2.5.0 This then ended up being put into the (fairly fragile) LooseVersion type which then failed awkwardly.

As I said, the underlying issue was a misconfiguration on my part. However I wanted to open an issue for two reasons:

So the next person to hit this has more luck googling a solution than I did
Maybe the code could be hardened a bit to make this failure case a bit clearer

My suggestion: _get_brew_version() should sanity check the version string (at least make sure it starts with a digit) If it didn't return a sensible result, fail immediately and include the stderr from brew --version in the thrown exception.

Issue Type

Bug Report

Component Name

homebrew_cask

Ansible Version

ansible [core 2.12.4]
  config file = /Users/mitch/git/config/ansible/ansible.cfg
  configured module search path = ['/Users/mitch/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/homebrew/Cellar/ansible/5.6.0/libexec/lib/python3.10/site-packages/ansible
  ansible collection location = /Users/mitch/.ansible/collections:/usr/share/ansible/collections
  executable location = /opt/homebrew/bin/ansible
  python version = 3.10.2 (main, Feb  2 2022, 05:51:25) [Clang 13.0.0 (clang-1300.0.29.3)]
  jinja version = 3.1.1
  libyaml = True

Community.general Version

# /opt/homebrew/Cellar/ansible/5.6.0/libexec/lib/python3.10/site-packages/ansible_collections
Collection        Version
----------------- -------
community.general 4.7.0

Configuration

$ ansible-config dump --only-changed

OS / Environment

OS/X 12.4

Steps to Reproduce

- name: Upgrade all casks
  when: ansible_os_family == "Darwin"
  become: true.  # oops
  community.general.homebrew_cask:
    upgrade_all: true

Expected Results

An comprehensible error message

Actual Results

  File \"/tmp/ansible_community.general.homebrew_cask_payload_9v0sif3i/ansible_community.general.homebrew_cask_payload.zip/ansible_collections/community/general/plugins/module_utils/_version.py\", line 338, in _cmp
TypeError: '<' not supported between instances of 'str' and 'int'

Code of Conduct

I agree to follow the Ansible Code of Conduct

The text was updated successfully, but these errors were encountered:

ansibullbot · 2022-05-20T23:29:10Z

Files identified in the description:

plugins/modules/packaging/os/homebrew_cask.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

ansibullbot · 2022-05-20T23:29:10Z

cc @Akasurde @danieljaouen @enriclluelles @indrajitr @kyleabenson @martinm82
click here for bot help

Akasurde · 2022-05-21T04:29:14Z

@mitchblank Thanks for reporting this issue and doing an in-depth analysis. Would you be interested in opening a PR with your findings? Thanks.

felixfontein · 2022-05-27T09:26:30Z

Might be a duplicate of #4734.

mitchblank · 2022-05-28T22:55:05Z

@mitchblank Thanks for reporting this issue and doing an in-depth analysis. Would you be interested in opening a PR with your findings? Thanks.

I don't know if I'm the best placed to fix it. I'm fairly handy with python, but I would need to learn a lot about ansible internals.

In particular, it's not clear to me at all what the best way of bubbling up the error would be. The current code does...

        cmd = [self.brew_path, '--version']

        rc, out, err = self.module.run_command(cmd, check_rc=True)

... so it seems that it's trying to catch the error via check_rc=True, but I don't know enough about the innards of ansible to say why that doesn't work.

I suspect there could be a simple one-liner fix for that, but somebody with more ansible knowledge would have to be the one to determine what that is.

felixfontein · 2022-05-29T07:18:26Z

I don't think this is related to the exit code. Also the command executed by the module for the version shouldn't have returned an exit code != 0, otherwise Ansible would have already exited. If it returned that exit code for you, you probably executed it in a slightly different environment where it did return that exit code.

The bug that needs to be fixed is parsing the version from the brew --version output. Right now it extracts >=2.5.0, and the leading >= must be removed before the version is processed. Probably https://github.com/ansible-collections/community.general/blob/main/plugins/modules/packaging/os/homebrew_cask.py#L495-L496 needs to be changed like

         version = out.split('\n')[0].split(' ')[1]
+        if version.startswith('>='):
+            version = version[2:]
         self.brew_version = version

mitchblank · 2022-05-29T20:27:25Z

Hmm, you seem to be right about the exit code. I could have sworn when I was debugging this earlier I found that rc==1 but at least if I run sudo brew --version at the shell prompt at least it has exit status 0.

Right now it extracts >=2.5.0, and the leading >= must be removed before the version is processed.

No, that won't be good enough. The reason that we're checking the version is for _brew_cask_command_is_deprecated() which checks for >= 2.6.0. When run with the wrong permissions brew version always returns >=2.5.0 (even if the version is, say, 3.4.x) so we'll decide to use the wrong cask commands which breaks things. If anything we should make _brew_cask_command_is_deprecated() return true if it can't detect the version since only old (and presumably rare) versions of homebrew need the older commands.

Basically the story seems to be that homebrew switched from a separate brew cask ... command set to now making --cask a command-line option instead. For some time both were accepted and I assume homebrew just kept using the old form for maximum compatibility. However now the old commands are removed so we must use the modern >=2.6 API on current versions. So pretending that version==2.5.0 isn't helpful.

It might make sense to have _brew_cask_command_is_deprecated() test if something simple like brew help cask returns status !=0 if it can't get the version. But really the fact that we can't get the version (at least in the case where the issue is we're running as the wrong user) is just something that is going to cause other problems anyway -- we don't really need to recover from brew --version not working, the only issue is that we don't bubble up the stderr in any way. It's possible that the #4734 situation is different, I don't know.

felixfontein · 2022-05-30T05:13:24Z

Right now it extracts >=2.5.0, and the leading >= must be removed before the version is processed.

No, that won't be good enough. The reason that we're checking the version is for _brew_cask_command_is_deprecated() which checks for >= 2.6.0. When run with the wrong permissions brew version always returns >=2.5.0 (even if the version is, say, 3.4.x) so we'll decide to use the wrong cask commands which breaks things. If anything we should make _brew_cask_command_is_deprecated() return true if it can't detect the version since only old (and presumably rare) versions of homebrew need the older commands.

Ok, that sucks quite a lot. I was hoping that they'd update that hardcoded version string regularly, so that >=2.5.0 is printed mainly before the 2.6.0 release is made, and then the string switches to >=2.6.0, or something like that.

(This is something that should be improved on homebrew's side.)

It might make sense to have _brew_cask_command_is_deprecated() test if something simple like brew help cask returns status !=0 if it can't get the version. But really the fact that we can't get the version (at least in the case where the issue is we're running as the wrong user) is just something that is going to cause other problems anyway -- we don't really need to recover from brew --version not working, the only issue is that we don't bubble up the stderr in any way. It's possible that the #4734 situation is different, I don't know.

For checking brew help cask we'd have to make sure that all old versions return a non-zero exit code in case that argument is used.

Another approach would be to assume that >=xxx always means that the latest version is used. That's also risky, since there might be folks using the latest master branch from a longer time ago, but then that would be bad luck for them...

Or only use this new test (check brew help cask's exit code) when the version string starts with >=?

…ty.general#4708

ansibullbot · 2022-11-05T19:35:52Z

Files identified in the description:

plugins/modules/homebrew_cask.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

thoernle · 2023-08-07T20:52:58Z

Is there a workaround? This completely prevents me from using Homebrew Casks on MacOS.

ansibullbot added bug This issue/PR relates to a bug module module os packaging plugins plugin (any type) labels May 20, 2022

felixfontein mentioned this issue May 27, 2022

Issue in version.py when using homebrew_cask module #4734

Closed

1 task

lotyp added a commit to wayofdev/ansible-role-homebrew that referenced this issue Jun 14, 2022

feat: enabling workaround for macos. see: ansible-collections/communi…

014b4cc

…ty.general#4708

lotyp added a commit to wayofdev/ansible-role-homebrew that referenced this issue Jun 14, 2022

feat: enabling workaround for macos. see: ansible-collections/communi…

d87d863

…ty.general#4708

github-actions bot mentioned this issue Jun 14, 2022

chore(master): release 1.7.0 wayofdev/ansible-role-homebrew#14

Merged

lotyp mentioned this issue Jun 19, 2022

Dockutil error geerlingguy/mac-dev-playbook#153

Closed

pinko-fowle mentioned this issue Jul 15, 2022

Cannot install homebrew casks that require sudo #4957

Open

1 task

ansibullbot added the has_pr label Nov 5, 2022

excalq mentioned this issue Feb 20, 2024

TypeError during Homebrew Cask Installation of android-platform-tools on MacOS geerlingguy/ansible-collection-mac#92

Closed

Kylmakalle mentioned this issue Mar 11, 2024

Dock Command does not install correctly, causing the dock commands to skip geerlingguy/ansible-collection-mac#79

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

homebrew_cask: unhelpful error when become_user set wrong #4708

homebrew_cask: unhelpful error when become_user set wrong #4708

mitchblank commented May 20, 2022

ansibullbot commented May 20, 2022

ansibullbot commented May 20, 2022

Akasurde commented May 21, 2022

felixfontein commented May 27, 2022

mitchblank commented May 28, 2022

felixfontein commented May 29, 2022

mitchblank commented May 29, 2022

felixfontein commented May 30, 2022

ansibullbot commented Nov 5, 2022

thoernle commented Aug 7, 2023

homebrew_cask: unhelpful error when become_user set wrong #4708

homebrew_cask: unhelpful error when become_user set wrong #4708

Comments

mitchblank commented May 20, 2022

Summary

Issue Type

Component Name

Ansible Version

Community.general Version

Configuration

OS / Environment

Steps to Reproduce

Expected Results

Actual Results

Code of Conduct

ansibullbot commented May 20, 2022

ansibullbot commented May 20, 2022

Akasurde commented May 21, 2022

felixfontein commented May 27, 2022

mitchblank commented May 28, 2022

felixfontein commented May 29, 2022

mitchblank commented May 29, 2022

felixfontein commented May 30, 2022

ansibullbot commented Nov 5, 2022

thoernle commented Aug 7, 2023