From cd4ef220d1cae74f61f24d58ffbffc2aa6974058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20A=CC=81lvaro?= Date: Thu, 10 Aug 2023 07:16:01 +0200 Subject: [PATCH 1/7] feat(mac_brew_pkg): Add homebrew_prefix method --- salt/modules/mac_brew_pkg.py | 129 ++++++++++++++++++++++++++++------- 1 file changed, 103 insertions(+), 26 deletions(-) diff --git a/salt/modules/mac_brew_pkg.py b/salt/modules/mac_brew_pkg.py index 4b4774532cf0..e2377761238e 100644 --- a/salt/modules/mac_brew_pkg.py +++ b/salt/modules/mac_brew_pkg.py @@ -1,6 +1,14 @@ """ Homebrew for macOS +It is recommended for the ``salt-minion`` to have the ``HOMEBREW_PREFIX`` +environment variable set. + +This will ensure that Salt uses the correct path for the ``brew`` binary. + +Typically, this is set to ``/usr/local`` for Intel Macs and ``/opt/homebrew`` +for Apple Silicon Macs. + .. important:: If you feel that Salt should be using this module to manage packages on a minion, and it is using a different module (or gives an error similar to @@ -10,6 +18,7 @@ import copy import logging +import os import salt.utils.data import salt.utils.functools @@ -31,7 +40,7 @@ def __virtual__(): """ if __grains__["os"] != "MacOS": return False, "brew module is macos specific" - if not _homebrew_os_bin(): + if not _homebrew_bin(quiet=False): return False, "The 'brew' binary was not found" return __virtualname__ @@ -97,31 +106,56 @@ def _homebrew_os_bin(): """ Fetch PATH binary brew full path eg: /usr/local/bin/brew (symbolic link) """ - return salt.utils.path.which("brew") + # Add "/opt/homebrew" temporary to the PATH for Apple Silicon if + # the PATH does not include "/opt/homebrew" + original_path = None + current_path = os.environ.get("PATH", "") + homebrew_path = "/opt/homebrew/bin" + if homebrew_path not in current_path.split(os.path.pathsep): + original_path = current_path + extended_path = os.path.pathsep.join([original_path, homebrew_path]) + os.environ["PATH"] = extended_path.lstrip(os.path.pathsep) + + # Search for the brew executable in the current PATH + brew = salt.utils.path.which("brew") + + # Restore the original PATH if needed + if original_path is not None: + if original_path == "": + del os.environ["PATH"] + else: + os.environ["PATH"] = original_path + + return brew -def _homebrew_bin(): + +def _homebrew_bin(quiet=False): """ Returns the full path to the homebrew binary in the homebrew installation folder + + quiet + When ``True``, does not log warnings when the homebrew prefix cannot be found. """ - brew = _homebrew_os_bin() - if brew: - # Fetch and ret brew installation folder full path eg: /opt/homebrew/bin/brew - brew = __salt__["cmd.run"](f"{brew} --prefix", output_loglevel="trace") - brew += "/bin/brew" - return brew + ret = homebrew_prefix(quiet=quiet) + if ret is not None: + ret += "/bin/brew" + + return ret def _call_brew(*cmd, failhard=True): """ Calls the brew command with the user account of brew """ - user = __salt__["file.get_user"](_homebrew_bin()) + brew_exec = _homebrew_bin(quiet=True) + + user = __salt__["file.get_user"](brew_exec) runas = user if user != __opts__["user"] else None _cmd = [] if runas: _cmd = [f"sudo -i -n -H -u {runas} -- "] - _cmd = _cmd + [_homebrew_bin()] + list(cmd) + _cmd = _cmd + [brew_exec] + list(cmd) _cmd = " ".join(_cmd) runas = None @@ -148,6 +182,59 @@ def _list_pkgs_from_context(versions_as_list): return ret +def homebrew_prefix(quiet=False): + """ + Returns the full path to the homebrew prefix. + + quiet + When ``True``, does not log warnings when the homebrew prefix cannot be found. + + CLI Example: + + .. code-block:: bash + + salt '*' pkg.homebrew_prefix + """ + + # Try HOMEBREW_PREFIX env variable + env_homebrew_prefix = "HOMEBREW_PREFIX" + if env_homebrew_prefix in os.environ: + log.debug(f"{env_homebrew_prefix} is set. Using it for homebrew prefix.") + return os.environ[env_homebrew_prefix] + elif not quiet: + log.warning(f"{env_homebrew_prefix} is not set. Please, consider adding it.") + + # Try brew --prefix + try: + log.debug("Trying to find homebrew prefix by running 'brew --prefix'") + + brew = _homebrew_os_bin() + if brew is not None: + # Check if the found brew command is the right one + import salt.modules.cmdmod + import salt.modules.file + + runas = salt.modules.file.get_user(brew) + ret = salt.modules.cmdmod.run( + "brew --prefix", runas=runas, output_loglevel="trace", raise_err=True + ) + + return ret + except CommandExecutionError as e: + if not quiet: + log.warning( + f"Unable to find homebrew prefix by running 'brew --prefix'. Error: {str(e)}" + ) + + # Unable to find brew prefix + if not quiet: + log.warning( + f"Failed to find homebrew prefix. Please, set {env_homebrew_prefix} env variable" + ) + + return None + + def list_pkgs(versions_as_list=False, **kwargs): """ List the packages currently installed in a dict:: @@ -657,17 +744,13 @@ def hold(name=None, pkgs=None, sources=None, **kwargs): # pylint: disable=W0613 if result: changes = {"old": "install", "new": "hold"} ret[target].update(changes=changes, result=True) - ret[target]["comment"] = "Package {} is now being held.".format( - target - ) + ret[target]["comment"] = f"Package {target} is now being held." else: ret[target].update(result=False) ret[target]["comment"] = f"Unable to hold package {target}." else: ret[target].update(result=True) - ret[target]["comment"] = "Package {} is already set to be held.".format( - target - ) + ret[target]["comment"] = f"Package {target} is already set to be held." return ret @@ -727,9 +810,7 @@ def unhold(name=None, pkgs=None, sources=None, **kwargs): # pylint: disable=W06 elif target in pinned: if "test" in __opts__ and __opts__["test"]: ret[target].update(result=None) - ret[target]["comment"] = "Package {} is set to be unheld.".format( - target - ) + ret[target]["comment"] = f"Package {target} is set to be unheld." else: result = _unpin(target) if result: @@ -740,14 +821,10 @@ def unhold(name=None, pkgs=None, sources=None, **kwargs): # pylint: disable=W06 ] = f"Package {target} is no longer being held." else: ret[target].update(result=False) - ret[target]["comment"] = "Unable to unhold package {}.".format( - target - ) + ret[target]["comment"] = f"Unable to unhold package {target}." else: ret[target].update(result=True) - ret[target]["comment"] = "Package {} is already set not to be held.".format( - target - ) + ret[target]["comment"] = f"Package {target} is already set not to be held." return ret From 78bb28a43a794c8680dcda7d9e3ac5ee1b6fef3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20A=CC=81lvaro?= Date: Thu, 10 Aug 2023 07:18:00 +0200 Subject: [PATCH 2/7] test(mac_brew_pkg): Add tests for homebrew_prefix --- .../pytests/unit/modules/test_mac_brew_pkg.py | 153 ++++++++++++++++-- 1 file changed, 138 insertions(+), 15 deletions(-) diff --git a/tests/pytests/unit/modules/test_mac_brew_pkg.py b/tests/pytests/unit/modules/test_mac_brew_pkg.py index 749c9765cce8..3a1e007aac72 100644 --- a/tests/pytests/unit/modules/test_mac_brew_pkg.py +++ b/tests/pytests/unit/modules/test_mac_brew_pkg.py @@ -1,6 +1,7 @@ """ :codeauthor: Nicole Thomas """ +import os import textwrap import pytest @@ -22,8 +23,13 @@ def TAPS_LIST(): @pytest.fixture -def HOMEBREW_BIN(): - return "/usr/local/bin/brew" +def HOMEBREW_PREFIX(): + return "/opt/homebrew" + + +@pytest.fixture +def HOMEBREW_BIN(HOMEBREW_PREFIX): + return HOMEBREW_PREFIX + "/bin/brew" @pytest.fixture @@ -371,7 +377,9 @@ def test_list_taps(TAPS_STRING, TAPS_LIST, HOMEBREW_BIN): mock_taps = MagicMock(return_value={"stdout": TAPS_STRING, "retcode": 0}) mock_user = MagicMock(return_value="foo") mock_cmd = MagicMock(return_value="") - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch.dict( mac_brew.__salt__, {"file.get_user": mock_user, "cmd.run_all": mock_taps, "cmd.run": mock_cmd}, @@ -399,7 +407,9 @@ def test_tap_failure(HOMEBREW_BIN): mock_failure = MagicMock(return_value={"stdout": "", "stderr": "", "retcode": 1}) mock_user = MagicMock(return_value="foo") mock_cmd = MagicMock(return_value="") - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch.dict( mac_brew.__salt__, { @@ -418,7 +428,9 @@ def test_tap(TAPS_LIST, HOMEBREW_BIN): mock_failure = MagicMock(return_value={"retcode": 0}) mock_user = MagicMock(return_value="foo") mock_cmd = MagicMock(return_value="") - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch.dict( mac_brew.__salt__, { @@ -432,17 +444,118 @@ def test_tap(TAPS_LIST, HOMEBREW_BIN): assert mac_brew._tap("homebrew/test") +# 'homebrew_prefix' function tests: 4 + + +def test_homebrew_prefix_env(HOMEBREW_PREFIX): + """ + Test the path to the homebrew prefix by looking + at the HOMEBREW_PREFIX environment variable. + """ + mock_env = os.environ.copy() + mock_env["HOMEBREW_PREFIX"] = HOMEBREW_PREFIX + + with patch.dict(os.environ, mock_env): + assert mac_brew.homebrew_prefix() == HOMEBREW_PREFIX + + +def test_homebrew_prefix_command(HOMEBREW_PREFIX, HOMEBREW_BIN): + """ + Test the path to the homebrew prefix by running + the brew --prefix command when the HOMEBREW_PREFIX + environment variable is not set. + """ + mock_env = os.environ.copy() + if "HOMEBREW_PREFIX" in mock_env: + del mock_env["HOMEBREW_PREFIX"] + + with patch.dict(os.environ, mock_env): + with patch( + "salt.modules.cmdmod.run", MagicMock(return_value=HOMEBREW_PREFIX) + ), patch("salt.modules.file.get_user", MagicMock(return_value="foo")), patch( + "salt.modules.mac_brew_pkg._homebrew_os_bin", + MagicMock(return_value=HOMEBREW_BIN), + ): + assert mac_brew.homebrew_prefix() == HOMEBREW_PREFIX + + +def test_homebrew_prefix_returns_none(): + """ + Tets that homebrew_prefix returns None when + all attempts fail. + """ + + mock_env = os.environ.copy() + if "HOMEBREW_PREFIX" in mock_env: + del mock_env["HOMEBREW_PREFIX"] + + with patch.dict(os.environ, mock_env): + with patch( + "salt.modules.mac_brew_pkg._homebrew_os_bin", MagicMock(return_value=None) + ): + assert mac_brew.homebrew_prefix() is None + + +def test_homebrew_prefix_returns_none_even_with_execution_errors(): + """ + Tets that homebrew_prefix returns None when + all attempts fail even with command execution errors. + """ + + mock_env = os.environ.copy() + if "HOMEBREW_PREFIX" in mock_env: + del mock_env["HOMEBREW_PREFIX"] + + with patch.dict(os.environ, mock_env): + with patch( + "salt.modules.cmdmod.run", MagicMock(side_effect=CommandExecutionError) + ), patch( + "salt.modules.mac_brew_pkg._homebrew_os_bin", + MagicMock(return_value=None), + ): + assert mac_brew.homebrew_prefix() is None + + +# '_homebrew_os_bin' function tests: 1 + + +def test_homebrew_os_bin_fallback_apple_silicon(): + """ + Test the path to the homebrew executable for Apple Silicon. + + This test checks that even if the PATH does not contains + the default Homebrew's prefix for the Apple Silicon + architecture, it is appended. + """ + + # Ensure Homebrew's prefix for Apple Silicon is not present in the PATH + mock_env = os.environ.copy() + mock_env["PATH"] = "/usr/local/bin:/usr/bin" + + apple_silicon_homebrew_path = "/opt/homebrew/bin" + apple_silicon_homebrew_bin = f"{apple_silicon_homebrew_path}/brew" + + def mock_utils_path_which(*args): + if apple_silicon_homebrew_path in os.environ.get("PATH", "").split( + os.path.pathsep + ): + return apple_silicon_homebrew_bin + return None + + with patch("salt.utils.path.which", mock_utils_path_which): + assert mac_brew._homebrew_os_bin() == apple_silicon_homebrew_bin + + # '_homebrew_bin' function tests: 1 -def test_homebrew_bin(HOMEBREW_BIN): +def test_homebrew_bin(HOMEBREW_PREFIX, HOMEBREW_BIN): """ Tests the path to the homebrew binary """ - mock_path = MagicMock(return_value="/usr/local") - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): - with patch.dict(mac_brew.__salt__, {"cmd.run": mock_path}): - assert mac_brew._homebrew_bin() == HOMEBREW_BIN + mock_path = MagicMock(return_value=HOMEBREW_PREFIX) + with patch("salt.modules.mac_brew_pkg.homebrew_prefix", mock_path): + assert mac_brew._homebrew_bin() == HOMEBREW_BIN # 'list_pkgs' function tests: 2 @@ -624,7 +737,9 @@ def test_hold(HOMEBREW_BIN): } mock_params = MagicMock(return_value=({"foo": None}, "repository")) - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch( "salt.modules.mac_brew_pkg.list_pkgs", return_value={"foo": "0.1.5"} ), patch.dict( @@ -658,7 +773,9 @@ def test_hold_not_installed(HOMEBREW_BIN): } mock_params = MagicMock(return_value=({"foo": None}, "repository")) - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch("salt.modules.mac_brew_pkg.list_pkgs", return_value={}), patch.dict( mac_brew.__salt__, { @@ -728,7 +845,9 @@ def test_unhold(HOMEBREW_BIN): } mock_params = MagicMock(return_value=({"foo": None}, "repository")) - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch( "salt.modules.mac_brew_pkg.list_pkgs", return_value={"foo": "0.1.5"} ), patch( @@ -876,7 +995,9 @@ def test_info_installed(HOMEBREW_BIN): }, } - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch("salt.modules.mac_brew_pkg.list_pkgs", return_value={}), patch( "salt.modules.mac_brew_pkg._list_pinned", return_value=["foo"] ), patch.dict( @@ -943,7 +1064,9 @@ def test_list_upgrades(HOMEBREW_BIN): "ksdiff": "2.3.6,123-jan-18-2021", } - with patch("salt.utils.path.which", MagicMock(return_value=HOMEBREW_BIN)): + with patch( + "salt.modules.mac_brew_pkg._homebrew_bin", MagicMock(return_value=HOMEBREW_BIN) + ): with patch("salt.modules.mac_brew_pkg.list_pkgs", return_value={}), patch( "salt.modules.mac_brew_pkg._list_pinned", return_value=["foo"] ), patch.dict( From e929cd83ff5f12cec57c5bf8117bcf4bafe4e121 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Sun, 6 Aug 2023 10:05:34 +0200 Subject: [PATCH 3/7] feat(rsax931): Use mac_brew_pkg.homebrew_prefix Uses the homebrew_prefix method to get the Homebrew's prefix if Homebrew is installed on the machine --- salt/utils/rsax931.py | 18 +++++++++++------- tests/pytests/unit/utils/test_rsax931.py | 9 +++++---- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/salt/utils/rsax931.py b/salt/utils/rsax931.py index fb8a1cbbd74e..6ff29d2df85b 100644 --- a/salt/utils/rsax931.py +++ b/salt/utils/rsax931.py @@ -44,13 +44,17 @@ def _find_libcrypto(): lib = lib or glob.glob("lib/libcrypto.dylib") # Find library symlinks in Homebrew locations. - brew_prefix = os.getenv("HOMEBREW_PREFIX", "/usr/local") - lib = lib or glob.glob( - os.path.join(brew_prefix, "opt/openssl/lib/libcrypto.dylib") - ) - lib = lib or glob.glob( - os.path.join(brew_prefix, "opt/openssl@*/lib/libcrypto.dylib") - ) + import salt.modules.mac_brew_pkg as mac_brew + + brew_prefix = mac_brew.homebrew_prefix(quiet=True) + if brew_prefix is not None: + lib = lib or glob.glob( + os.path.join(brew_prefix, "opt/openssl/lib/libcrypto.dylib") + ) + lib = lib or glob.glob( + os.path.join(brew_prefix, "opt/openssl@*/lib/libcrypto.dylib") + ) + # look in macports. lib = lib or glob.glob("/opt/local/lib/libcrypto.dylib") # check if 10.15, regular libcrypto.dylib is just a false pointer. diff --git a/tests/pytests/unit/utils/test_rsax931.py b/tests/pytests/unit/utils/test_rsax931.py index a1c81e653f86..a9f084fccf2d 100644 --- a/tests/pytests/unit/utils/test_rsax931.py +++ b/tests/pytests/unit/utils/test_rsax931.py @@ -247,11 +247,12 @@ def test_glob(pattern): platform, "mac_ver", lambda: ("11.2.2", (), "") ), patch.object(sys, "platform", "macosx"): for package_manager, expected_lib in managed_paths.items(): + mock_env = os.environ.copy() if package_manager == "brew": - env = {"HOMEBREW_PREFIX": "/test/homebrew/prefix"} - else: - env = {"HOMEBREW_PREFIX": ""} - with patch.object(os, "getenv", mock_getenv(env)): + mock_env["HOMEBREW_PREFIX"] = "/test/homebrew/prefix" + elif "HOMEBREW_PREFIX" in mock_env: + del mock_env["HOMEBREW_PREFIX"] + with patch.dict(os.environ, mock_env): with patch.object(glob, "glob", mock_glob(expected_lib)): lib_path = _find_libcrypto() From e81eefe26842c9892785e655c6469a6dd75f003a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Sun, 6 Aug 2023 10:04:17 +0200 Subject: [PATCH 4/7] changelog: Add changelog for PR #64924 --- changelog/64924.fixed.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/64924.fixed.md diff --git a/changelog/64924.fixed.md b/changelog/64924.fixed.md new file mode 100644 index 000000000000..dc6bef558120 --- /dev/null +++ b/changelog/64924.fixed.md @@ -0,0 +1,7 @@ +Fix the way Salt tries to get the Homebrew's prefix + +The first attempt to get the Homebrew's prefix is to look for +the `HOMEBREW_PREFIX` environment variable. If it's not set, then +Salt tries to get the prefix from the `brew` command. However, the +`brew` command can failed. So a last attempt is made to get the +prefix by guessing the installation path. From 751c1a0fb91270e31b739620348e0c1d12c9e746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Thu, 14 Dec 2023 18:29:46 +0100 Subject: [PATCH 5/7] feat(mac_brew_pkg): Remove some logs --- salt/modules/mac_brew_pkg.py | 22 ++++++---------------- salt/utils/rsax931.py | 2 +- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/salt/modules/mac_brew_pkg.py b/salt/modules/mac_brew_pkg.py index e2377761238e..9166b0f5c880 100644 --- a/salt/modules/mac_brew_pkg.py +++ b/salt/modules/mac_brew_pkg.py @@ -137,9 +137,11 @@ def _homebrew_bin(quiet=False): quiet When ``True``, does not log warnings when the homebrew prefix cannot be found. """ - ret = homebrew_prefix(quiet=quiet) + ret = homebrew_prefix() if ret is not None: ret += "/bin/brew" + else: + log.warning("Failed to find homebrew prefix") return ret @@ -182,13 +184,10 @@ def _list_pkgs_from_context(versions_as_list): return ret -def homebrew_prefix(quiet=False): +def homebrew_prefix(): """ Returns the full path to the homebrew prefix. - quiet - When ``True``, does not log warnings when the homebrew prefix cannot be found. - CLI Example: .. code-block:: bash @@ -201,8 +200,6 @@ def homebrew_prefix(quiet=False): if env_homebrew_prefix in os.environ: log.debug(f"{env_homebrew_prefix} is set. Using it for homebrew prefix.") return os.environ[env_homebrew_prefix] - elif not quiet: - log.warning(f"{env_homebrew_prefix} is not set. Please, consider adding it.") # Try brew --prefix try: @@ -221,15 +218,8 @@ def homebrew_prefix(quiet=False): return ret except CommandExecutionError as e: - if not quiet: - log.warning( - f"Unable to find homebrew prefix by running 'brew --prefix'. Error: {str(e)}" - ) - - # Unable to find brew prefix - if not quiet: - log.warning( - f"Failed to find homebrew prefix. Please, set {env_homebrew_prefix} env variable" + log.debug( + f"Unable to find homebrew prefix by running 'brew --prefix'. Error: {str(e)}" ) return None diff --git a/salt/utils/rsax931.py b/salt/utils/rsax931.py index 6ff29d2df85b..d84995e3cffb 100644 --- a/salt/utils/rsax931.py +++ b/salt/utils/rsax931.py @@ -46,7 +46,7 @@ def _find_libcrypto(): # Find library symlinks in Homebrew locations. import salt.modules.mac_brew_pkg as mac_brew - brew_prefix = mac_brew.homebrew_prefix(quiet=True) + brew_prefix = mac_brew.homebrew_prefix() if brew_prefix is not None: lib = lib or glob.glob( os.path.join(brew_prefix, "opt/openssl/lib/libcrypto.dylib") From e2dc30de077eb213f1334bcc9ff34fad0c57c5ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Thu, 14 Dec 2023 21:44:39 +0100 Subject: [PATCH 6/7] test(mac_brew_pkg): Update tests for Apple Silicon --- salt/modules/mac_brew_pkg.py | 46 +++++++++---------- .../pytests/unit/modules/test_mac_brew_pkg.py | 10 ++-- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/salt/modules/mac_brew_pkg.py b/salt/modules/mac_brew_pkg.py index 9166b0f5c880..ff20b3f483f9 100644 --- a/salt/modules/mac_brew_pkg.py +++ b/salt/modules/mac_brew_pkg.py @@ -36,11 +36,11 @@ def __virtual__(): """ - Confine this module to Mac OS with Homebrew. + Confine this module to macOS with Homebrew. """ if __grains__["os"] != "MacOS": return False, "brew module is macos specific" - if not _homebrew_bin(quiet=False): + if not _homebrew_bin(): return False, "The 'brew' binary was not found" return __virtualname__ @@ -107,22 +107,21 @@ def _homebrew_os_bin(): Fetch PATH binary brew full path eg: /usr/local/bin/brew (symbolic link) """ - # Add "/opt/homebrew" temporary to the PATH for Apple Silicon if - # the PATH does not include "/opt/homebrew" - original_path = None - current_path = os.environ.get("PATH", "") - homebrew_path = "/opt/homebrew/bin" - if homebrew_path not in current_path.split(os.path.pathsep): - original_path = current_path - extended_path = os.path.pathsep.join([original_path, homebrew_path]) - os.environ["PATH"] = extended_path.lstrip(os.path.pathsep) - - # Search for the brew executable in the current PATH - brew = salt.utils.path.which("brew") - - # Restore the original PATH if needed - if original_path is not None: - if original_path == "": + original_path = os.environ.get("PATH") + try: + # Add "/opt/homebrew" temporary to the PATH for Apple Silicon if + # the PATH does not include "/opt/homebrew" + current_path = original_path or "" + homebrew_path = "/opt/homebrew/bin" + if homebrew_path not in current_path.split(os.path.pathsep): + extended_path = os.path.pathsep.join([current_path, homebrew_path]) + os.environ["PATH"] = extended_path.lstrip(os.path.pathsep) + + # Search for the brew executable in the current PATH + brew = salt.utils.path.which("brew") + finally: + # Restore original PATH + if original_path is None: del os.environ["PATH"] else: os.environ["PATH"] = original_path @@ -130,12 +129,9 @@ def _homebrew_os_bin(): return brew -def _homebrew_bin(quiet=False): +def _homebrew_bin(): """ Returns the full path to the homebrew binary in the homebrew installation folder - - quiet - When ``True``, does not log warnings when the homebrew prefix cannot be found. """ ret = homebrew_prefix() if ret is not None: @@ -150,7 +146,7 @@ def _call_brew(*cmd, failhard=True): """ Calls the brew command with the user account of brew """ - brew_exec = _homebrew_bin(quiet=True) + brew_exec = _homebrew_bin() user = __salt__["file.get_user"](brew_exec) runas = user if user != __opts__["user"] else None @@ -195,13 +191,13 @@ def homebrew_prefix(): salt '*' pkg.homebrew_prefix """ - # Try HOMEBREW_PREFIX env variable + # If HOMEBREW_PREFIX env variable is present, use it env_homebrew_prefix = "HOMEBREW_PREFIX" if env_homebrew_prefix in os.environ: log.debug(f"{env_homebrew_prefix} is set. Using it for homebrew prefix.") return os.environ[env_homebrew_prefix] - # Try brew --prefix + # Try brew --prefix otherwise try: log.debug("Trying to find homebrew prefix by running 'brew --prefix'") diff --git a/tests/pytests/unit/modules/test_mac_brew_pkg.py b/tests/pytests/unit/modules/test_mac_brew_pkg.py index 3a1e007aac72..e824ba175dfb 100644 --- a/tests/pytests/unit/modules/test_mac_brew_pkg.py +++ b/tests/pytests/unit/modules/test_mac_brew_pkg.py @@ -481,7 +481,7 @@ def test_homebrew_prefix_command(HOMEBREW_PREFIX, HOMEBREW_BIN): def test_homebrew_prefix_returns_none(): """ - Tets that homebrew_prefix returns None when + Tests that homebrew_prefix returns None when all attempts fail. """ @@ -489,7 +489,7 @@ def test_homebrew_prefix_returns_none(): if "HOMEBREW_PREFIX" in mock_env: del mock_env["HOMEBREW_PREFIX"] - with patch.dict(os.environ, mock_env): + with patch.dict(os.environ, mock_env, clear=True): with patch( "salt.modules.mac_brew_pkg._homebrew_os_bin", MagicMock(return_value=None) ): @@ -498,7 +498,7 @@ def test_homebrew_prefix_returns_none(): def test_homebrew_prefix_returns_none_even_with_execution_errors(): """ - Tets that homebrew_prefix returns None when + Tests that homebrew_prefix returns None when all attempts fail even with command execution errors. """ @@ -506,7 +506,7 @@ def test_homebrew_prefix_returns_none_even_with_execution_errors(): if "HOMEBREW_PREFIX" in mock_env: del mock_env["HOMEBREW_PREFIX"] - with patch.dict(os.environ, mock_env): + with patch.dict(os.environ, mock_env, clear=True): with patch( "salt.modules.cmdmod.run", MagicMock(side_effect=CommandExecutionError) ), patch( @@ -523,7 +523,7 @@ def test_homebrew_os_bin_fallback_apple_silicon(): """ Test the path to the homebrew executable for Apple Silicon. - This test checks that even if the PATH does not contains + This test checks that even if the PATH does not contain the default Homebrew's prefix for the Apple Silicon architecture, it is appended. """ From 972c10f19aa3e2a557c9e78ea376dc99a3e265e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Mon, 18 Dec 2023 23:03:18 +0100 Subject: [PATCH 7/7] fix: Fix typo in changelog --- changelog/64924.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/64924.fixed.md b/changelog/64924.fixed.md index dc6bef558120..a843345813db 100644 --- a/changelog/64924.fixed.md +++ b/changelog/64924.fixed.md @@ -3,5 +3,5 @@ Fix the way Salt tries to get the Homebrew's prefix The first attempt to get the Homebrew's prefix is to look for the `HOMEBREW_PREFIX` environment variable. If it's not set, then Salt tries to get the prefix from the `brew` command. However, the -`brew` command can failed. So a last attempt is made to get the +`brew` command can fail. So a last attempt is made to get the prefix by guessing the installation path.