Fix installation on Apple silicon Macs #59809
Conversation
Salt fails to install on macOS Big Sur because of this `pyobjc` issue:
ronaldoussoren/pyobjc#333
On an Apple M1 Mac, `cffi` (required by `cryptography`) won't install due to
https://foss.heptapod.net/pypy/cffi/-/issues/479
This is fixed in 1.14.4. A similar change was made to Windows dependencies in
saltstack#59479
This change was tested on an M1 Mac running macOS 11.2.1 against both the
built-in Python 3.8.2 as well as Python 3.9.2 from Homebrew.
When loading `libcrypto`, Salt checks for a Homebrew installation of `openssl`
at Homebrew's default prefix of `/usr/local`. However, on Apple Silicon Macs,
Homebrew's default installation prefix is `/opt/homebrew`. On all platforms,
the prefix is configurable. If Salt doesn't find one of those `libcrypto`s,
it will fall back on the un-versioned `/usr/lib/libcrypto.dylib`, which will
cause the following crash:
Application Specific Information:
/usr/lib/libcrypto.dylib
abort() called
Invalid dylib load. Clients should not load the unversioned libcrypto dylib as it does not have a stable ABI.
This commit checks $HOMEBREW_PREFIX instead of hard-coding `/usr/local`.
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
sagetherage
added
MacOS
tests/unit/utils/test_rsax931.py
Outdated
| if fnmatch.fnmatch(lib_path, i): | ||
| passed = True | ||
| break | ||
| self.assertTrue(passed) |
There was a problem hiding this comment.
suggestion: this test looks like it's simply re-implementing the code it's supposed to be testing. That's not very meaningful.
It would be more meaningful to do something like:
def test_when_homebrew_prefix_is_set_on_mac_lib_crypto_should_use_that_path():
expected_lib_path = "/some/path/to/opt/openssl/lib/libcrypto.dylib"
# patch getenv to return '/some/path/to'. Also probably need to patch glob
lib_path = _find_libcrypto()
assert lib_path == expected_lib_path
I would write a couple of tests - at minimum to cover the two scenarios that add BREW_PREFIX.
It's also preferred to add tests to tests/pytests/. If you need help, you can attend one of the Test Clinics on Twitch and we'll help you get pointed in the right direction.
|
@hl-tam I ported some of the changes from this PR to here: #59828 so it can be included in the Aluminium release. We are hesitant in including the updated versions of the libraries in Aluminium since we are so close to releasing and this has caused us issues in the past when we update libraries right before a release without giving it enough time for testing. We will keep your changes to update the libraries here in master so it can be included in the next release Silicone, but will not make it into Aluminium. Thanks for the PR! |
|
Has anyone tried actually building the packages with these changes....? Sometimes the dependencies can be a little tricky. |
Right now, if `_find_libcrypto` can't find any externally-managed versions of libcrypto, it will fall back on the pre-Catalina un-versioned system libcrypto. This does not exist on Big Sur and it would be better to raise an exception here rather than crashing later when trying to open it.
This commit simplifies the unit tests for _find_libcrypto by mocking out the host's filesystem and testing the common libcrypto installations (brew, ports, etc.) on Big Sur. It simplifies the tests for falling back on system versions of libcrypto on previous versions of macOS.
|
I've added change log entry and rewrote the tests for |
tests/unit/utils/test_rsax931.py
Outdated
| @patch.object(platform, "mac_ver", lambda: ("10.14.2", (), "")) | ||
| @patch.object(glob, "glob", lambda _: []) | ||
| def test_find_libcrypto_with_system_and_not_catalina(self): | ||
| def test_find_libcrypto_with_system_before_catalina(self): | ||
| """ | ||
| Test _find_libcrypto on a Catalina-like macOS host, simulate |
There was a problem hiding this comment.
Can you update the comment to reflect this tests before catalina and not on a catalina macos host.
|
Also it looks like the tests are failing on windows. |
Ah, I see the issue: |
|
I fixed those tests on the freeze branch PR here: 0dd310d I can push that unless you had an idea for a different fix? |
That's exactly what I was thinking, just didn't get around to it. Thanks! |
|
@twangboy are you good with this PR? |
|
@Ch3LL sorry I missed this. With pyobjc upgraded by another PR, that still leaves CFFI as well as the Homebrew environment changes for Apple Silicon. |
|
Oh, never mind, I see that the Homebrew changes and associated tests have already been upstreamed. Installation works for me without issue on top of tree. Thanks for grabbing all of this! |
What does this PR do?
This PR addresses two issues that blocks Salt from being installed on Apple silicon and a third issue that causes
saltto crash on launch.The first two issues are fixed by updating the version pins for
pyobjcandcffi. The crash is fixed by having Salt use theHOMEBREW_PREFIXenvironment variable instead of assuming thatbrewalways installs to/usr/local(the default on Apple silicon Macs is/opt/homebrewand it's configurable on all platforms).What issues does this PR fix or reference?
Fixes: #59808
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
I don't think docs or tests are applicable to this PR. I'm not sure if this counts as a notable change for the change log.
Commits signed with GPG?
No