diff --git a/changelog/64924.fixed.md b/changelog/64924.fixed.md new file mode 100644 index 000000000000..a843345813db --- /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 fail. So a last attempt is made to get the +prefix by guessing the installation path. diff --git a/salt/modules/mac_brew_pkg.py b/salt/modules/mac_brew_pkg.py index 4b4774532cf0..ff20b3f483f9 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 @@ -27,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_os_bin(): + if not _homebrew_bin(): return False, "The 'brew' binary was not found" return __virtualname__ @@ -97,31 +106,54 @@ def _homebrew_os_bin(): """ Fetch PATH binary brew full path eg: /usr/local/bin/brew (symbolic link) """ - return salt.utils.path.which("brew") + + 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 + + return brew def _homebrew_bin(): """ Returns the full path to the homebrew binary in the homebrew installation folder """ - 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() + if ret is not None: + ret += "/bin/brew" + else: + log.warning("Failed to find homebrew prefix") + + 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() + + 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 +180,47 @@ def _list_pkgs_from_context(versions_as_list): return ret +def homebrew_prefix(): + """ + Returns the full path to the homebrew prefix. + + CLI Example: + + .. code-block:: bash + + salt '*' pkg.homebrew_prefix + """ + + # 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 otherwise + 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: + log.debug( + f"Unable to find homebrew prefix by running 'brew --prefix'. Error: {str(e)}" + ) + + return None + + def list_pkgs(versions_as_list=False, **kwargs): """ List the packages currently installed in a dict:: @@ -657,17 +730,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 +796,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 +807,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 diff --git a/salt/utils/rsax931.py b/salt/utils/rsax931.py index fb8a1cbbd74e..d84995e3cffb 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() + 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/modules/test_mac_brew_pkg.py b/tests/pytests/unit/modules/test_mac_brew_pkg.py index 749c9765cce8..e824ba175dfb 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(): + """ + Tests 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, clear=True): + 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(): + """ + Tests 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, clear=True): + 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 contain + 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( 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()