Greatly speed up homebrew module when multiple packages are passed in the `name` key (#9181)

* Increase test coverage and assert output more strictly

* Remove unused `_current_package_is_installed_from_head`

* Remove `un/changed_count` and infer from un/changed_pkgs length

* Track `installed` & `outdated` package state once

* Validate package names beforehand

* Install packages in 1 brew call instead of N

This also has the side effect of fixing the check message so that it prints every packages that will be installed instead of only the first one.

* Uninstall packages in 1 brew call instead of N

* Link packages in 1 brew call instead of N

* Unlink packages in 1 brew call instead of N

* Upgrade packages in 1 brew call instead of N

* Remove dangling checks

* Remove `_status` method and directly return the tuple

* Add changelog fragment

* Fix invalid format string (nice catch pylint!)

* Update changelogs/fragments/9181-improve-homebrew-module-performance.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update brew info parsing for casks

* Update changelogs/fragments/9181-improve-homebrew-module-performance.yml

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
pull/9223/head
Thibaut Decombe 2024-12-02 20:17:04 +01:00 committed by GitHub
parent da97e220ef
commit fddccea940
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 409 additions and 191 deletions

View File

@ -0,0 +1,2 @@
minor_changes:
- homebrew - greatly speed up module when multiple packages are passed in the ``name`` option (https://github.com/ansible-collections/community.general/pull/9181).

View File

@ -308,21 +308,6 @@ class Homebrew(object):
self._params = self.module.params
return self._params
@property
def current_package(self):
return self._current_package
@current_package.setter
def current_package(self, package):
if not HomebrewValidate.valid_package(package):
self._current_package = None
self.failed = True
self.message = 'Invalid package: {0}.'.format(package)
raise HomebrewException(self.message)
else:
self._current_package = package
return package
# /class properties -------------------------------------------- }}}
def __init__(self, module, path, packages=None, state=None,
@ -347,13 +332,13 @@ class Homebrew(object):
def _setup_status_vars(self):
self.failed = False
self.changed = False
self.changed_count = 0
self.unchanged_count = 0
self.changed_pkgs = []
self.unchanged_pkgs = []
self.message = ''
def _setup_instance_vars(self, **kwargs):
self.installed_packages = set()
self.outdated_packages = set()
for key, val in iteritems(kwargs):
setattr(self, key, val)
@ -380,8 +365,48 @@ class Homebrew(object):
return self.brew_path
def _status(self):
return (self.failed, self.changed, self.message)
def _validate_packages_names(self):
invalid_packages = []
for package in self.packages:
if not HomebrewValidate.valid_package(package):
invalid_packages.append(package)
if invalid_packages:
self.failed = True
self.message = 'Invalid package{0}: {1}'.format(
"s" if len(invalid_packages) > 1 else "",
", ".join(invalid_packages),
)
raise HomebrewException(self.message)
def _get_packages_info(self):
cmd = [
"{brew_path}".format(brew_path=self.brew_path),
"info",
"--json=v2",
]
cmd.extend(self.packages)
if self.force_formula:
cmd.append("--formula")
rc, out, err = self.module.run_command(cmd)
if rc != 0:
self.failed = True
self.message = err.strip() or ("Unknown failure with exit code %d" % rc)
raise HomebrewException(self.message)
data = json.loads(out)
for package_detail in data.get("formulae", []):
if bool(package_detail.get("installed")):
self.installed_packages.add(package_detail["name"])
if bool(package_detail.get("outdated")):
self.outdated_packages.add(package_detail["name"])
for package_detail in data.get("casks", []):
if bool(package_detail.get("installed")):
self.installed_packages.add(package_detail["token"])
if bool(package_detail.get("outdated")):
self.outdated_packages.add(package_detail["token"])
# /prep -------------------------------------------------------- }}}
def run(self):
@ -390,60 +415,14 @@ class Homebrew(object):
except HomebrewException:
pass
if not self.failed and (self.changed_count + self.unchanged_count > 1):
changed_count = len(self.changed_pkgs)
unchanged_count = len(self.unchanged_pkgs)
if not self.failed and (changed_count + unchanged_count > 1):
self.message = "Changed: %d, Unchanged: %d" % (
self.changed_count,
self.unchanged_count,
changed_count,
unchanged_count,
)
(failed, changed, message) = self._status()
return (failed, changed, message)
# checks ------------------------------------------------------- {{{
def _current_package_is_installed(self):
cmd = [
"{brew_path}".format(brew_path=self.brew_path),
"info",
"--json=v2",
self.current_package,
]
if self.force_formula:
cmd.append("--formula")
rc, out, err = self.module.run_command(cmd)
if rc != 0:
self.failed = True
self.message = err.strip() or ("Unknown failure with exit code %d" % rc)
raise HomebrewException(self.message)
data = json.loads(out)
return _check_package_in_json(data, "formulae") or _check_package_in_json(data, "casks")
def _current_package_is_outdated(self):
rc, out, err = self.module.run_command([
self.brew_path,
'outdated',
self.current_package,
])
return rc != 0
def _current_package_is_installed_from_head(self):
if not self._current_package_is_installed():
return False
rc, out, err = self.module.run_command([
self.brew_path,
'info',
self.current_package,
])
try:
version_info = [line for line in out.split('\n') if line][0]
except IndexError:
return False
return version_info.split(' ')[-1] == 'HEAD'
# /checks ------------------------------------------------------ }}}
return (self.failed, self.changed, self.message)
# commands ----------------------------------------------------- {{{
def _run(self):
@ -454,6 +433,8 @@ class Homebrew(object):
self._upgrade_all()
if self.packages:
self._validate_packages_names()
self._get_packages_info()
if self.state == 'installed':
return self._install_packages()
elif self.state == 'upgraded':
@ -523,19 +504,22 @@ class Homebrew(object):
# /_upgrade_all -------------------------- }}}
# installed ------------------------------ {{{
def _install_current_package(self):
if self._current_package_is_installed():
self.unchanged_count += 1
self.unchanged_pkgs.append(self.current_package)
self.message = 'Package already installed: {0}'.format(
self.current_package,
def _install_packages(self):
packages_to_install = set(self.packages) - self.installed_packages
if len(packages_to_install) == 0:
self.unchanged_pkgs.extend(self.packages)
self.message = 'Package{0} already installed: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages),
)
return True
if self.module.check_mode:
self.changed = True
self.message = 'Package would be installed: {0}'.format(
self.current_package
self.message = 'Package{0} would be installed: {1}'.format(
"s" if len(packages_to_install) > 1 else "",
", ".join(packages_to_install)
)
raise HomebrewException(self.message)
@ -552,72 +536,28 @@ class Homebrew(object):
opts = (
[self.brew_path, 'install']
+ self.install_options
+ [self.current_package, head, formula]
+ list(packages_to_install)
+ [head, formula]
)
cmd = [opt for opt in opts if opt]
rc, out, err = self.module.run_command(cmd)
if rc == 0:
self.changed_count += 1
self.changed_pkgs.append(self.current_package)
self.changed_pkgs.extend(packages_to_install)
self.unchanged_pkgs.extend(self.installed_packages)
self.changed = True
self.message = 'Package installed: {0}'.format(self.current_package)
self.message = 'Package{0} installed: {1}'.format(
"s" if len(packages_to_install) > 1 else "",
", ".join(packages_to_install)
)
return True
else:
self.failed = True
self.message = err.strip()
raise HomebrewException(self.message)
def _install_packages(self):
for package in self.packages:
self.current_package = package
self._install_current_package()
return True
# /installed ----------------------------- }}}
# upgraded ------------------------------- {{{
def _upgrade_current_package(self):
command = 'upgrade'
current_package_is_installed = self._current_package_is_installed()
if not current_package_is_installed:
command = 'install'
if current_package_is_installed and not self._current_package_is_outdated():
self.message = 'Package is already upgraded: {0}'.format(
self.current_package,
)
self.unchanged_count += 1
self.unchanged_pkgs.append(self.current_package)
return True
if self.module.check_mode:
self.changed = True
self.message = 'Package would be upgraded: {0}'.format(
self.current_package
)
raise HomebrewException(self.message)
opts = (
[self.brew_path, command]
+ self.install_options
+ [self.current_package]
)
cmd = [opt for opt in opts if opt]
rc, out, err = self.module.run_command(cmd)
if rc == 0:
self.changed_count += 1
self.changed_pkgs.append(self.current_package)
self.changed = True
self.message = 'Package upgraded: {0}'.format(self.current_package)
return True
else:
self.failed = True
self.message = err.strip()
raise HomebrewException(self.message)
def _upgrade_all_packages(self):
opts = (
[self.brew_path, 'upgrade']
@ -639,138 +579,188 @@ class Homebrew(object):
if not self.packages:
self._upgrade_all_packages()
else:
for package in self.packages:
self.current_package = package
self._upgrade_current_package()
return True
# There are 3 action possible here depending on installed and outdated states:
# - not installed -> 'install'
# - installed and outdated -> 'upgrade'
# - installed and NOT outdated -> Nothing to do!
packages_to_install = set(self.packages) - self.installed_packages
packages_to_upgrade = self.installed_packages & self.outdated_packages
packages_to_install_or_upgrade = packages_to_install | packages_to_upgrade
if len(packages_to_install_or_upgrade) == 0:
self.unchanged_pkgs.extend(self.packages)
self.message = 'Package{0} already upgraded: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages),
)
return True
if self.module.check_mode:
self.changed = True
self.message = 'Package{0} would be upgraded: {1}'.format(
"s" if len(packages_to_install_or_upgrade) > 1 else "",
", ".join(packages_to_install_or_upgrade)
)
raise HomebrewException(self.message)
for command, packages in [
("install", packages_to_install),
("upgrade", packages_to_upgrade)
]:
if not packages:
continue
opts = (
[self.brew_path, command]
+ self.install_options
+ list(packages)
)
cmd = [opt for opt in opts if opt]
rc, out, err = self.module.run_command(cmd)
if rc != 0:
self.failed = True
self.message = err.strip()
raise HomebrewException(self.message)
self.changed_pkgs.extend(packages_to_install_or_upgrade)
self.unchanged_pkgs.extend(set(self.packages) - packages_to_install_or_upgrade)
self.changed = True
self.message = 'Package{0} upgraded: {1}'.format(
"s" if len(packages_to_install_or_upgrade) > 1 else "",
", ".join(packages_to_install_or_upgrade),
)
# /upgraded ------------------------------ }}}
# uninstalled ---------------------------- {{{
def _uninstall_current_package(self):
if not self._current_package_is_installed():
self.unchanged_count += 1
self.unchanged_pkgs.append(self.current_package)
self.message = 'Package already uninstalled: {0}'.format(
self.current_package,
def _uninstall_packages(self):
packages_to_uninstall = self.installed_packages & set(self.packages)
if len(packages_to_uninstall) == 0:
self.unchanged_pkgs.extend(self.packages)
self.message = 'Package{0} already uninstalled: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages),
)
return True
if self.module.check_mode:
self.changed = True
self.message = 'Package would be uninstalled: {0}'.format(
self.current_package
self.message = 'Package{0} would be uninstalled: {1}'.format(
"s" if len(packages_to_uninstall) > 1 else "",
", ".join(packages_to_uninstall)
)
raise HomebrewException(self.message)
opts = (
[self.brew_path, 'uninstall', '--force']
+ self.install_options
+ [self.current_package]
+ list(packages_to_uninstall)
)
cmd = [opt for opt in opts if opt]
rc, out, err = self.module.run_command(cmd)
if not self._current_package_is_installed():
self.changed_count += 1
self.changed_pkgs.append(self.current_package)
if rc == 0:
self.changed_pkgs.extend(packages_to_uninstall)
self.unchanged_pkgs.extend(set(self.packages) - self.installed_packages)
self.changed = True
self.message = 'Package uninstalled: {0}'.format(self.current_package)
self.message = 'Package{0} uninstalled: {1}'.format(
"s" if len(packages_to_uninstall) > 1 else "",
", ".join(packages_to_uninstall)
)
return True
else:
self.failed = True
self.message = err.strip()
raise HomebrewException(self.message)
def _uninstall_packages(self):
for package in self.packages:
self.current_package = package
self._uninstall_current_package()
return True
# /uninstalled ----------------------------- }}}
# linked --------------------------------- {{{
def _link_current_package(self):
if not self._current_package_is_installed():
def _link_packages(self):
missing_packages = set(self.packages) - self.installed_packages
if missing_packages:
self.failed = True
self.message = 'Package not installed: {0}.'.format(self.current_package)
self.message = 'Package{0} not installed: {1}.'.format(
"s" if len(missing_packages) > 1 else "",
", ".join(missing_packages),
)
raise HomebrewException(self.message)
if self.module.check_mode:
self.changed = True
self.message = 'Package would be linked: {0}'.format(
self.current_package
self.message = 'Package{0} would be linked: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages)
)
raise HomebrewException(self.message)
opts = (
[self.brew_path, 'link']
+ self.install_options
+ [self.current_package]
+ self.packages
)
cmd = [opt for opt in opts if opt]
rc, out, err = self.module.run_command(cmd)
if rc == 0:
self.changed_count += 1
self.changed_pkgs.append(self.current_package)
self.changed_pkgs.extend(self.packages)
self.changed = True
self.message = 'Package linked: {0}'.format(self.current_package)
self.message = 'Package{0} linked: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages)
)
return True
else:
self.failed = True
self.message = 'Package could not be linked: {0}.'.format(self.current_package)
self.message = 'Package{0} could not be linked: {1}.'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages)
)
raise HomebrewException(self.message)
def _link_packages(self):
for package in self.packages:
self.current_package = package
self._link_current_package()
return True
# /linked -------------------------------- }}}
# unlinked ------------------------------- {{{
def _unlink_current_package(self):
if not self._current_package_is_installed():
def _unlink_packages(self):
missing_packages = set(self.packages) - self.installed_packages
if missing_packages:
self.failed = True
self.message = 'Package not installed: {0}.'.format(self.current_package)
self.message = 'Package{0} not installed: {1}.'.format(
"s" if len(missing_packages) > 1 else "",
", ".join(missing_packages),
)
raise HomebrewException(self.message)
if self.module.check_mode:
self.changed = True
self.message = 'Package would be unlinked: {0}'.format(
self.current_package
self.message = 'Package{0} would be unlinked: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages)
)
raise HomebrewException(self.message)
opts = (
[self.brew_path, 'unlink']
+ self.install_options
+ [self.current_package]
+ self.packages
)
cmd = [opt for opt in opts if opt]
rc, out, err = self.module.run_command(cmd)
if rc == 0:
self.changed_count += 1
self.changed_pkgs.append(self.current_package)
self.changed_pkgs.extend(self.packages)
self.changed = True
self.message = 'Package unlinked: {0}'.format(self.current_package)
self.message = 'Package{0} unlinked: {1}'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages)
)
return True
else:
self.failed = True
self.message = 'Package could not be unlinked: {0}.'.format(self.current_package)
self.message = 'Package{0} could not be unlinked: {1}.'.format(
"s" if len(self.packages) > 1 else "",
", ".join(self.packages)
)
raise HomebrewException(self.message)
def _unlink_packages(self):
for package in self.packages:
self.current_package = package
self._unlink_current_package()
return True
# /unlinked ------------------------------ }}}
# /commands ---------------------------------------------------- }}}

View File

@ -56,6 +56,9 @@
- assert:
that:
- package_result.changed
- "package_result.msg == 'Package installed: gnu-tar'"
- "package_result.changed_pkgs == ['gnu-tar']"
- "package_result.unchanged_pkgs == []"
- name: Again install {{ package_name }} package using homebrew
homebrew:
@ -69,6 +72,41 @@
- assert:
that:
- not package_result.changed
- "package_result.msg == 'Package already installed: gnu-tar'"
- "package_result.changed_pkgs == []"
- "package_result.unchanged_pkgs == ['gnu-tar']"
- name: Unlink {{ package_name }} package using homebrew
homebrew:
name: "{{ package_name }}"
state: unlinked
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Package unlinked: gnu-tar'"
- "package_result.changed_pkgs == ['gnu-tar']"
- "package_result.unchanged_pkgs == []"
- name: Link {{ package_name }} package using homebrew
homebrew:
name: "{{ package_name }}"
state: linked
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Package linked: gnu-tar'"
- "package_result.changed_pkgs == ['gnu-tar']"
- "package_result.unchanged_pkgs == []"
- name: Uninstall {{ package_name }} package using homebrew
homebrew:
@ -82,6 +120,9 @@
- assert:
that:
- package_result.changed
- "package_result.msg == 'Package uninstalled: gnu-tar'"
- "package_result.changed_pkgs == ['gnu-tar']"
- "package_result.unchanged_pkgs == []"
- name: Again uninstall {{ package_name }} package using homebrew
homebrew:
@ -95,3 +136,188 @@
- assert:
that:
- not package_result.changed
- "package_result.msg == 'Package already uninstalled: gnu-tar'"
- "package_result.changed_pkgs == []"
- "package_result.unchanged_pkgs == ['gnu-tar']"
- name: Upgrade {{ package_name }} package using homebrew
homebrew:
name: "{{ package_name }}"
state: latest
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Package upgraded: gnu-tar'"
- "package_result.changed_pkgs == ['gnu-tar']"
- "package_result.unchanged_pkgs == []"
- name: Again upgrade {{ package_name }} package using homebrew
homebrew:
name: "{{ package_name }}"
state: latest
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- not package_result.changed
- "package_result.msg == 'Package already upgraded: gnu-tar'"
- "package_result.changed_pkgs == []"
- "package_result.unchanged_pkgs == ['gnu-tar']"
- vars:
package_names:
- gnu-tar
- gnu-time
block:
- name: Make sure {{ package_names }} packages are not installed
homebrew:
name: "{{ package_names }}"
state: absent
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
- name: Install only {{ package_names[0] }} package using homebrew
homebrew:
name: "{{ package_names[0] }}"
state: present
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
- name: Install {{ package_names }} packages using homebrew (one is already installed)
homebrew:
name: "{{ package_names }}"
state: present
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Changed: 1, Unchanged: 1'"
- "package_result.changed_pkgs == ['gnu-time']"
- "package_result.unchanged_pkgs == ['gnu-tar']"
- name: Again install {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: present
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- not package_result.changed
- "package_result.msg == 'Changed: 0, Unchanged: 2'"
- "package_result.changed_pkgs == []"
- "package_result.unchanged_pkgs | sort == ['gnu-tar', 'gnu-time']"
- name: Unlink {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: unlinked
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Changed: 2, Unchanged: 0'"
- "package_result.changed_pkgs | sort == ['gnu-tar', 'gnu-time']"
- "package_result.unchanged_pkgs == []"
- name: Link {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: linked
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Changed: 2, Unchanged: 0'"
- "package_result.changed_pkgs | sort == ['gnu-tar', 'gnu-time']"
- "package_result.unchanged_pkgs == []"
- name: Uninstall {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: absent
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Changed: 2, Unchanged: 0'"
- "package_result.changed_pkgs | sort == ['gnu-tar', 'gnu-time']"
- "package_result.unchanged_pkgs == []"
- name: Again uninstall {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: absent
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- not package_result.changed
- "package_result.msg == 'Changed: 0, Unchanged: 2'"
- "package_result.changed_pkgs == []"
- "package_result.unchanged_pkgs | sort == ['gnu-tar', 'gnu-time']"
- name: Upgrade {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: latest
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- package_result.changed
- "package_result.msg == 'Changed: 2, Unchanged: 0'"
- "package_result.changed_pkgs | sort == ['gnu-tar', 'gnu-time']"
- "package_result.unchanged_pkgs == []"
- name: Again upgrade {{ package_names }} packages using homebrew
homebrew:
name: "{{ package_names }}"
state: latest
update_homebrew: false
become: true
become_user: "{{ brew_stat.stat.pw_name }}"
register: package_result
- assert:
that:
- not package_result.changed
- "package_result.msg == 'Changed: 0, Unchanged: 2'"
- "package_result.changed_pkgs == []"
- "package_result.unchanged_pkgs | sort == ['gnu-tar', 'gnu-time']"