Skip to content

Commit

Permalink
17661: Improved error messaging when library binary is not found, MIN…
Browse files Browse the repository at this point in the history 
…OR (#17)

- Added check for invalid library_postfix
- Arch validation now takes into account the os
- Added platform/arch/filepath to error messages where appropriate for
additional context
- Updated unit tests
  • Loading branch information
@fulpm
fulpm authored Sep 29, 2023
1 parent ec58c64 commit 9c7dd19
Show file tree
Hide file tree
Showing 2 changed files with 171 additions and 49 deletions.
150 changes: 108 additions & 42 deletions amalgam/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,19 +133,6 @@ def __init__( # noqa: C901
self.trace = None

_logger.debug(f"Loading amalgam library: {self.library_path}")
if not self.library_path.exists():
if library_path:
raise FileNotFoundError(
'No Amalgam library was found at the provided '
'`library_path`. Please check that the path is correct.'
)
else:
raise FileNotFoundError(
'The auto-determined Amalgam binary to use was not found. '
'This could indicate that the combination of operating '
'system, machine architecture and library-postfix is not '
'yet supported.'
)
_logger.debug(f"SBF_DATASTORE enabled: {sbf_datastore_enabled}")
self.amlg = cdll.LoadLibrary(str(self.library_path))
self.set_amlg_flags(sbf_datastore_enabled)
Expand All @@ -154,10 +141,54 @@ def __init__( # noqa: C901
self.op_count = 0
self.load_command_log_entry = None

@classmethod
def _get_allowed_postfixes(cls, library_dir: Path) -> List[str]:
"""
Return list of all library postfixes allowed given library directory.
Parameters
----------
library_dir : Path
The path object to the library directory.
Returns
-------
list of str
The allowed library postfixes.
"""
allowed_postfixes = set()
for file in library_dir.glob("amalgam*"):
postfix = cls._parse_postfix(file.name)
if postfix is not None:
allowed_postfixes.add(postfix)
return list(allowed_postfixes)

@classmethod
def _parse_postfix(cls, filename: str) -> Union[str, None]:
"""
Determine library postfix given a filename.
Parameters
----------
filename : str
The filename to parse.
Returns
-------
str or None
The library postfix of the filename, or None if no postfix.
"""
matches = re.findall(r'-(.+?)\.', filename)
if len(matches) > 0:
return f'-{matches[-1]}'
else:
return None

@classmethod
def _get_library_path(
self,
cls,
library_path: Optional[Union[Path, str]] = None,
library_postfix: Optional[str] = '-mt',
library_postfix: Optional[str] = None,
arch: Optional[str] = None
) -> Tuple[Path, str]:
"""
Expand All @@ -175,7 +206,7 @@ def _get_library_path(
Optional, The library type as specified by a postfix to the word
"amalgam" in the library's filename. E.g., the "-mt" in
`amalgam-mt.dll`.
arch: str, default None
arch : str, default None
Optional, the platform architecture of the embedded Amalgam
library. If not provided, it will be automatically detected.
(Note: arm64_8a architecture must be manually specified!)
Expand All @@ -185,72 +216,107 @@ def _get_library_path(
Path
The path to the appropriate Amalgam shared lib (.dll, .so, .dylib).
str
The library prefix
The library postfix.
"""
if library_postfix and not library_postfix.startswith("-"):
# Library postfix must start with a dash
raise ValueError(
f'The provided `library_postfix` value of "{library_postfix}" '
'must start with a "-".'
)

if library_path:
# Find the library postfix, if one is present in the given
# library_path.
filename = Path(library_path).name
matches = re.findall(r'-(.+?)\.', filename)
if len(matches) > 0:
_library_postfix = f'-{matches[-1]}'
else:
_library_postfix = None
_library_postfix = cls._parse_postfix(filename)
if library_postfix and library_postfix != _library_postfix:
warnings.warn(
'The supplied `library_postfix` does not match the '
'postfix given in `library_path` and will be ignored.',
UserWarning
)
library_postfix = _library_postfix

library_path = Path(library_path).expanduser()

if not library_path.exists():
raise FileNotFoundError(
'No Amalgam library was found at the provided '
f'`library_path`: "{library_path}". Please check that the '
'path is correct.'
)
else:
# No library_path was provided so, auto-determine the correct one
# to use for this running environment. For this, the operating
# system, the machine architecture and postfix are used.
os = platform.system().lower()

arch_supported = False
if not arch:
arch = platform.machine().lower()

if arch == 'x86_64':
arch = 'amd64'
elif arch.startswith('aarch64') or arch.startswith('arm64'):
# see: https://stackoverflow.com/q/45125516/440805
arch = 'arm64'

if os == 'windows':
path_os = 'windows'
path_ext = 'dll'
arch_supported = arch in ['amd64']
elif os == "darwin":
path_os = 'darwin'
path_ext = 'dylib'
arch_supported = arch in ['amd64', 'arm64']
elif os == "linux":
path_os = 'linux'
path_ext = 'so'
arch_supported = arch in ['amd64', 'arm64', 'arm64_8a']
else:
raise RuntimeError(
'Detected an unsupported machine platform type. Please '
'specify the `library_path` to the Amalgam shared library '
'to use with this platform.')

if not arch:
arch = platform.machine().lower()

if arch == 'x86_64':
arch = 'amd64'
elif arch.startswith('aarch64') or arch.startswith('arm64'):
# see: https://stackoverflow.com/q/45125516/440805
arch = 'arm64'
f'Detected an unsupported machine platform type "{os}". '
'Please specify the `library_path` to the Amalgam shared '
'library to use with this platform.')

if arch not in ['amd64', 'arm64', 'arm64_8a']:
if not arch_supported:
raise RuntimeError(
'An unsupported machine architecture was detected or '
'provided. Please specify the `library_path` to the '
'Amalgam shared library to use with this machine '
f'An unsupported machine architecture "{arch}" was '
'detected or provided. Please specify the `library_path` '
'to the Amalgam shared library to use with this machine '
'architecture.')

if not library_postfix:
library_postfix = '-mt'
library_postfix = '-mt' if arch != "arm64_8a" else '-st'

# Default path for Amalgam binary should be at <package_root>/lib
lib_root = Path(Path(__file__).parent, 'lib')

# Build path
library_path = Path(lib_root, path_os, arch,
f'amalgam{library_postfix}.{path_ext}')
dir_path = Path(lib_root, path_os, arch)
filename = f'amalgam{library_postfix}.{path_ext}'
library_path = Path(dir_path, filename)

if not library_path.exists():
# First check if invalid postfix, otherwise show generic error
allowed_postfixes = cls._get_allowed_postfixes(dir_path)
_library_postfix = cls._parse_postfix(filename)
if (
allowed_postfixes and
_library_postfix not in allowed_postfixes
):
raise RuntimeError(
'An unsupported `library_postfix` value of '
f'"{_library_postfix}" was provided. Supported options '
"for your machine's platform and architecture include: "
f'{", ".join(allowed_postfixes)}.'
)
raise FileNotFoundError(
'The auto-determined Amalgam library to use was not found '
f'at "{library_path}". This could indicate that the '
'combination of operating system, machine architecture and '
'library-postfix is not yet supported.'
)

return library_path, library_postfix

Expand Down
70 changes: 63 additions & 7 deletions amalgam/test/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,84 @@ def _factory(*args, **kwargs):
('linux', 'arm64', '', 'lib/linux/arm64/amalgam-mt.so', '-mt'),
('linux', 'arm64', '-st', 'lib/linux/arm64/amalgam-st.so', '-st'),
('windows', 'amd64', '-st', 'lib/windows/amd64/amalgam-st.dll', '-st'),
('windows', 'arm64', '', 'lib/windows/arm64/amalgam-mt.dll', '-mt'),
('windows', 'arm64', '', RuntimeError, '-mt'),
('windows', 'amd64', '-mt', 'lib/windows/amd64/amalgam-mt.dll', '-mt'),
('windows', 'x86_64', '-st', 'lib/windows/amd64/amalgam-st.dll', '-st'),
])
def test_get_library_path_defaults(mocker, amalgam_factory, platform, arch,
postfix, expected_path, expected_postfix):
def test_amalgam_library_path_defaults(
mocker, amalgam_factory, platform, arch, postfix, expected_path,
expected_postfix
):
"""Test Amalgam._get_library_path is valid."""
mocker.patch('amalgam.api.platform.system', return_value=platform)
mocker.patch('amalgam.api.platform.machine', return_value=arch)
mocker.patch('amalgam.api.Amalgam._get_allowed_postfixes', return_value=["-mt", "-st"])

try:
amlg = amalgam_factory(library_path=None, library_postfix=postfix)
except Exception as e:
assert isinstance(e, expected_path), (
f'Expected a RuntimeError, but a {type(e)} was raised instead')
if not isinstance(expected_path, str):
assert isinstance(e, expected_path), (
f'Expected a {type(expected_path)}, but a {type(e)} was raised instead')
else:
raise
else:
assert isinstance(expected_path, str), (
f'Expected {expected_path} to be raised.')
assert str(amlg.library_path).endswith(expected_path)
assert amlg.library_postfix == expected_postfix


@pytest.mark.parametrize('platform, arch, expected_postfix, should_raise', [
('linux', 'arm64', '-mt', False),
('linux', 'arm64_8a', '-st', False),
('linux', 'amd64', '-mt', False),
('linux', 'i386', '', True),
('darwin', 'amd64', '-mt', False),
('darwin', 'arm64', '-mt', False),
('darwin', 'arm64_8a', '', True),
('windows', 'amd64', '-mt', False),
('windows', 'i386', '', True),
('windows', 'arm64', '', True),
('solaris', 'amd64', '', True),
])
def test_get_library_path_arch(mocker, platform, arch, expected_postfix, should_raise):
"""Test Amalgam._get_library_path arch is valid."""
mocker.patch('amalgam.api.Path.exists', return_value=True)
mocker.patch('amalgam.api.platform.system', return_value=platform)

if should_raise:
with pytest.raises(RuntimeError, match="unsupported machine"):
Amalgam._get_library_path(arch=arch)
else:
_, postfix = Amalgam._get_library_path(arch=arch)
assert postfix == expected_postfix


@pytest.mark.parametrize('postfix, allowed, expected_error', [
('', ['-st', '-mt'], None),
('-st', ['-st', '-mt'], None),
('-mt', ['-st', '-mt'], None),
('-omp', ['-mt', '-omp'], None),
('mt', ['-st', '-mt'], ValueError),
('-abc', ['-st', '-mt'], RuntimeError),
])
def test_get_library_path_postfix(mocker, postfix, allowed, expected_error):
"""Test Amalgam._get_library_path postfix is valid."""
mocker.patch('amalgam.api.Path.exists', return_value=False)
mocker.patch('amalgam.api.platform.system', return_value="windows")
mocker.patch('amalgam.api.platform.machine', return_value="amd64")
mocker.patch('amalgam.api.Amalgam._get_allowed_postfixes', return_value=allowed)

if expected_error:
with pytest.raises(expected_error):
Amalgam._get_library_path(library_postfix=postfix)
else:
# Since path.exists is patched to False, we should expect a FileNotFoundError
with pytest.raises(FileNotFoundError):
Amalgam._get_library_path(library_postfix=postfix)


@pytest.mark.parametrize('path_in, postfix_in, postfix, raise_warning', [
('/lib/windows/amd64/amalgam-mt.dll', '', '-mt', False),
('/lib/windows/amd64/amalgam-mt.dll', '-mt', '-mt', False),
Expand Down

0 comments on commit 9c7dd19

Please sign in to comment.