Repository: Diaoul/subliminal (python)

Issue:
Make `rarfile` dependency optional
The `rar` compression is proprietary.

It is only used to read rar of video files, which is very unlikely.

Pull request:
Make rarfile dependency optional
closes #1096

The `rarfile` python module depends on external tools (`unar` or other) and it is not strictly needed. It is used to find video files inside a ".rar" archive. 

With this PR, installing subliminal with `pip install subliminal` does not install the `rarfile` dependency.
To keep the same behavior, install with `pip install subliminal[rar]`.

Patch:
diff --git a/changelog.d/1096.change.rst b/changelog.d/1096.change.rst
new file mode 100644
index 0000000..97dcc83
--- /dev/null
+++ b/changelog.d/1096.change.rst
@@ -0,0 +1,1 @@
+Make `rarfile` an optional dependency, install with subliminal[rar]
diff --git a/pyproject.toml b/pyproject.toml
index 594f3f1..83ffd03 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -45,7 +45,6 @@ dependencies = [
     "knowit>=0.5.5",
     "platformdirs>=3",
     "pysubs2>=1.7",
-    "rarfile>=2.7",
     "requests>=2.0",
     "srt>=3.5",
     "stevedore>=3.0",
@@ -55,6 +54,7 @@ dependencies = [
 # extras
 # https://peps.python.org/pep-0621/#dependencies-optional-dependencies
 [project.optional-dependencies]
+rar = ["rarfile>=2.7"]
 docs = [
     "sphinx",
     "sphinx_rtd_theme>=2",
@@ -64,6 +64,7 @@ docs = [
     "towncrier",
 ]
 tests = [
+    "subliminal[rar]",
     "coverage[toml]>=7",
     "pytest>=6.0",
     "pytest-cov",
diff --git a/src/subliminal/archives.py b/src/subliminal/archives.py
new file mode 100644
index 0000000..21ee5c5
--- /dev/null
+++ b/src/subliminal/archives.py
@@ -0,0 +1,139 @@
+"""Core functions."""
+
+from __future__ import annotations
+
+import logging
+import operator
+import os
+import warnings
+from pathlib import Path
+from zipfile import BadZipfile
+
+from guessit import guessit  # type: ignore[import-untyped]
+
+from .exceptions import ArchiveError
+from .video import VIDEO_EXTENSIONS, Video
+
+logger = logging.getLogger(__name__)
+
+
+try:
+    from rarfile import (  # type: ignore[import-untyped]
+        BadRarFile,
+        Error,
+        NotRarFile,
+        RarCannotExec,
+        RarFile,
+        is_rarfile,
+    )
+
+    #: Supported archive extensions (.rar)
+    ARCHIVE_EXTENSIONS: tuple[str] = ('.rar',)
+
+    #: Supported archive errors
+    ARCHIVE_ERRORS: tuple[Exception] = (ArchiveError, BadZipfile, BadRarFile)  # type: ignore[assignment]
+
+except ImportError:
+    #: Supported archive extensions
+    ARCHIVE_EXTENSIONS: tuple[str] = ()  # type: ignore[no-redef]
+
+    #: Supported archive errors
+    ARCHIVE_ERRORS: tuple[Exception] = (ArchiveError, BadZipfile)  # type: ignore[no-redef]
+
+
+def is_supported_archive(filename: str) -> bool:
+    """Check if an archive format is supported and warn to install additional modules."""
+    if filename.lower().endswith(ARCHIVE_EXTENSIONS):
+        return True
+
+    if filename.lower().endswith('.rar'):
+        msg = 'Install the rarfile module to be able to read rar archives.'
+        warnings.warn(msg, UserWarning, stacklevel=2)
+
+    return False
+
+
+def scan_archive(path: str | os.PathLike, name: str | None = None) -> Video:  # pragma: no cover
+    """Scan an archive from a `path`.
+
+    :param str path: existing path to the archive.
+    :param str name: if defined, name to use with guessit instead of the path.
+    :return: the scanned video.
+    :rtype: :class:`~subliminal.video.Video`
+    :raises: :class:`ArchiveError`: error opening the archive.
+    """
+    path = Path(path)
+
+    # rar
+    if '.rar' in ARCHIVE_EXTENSIONS and path.suffix.lower() == '.rar':
+        try:
+            video = scan_archive_rar(path, name=name)
+        except (Error, NotRarFile, RarCannotExec, ValueError) as e:
+            args = (e.message,) if hasattr(e, 'message') else e.args
+            raise ArchiveError(*args) from e
+
+        return video
+
+    msg = f'{path.suffix!r} is not a valid archive'
+    raise ArchiveError(msg)
+
+
+def scan_archive_rar(path: str | os.PathLike, name: str | None = None) -> Video:  # pragma: no cover
+    """Scan a rar archive from a `path`.
+
+    :param str path: existing path to the archive.
+    :param str name: if defined, name to use with guessit instead of the path.
+    :return: the scanned video.
+    :rtype: :class:`~subliminal.video.Video`
+    :raises: :class:`ValueError`: video path is not well defined.
+    """
+    path = os.fspath(path)
+    # check for non-existing path
+    if not os.path.exists(path):  # pragma: no cover
+        msg = 'Path does not exist'
+        raise ValueError(msg)
+
+    if not is_rarfile(path):
+        msg = f'{os.path.splitext(path)[1]!r} is not a valid archive'
+        raise ValueError(msg)
+
+    dir_path, filename = os.path.split(path)
+
+    logger.info('Scanning archive %r in %r', filename, dir_path)
+
+    # Get filename and file size from RAR
+    rar = RarFile(path)
+
+    # check that the rar doesnt need a password
+    if rar.needs_password():
+        msg = 'Rar requires a password'
+        raise ValueError(msg)
+
+    # raise an exception if the rar file is broken
+    # must be called to avoid a potential deadlock with some broken rars
+    rar.testrar()
+
+    file_infos = [f for f in rar.infolist() if not f.isdir() and f.filename.endswith(VIDEO_EXTENSIONS)]
+
+    # sort by file size descending, the largest video in the archive is the one we want, there may be samples or intros
+    file_infos.sort(key=operator.attrgetter('file_size'), reverse=True)
+
+    # no video found
+    if not file_infos:
+        msg = 'No video in archive'
+        raise ValueError(msg)
+
+    # Free the information about irrelevant files before guessing
+    file_info = file_infos[0]
+
+    # guess
+    video_filename = file_info.filename
+    video_path = os.path.join(dir_path, video_filename)
+
+    repl = name if name else video_path
+    video = Video.fromguess(video_path, guessit(repl))
+
+    # size
+    video.size = file_info.file_size
+
+    return video
diff --git a/src/subliminal/core.py b/src/subliminal/core.py
index f9acc86..f60cc00 100644
--- a/src/subliminal/core.py
+++ b/src/subliminal/core.py
@@ -9,12 +9,12 @@ import os
 from collections import defaultdict
 from concurrent.futures import ThreadPoolExecutor
 from typing import TYPE_CHECKING, Any
-from zipfile import BadZipfile
 
 from babelfish import Language, LanguageReverseError  # type: ignore[import-untyped]
 from guessit import guessit  # type: ignore[import-untyped]
-from rarfile import BadRarFile, Error, NotRarFile, RarCannotExec, RarFile, is_rarfile  # type: ignore[import-untyped]
 
+from .archives import ARCHIVE_ERRORS, ARCHIVE_EXTENSIONS, is_supported_archive, scan_archive
+from .exceptions import ArchiveError
 from .extensions import (
     discarded_episode_refiners,
     discarded_movie_refiners,
@@ -36,10 +36,6 @@ if TYPE_CHECKING:
     from subliminal.providers import Provider
     from subliminal.score import ComputeScore
 
-
-#: Supported archive extensions (.rar)
-ARCHIVE_EXTENSIONS = ('.rar',)
-
 logger = logging.getLogger(__name__)
 
 
@@ -200,7 +196,7 @@ class ProviderPool:
         logger.info('Downloading subtitle %r', subtitle)
         try:
             self[subtitle.provider_name].download_subtitle(subtitle)
-        except (BadZipfile, BadRarFile):  # pragma: no cover
+        except ARCHIVE_ERRORS:  # type: ignore[misc]  # pragma: no cover
             logger.exception('Bad archive for subtitle %r', subtitle)
         except Exception as e:  # noqa: BLE001
             handle_exception(e, f'Discarding provider {subtitle.provider_name}')
@@ -511,67 +507,6 @@ def scan_video(path: str | os.PathLike, name: str | None = None) -> Video:
     return video
 
 
-def scan_archive(path: str | os.PathLike, name: str | None = None) -> Video:  # pragma: no cover
-    """Scan an archive from a `path`.
-
-    :param str path: existing path to the archive.
-    :param str name: if defined, name to use with guessit instead of the path.
-    :return: the scanned video.
-    :rtype: :class:`~subliminal.video.Video`
-    :raises: :class:`ValueError`: video path is not well defined.
-    """
-    path = os.fspath(path)
-    # check for non-existing path
-    if not os.path.exists(path):  # pragma: no cover
-        msg = 'Path does not exist'
-        raise ValueError(msg)
-
-    if not is_rarfile(path):
-        msg = f'{os.path.splitext(path)[1]!r} is not a valid archive'
-        raise ValueError(msg)
-
-    dir_path, filename = os.path.split(path)
-
-    logger.info('Scanning archive %r in %r', filename, dir_path)
-
-    # Get filename and file size from RAR
-    rar = RarFile(path)
-
-    # check that the rar doesnt need a password
-    if rar.needs_password():
-        msg = 'Rar requires a password'
-        raise ValueError(msg)
-
-    # raise an exception if the rar file is broken
-    # must be called to avoid a potential deadlock with some broken rars
-    rar.testrar()
-
-    file_infos = [f for f in rar.infolist() if not f.isdir() and f.filename.endswith(VIDEO_EXTENSIONS)]
-
-    # sort by file size descending, the largest video in the archive is the one we want, there may be samples or intros
-    file_infos.sort(key=operator.attrgetter('file_size'), reverse=True)
-
-    # no video found
-    if not file_infos:
-        msg = 'No video in archive'
-        raise ValueError(msg)
-
-    # Free the information about irrelevant files before guessing
-    file_info = file_infos[0]
-
-    # guess
-    video_filename = file_info.filename
-    video_path = os.path.join(dir_path, video_filename)
-
-    repl = name if name else video_path
-    video = Video.fromguess(video_path, guessit(repl))
-
-    # size
-    video.size = file_info.file_size
-
-    return video
-
-
 def scan_videos(
     path: str | os.PathLike,
     *,
@@ -663,10 +598,10 @@ def scan_videos(
                 except ValueError:  # pragma: no cover
                     logger.exception('Error scanning video')
                     continue
-            elif archives and filename.lower().endswith(ARCHIVE_EXTENSIONS):  # archive
+            elif archives and is_supported_archive(filename):  # archive
                 try:
                     video = scan_archive(filepath, name=name)
-                except (Error, NotRarFile, RarCannotExec, ValueError):  # pragma: no cover
+                except (ArchiveError, ValueError):  # pragma: no cover
                     logger.exception('Error scanning archive')
                     continue
             else:  # pragma: no cover
diff --git a/src/subliminal/exceptions.py b/src/subliminal/exceptions.py
index dd61f19..4b3659b 100644
--- a/src/subliminal/exceptions.py
+++ b/src/subliminal/exceptions.py
@@ -7,6 +7,12 @@ class Error(Exception):
     pass
 
 
+class ArchiveError(Error):
+    """Exception raised by reading an archive."""
+
+    pass
+
+
 class ProviderError(Error):
     """Exception raised by providers."""
 


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index 489f670..9ffce7b 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -19,6 +19,7 @@ from subliminal.core import (
     scan_videos,
     search_external_subtitles,
 )
+from subliminal.exceptions import ArchiveError
 from subliminal.subtitle import Subtitle
 from subliminal.utils import timestamp
 from subliminal.video import Episode, Movie
@@ -285,7 +286,7 @@ def test_scan_archive_invalid_extension(movies: dict[str, Movie], tmpdir, monkey
     monkeypatch.chdir(str(tmpdir))
     movie_name = os.path.splitext(movies['interstellar'].name)[0] + '.mp3'
     tmpdir.ensure(movie_name)
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(ArchiveError) as excinfo:
         scan_archive(movie_name)
     assert str(excinfo.value) == "'.mp3' is not a valid archive"
 
@@ -442,20 +443,20 @@ def test_scan_archive_with_multiple_videos(rar: dict[str, str], mkv: dict[str, s
 
 @unix_platform
 def test_scan_archive_with_no_video(rar: dict[str, str]) -> None:
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(ArchiveError) as excinfo:
         scan_archive(rar['simple'])
     assert excinfo.value.args == ('No video in archive',)
 
 
 @unix_platform
 def test_scan_bad_archive(mkv: dict[str, str]) -> None:
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(ArchiveError) as excinfo:
         scan_archive(mkv['test1'])
     assert excinfo.value.args == ("'.mkv' is not a valid archive",)
 
 
 @unix_platform
 def test_scan_password_protected_archive(rar: dict[str, str]) -> None:
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(ArchiveError) as excinfo:
         scan_archive(rar['pwd-protected'])
     assert excinfo.value.args == ('Rar requires a password',)