From fddccea9403d76fc514bba3f87c1eedc1c0c8b8e Mon Sep 17 00:00:00 2001 From: Thibaut Decombe <68703331+UnknownPlatypus@users.noreply.github.com> Date: Mon, 2 Dec 2024 20:17:04 +0100 Subject: [PATCH] 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 * 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 Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> --- ...81-improve-homebrew-module-performance.yml | 2 + plugins/modules/homebrew.py | 372 +++++++++--------- .../targets/homebrew/tasks/formulae.yml | 226 +++++++++++ 3 files changed, 409 insertions(+), 191 deletions(-) create mode 100644 changelogs/fragments/9181-improve-homebrew-module-performance.yml diff --git a/changelogs/fragments/9181-improve-homebrew-module-performance.yml b/changelogs/fragments/9181-improve-homebrew-module-performance.yml new file mode 100644 index 0000000000..b3b6ba2ca4 --- /dev/null +++ b/changelogs/fragments/9181-improve-homebrew-module-performance.yml @@ -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). \ No newline at end of file diff --git a/plugins/modules/homebrew.py b/plugins/modules/homebrew.py index bc5d8649e7..8eb1b9d689 100644 --- a/plugins/modules/homebrew.py +++ b/plugins/modules/homebrew.py @@ -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 ---------------------------------------------------- }}} diff --git a/tests/integration/targets/homebrew/tasks/formulae.yml b/tests/integration/targets/homebrew/tasks/formulae.yml index 1ca8d753e7..21276e3a2e 100644 --- a/tests/integration/targets/homebrew/tasks/formulae.yml +++ b/tests/integration/targets/homebrew/tasks/formulae.yml @@ -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']"