Further fixes to external program version testing

docs/advanced.rst

@@ -85,7 +85,6 @@ For example, if you have a development build of Tesseract don't wish to use the
 
 In this example ``TESSDATA_PREFIX`` is required to redirect Tesseract to an alternate folder for its "tessdata" files.
 
-
 Overriding other support programs
 """""""""""""""""""""""""""""""""
 

docs/batch.rst

@@ -80,9 +80,9 @@ This user contributed script also provides an example of batch processing.
 	            print(full_path)
 	            cmd = ["ocrmypdf",  "--deskew", filename, filename]
 	            logging.info(cmd)
-	            proc = subprocess.Popen(
+	            proc = subprocess.run(
 	            	cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
-	            result = proc.stdout.read()
+	            result = proc.stdout
 	            if proc.returncode == 6:
 	                print("Skipped document because it already contained text")
 	            elif proc.returncode == 0:
@@ -151,7 +151,7 @@ This is only possible for x86-based Synology products. Some Synology products us
 	# the script is processed as root user via chron
 	            cmd = ['docker', 'run', '--rm', '-v', docker_mount, '-u=1030:65538', 'jbarlow83/ocrmypdf', , '--deskew' , filename, filename_OCR]
 	            logging.info(cmd)
-	            proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+	            proc = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
 	            result = proc.stdout.read()
 	            logging.info(result)
 	            full_path_OCR = dir_name + '/' + filename_OCR

docs/release_notes.rst

@@ -13,6 +13,18 @@ Note that it is licensed under GPLv3, so scripts that ``import ocrmypdf`` and ar
    find:    [^`]\#([0-9]{1,3})[^0-9]
    replace: `#$1 <https://github.com/jbarlow83/OCRmyPDF/issues/$1>`_
 
+v8.2.2
+------
+
+-   Fixed a regression from v8.2.0, an exception that occurred while attempting to report that ``unpaper`` or another optional dependency was unavailable.
+
+-   In some cases, ``ocrmypdf [-c|--clean]`` failed to exit with an error when ``unpaper`` is not installed.
+
+v8.2.1
+------
+
+-   This release was canceled.
+
 v8.2.0
 ------
 

src/ocrmypdf/__main__.py

@@ -46,6 +46,7 @@ from .exceptions import (
 )
 from .exec import (
     ghostscript,
+    jbig2enc,
     qpdf,
     tesseract,
     check_external_program,
@@ -602,43 +603,19 @@ def check_options_sidecar(options, log):
         options.sidecar = options.output_file + '.txt'
 
 
-def _optional_program_required(name, version_fn, min_version, for_argument):
-    try:
-        if version_fn() < min_version:
-            raise MissingDependencyError(
-                f"The installed '{name}' is not supported. "
-                f"Install version {min_version} or newer."
-            )
-    except (FileNotFoundError, MissingDependencyError):
-        raise MissingDependencyError(
-            f"Install the '{name}' program to use {for_argument}."
-        )
-
-
-def _optional_program_recommended(name, version_fn, min_version, for_argument):
-    try:
-        if version_fn() < min_version:
-            raise MissingDependencyError(
-                f"The installed '{name}' is not supported. "
-                f"Install version {min_version} or newer."
-            )
-    except (FileNotFoundError, MissingDependencyError):
-        complain(
-            f"For best results, install the optional program '{name}' to use the "
-            f"argument {for_argument}."
-        )
-
-
 def check_options_preprocessing(options, log):
     if options.clean_final:
         options.clean = True
     if options.unpaper_args and not options.clean:
         raise argparse.ArgumentError(None, "--clean is required for --unpaper-args")
-    if any((options.clean, options.clean_final)):
-        from .exec import unpaper
-
-        _optional_program_required(
-            'unpaper', unpaper.version, '6.1', '--clean, --clean-final'
+    if options.clean:
+        check_external_program(
+            log=log,
+            program='unpaper',
+            package='unpaper',
+            version_checker=unpaper.version,
+            need_version='6.1',
+            required_for=['--clean, --clean-final'],
         )
         try:
             if options.unpaper_args:
@@ -664,19 +641,26 @@ def check_options_ocr_behavior(options, log):
 
 def check_options_optimizing(options, log):
     if options.optimize >= 2:
-        from .exec import pngquant, jbig2enc
-
-        _optional_program_required(
-            'pngquant', pngquant.version, '2.0.1', '--optimize {2,3}'
+        check_external_program(
+            log=log,
+            program='pngquant',
+            package='pngquant',
+            version_checker=pngquant.version,
+            need_version='2.0.1',
+            required_for='--optimize {2,3}',
         )
 
-    if options.jbig2_lossy:
-        _optional_program_required('jbig2', jbig2enc.version, '0.28', '--jbig2-lossy')
-    elif options.optimize >= 2:
+    if options.optimize >= 2:
         # Although we use JBIG2 for optimize=1, don't nag about it unless the
         # user is asking for more optimization
-        _optional_program_recommended(
-            'jbig2', jbig2enc.version, '0.28', '--optimize {2,3}'
+        check_external_program(
+            log=log,
+            program='jbig2',
+            package='jbig2enc',
+            version_checker=jbig2enc.version,
+            need_version='0.28',
+            required_for='--optimize {2,3} | --jbig2-lossy',
+            recommended=True if not options.jbig2_lossy else False,
         )
 
     if options.optimize == 0 and any(
@@ -1027,7 +1011,7 @@ def report_output_file_size(options, _log, input_file, output_file):
     )
 
 
-def check_dependency_versions(log):
+def check_dependency_versions(options, log):
     check_external_program(
         log=log,
         program='tesseract',
@@ -1051,27 +1035,10 @@ def check_dependency_versions(log):
         return ExitCode.missing_dependency
     check_external_program(
         log=log,
-        program='unpaper',
-        package='unpaper',
-        version_checker=unpaper.version,
-        need_version='6.1',  # latest sane version
-        optional=True,
-    )
-    if os.environ.get('TRAVIS') != 'true':  # Suppress for Ubuntu trusty
-        check_external_program(
-            log=log,
-            program='qpdf',
-            package='qpdf',
-            version_checker=qpdf.version,
-            need_version='8.0.2',
-        )
-    check_external_program(
-        log=log,
-        program='pngquant',
-        package='pngquant',
-        version_checker=pngquant.version,
-        need_version='2.0.0',
-        optional=True,
+        program='qpdf',
+        package='qpdf',
+        version_checker=qpdf.version,
+        need_version='8.0.2',
     )
 
 
@@ -1091,7 +1058,7 @@ def run_pipeline(args=None):
     )
     preamble(_log)
     check_options(options, _log)
-    check_dependency_versions(_log)
+    check_dependency_versions(options, _log)
 
     # Any changes to options will not take effect for options that are already
     # bound to function parameters in the pipeline. (For example

src/ocrmypdf/_unicodefun.py

@@ -58,9 +58,9 @@ def verify_python3_env():  # pragma: no cover
     if os.name == 'posix':
         import subprocess
 
-        rv = subprocess.Popen(
+        rv = subprocess.run(
             ['locale', '-a'], stdout=subprocess.PIPE, stderr=subprocess.PIPE
-        ).communicate()[0]
+        ).stdout
         good_locales = set()
         has_c_utf8 = False
 

src/ocrmypdf/exec/__init__.py

@@ -21,7 +21,7 @@ import os
 import re
 import sys
 from subprocess import run, STDOUT, PIPE, CalledProcessError
-from ..exceptions import MissingDependencyError
+from ..exceptions import MissingDependencyError, ExitCode
 from collections.abc import Mapping
 
 
@@ -43,7 +43,7 @@ def get_version(program, *, version_arg='--version', regex=r'(\d+(\.\d+)*)'):
             f"Could not find program '{program}' on the PATH"
         ) from e
     except CalledProcessError as e:
-        if e.returncode < 0:
+        if e.returncode != 0:
             raise MissingDependencyError(
                 f"Ran program '{program}' but it exited with an error:\n{e.output}"
             ) from e
@@ -66,10 +66,17 @@ The program '{program}' could not be executed or was not found on your
 system PATH.
 '''
 
-unknown_version = '''
-OCRmyPDF requires '{program}' {need_version} or higher.  Your system has
-'{program}' but we cannot tell what version is installed.  Contact the
-package maintainer.
+missing_optional_program = '''
+The program '{program}' could not be executed or was not found on your
+system PATH.  This program is required when you use the
+{required_for} arguments.  You could try omitting these arguments, or install
+the package.
+'''
+
+missing_recommend_program = '''
+The program '{program}' could not be executed or was not found on your
+system PATH.  This program is recommended when using the {required_for} arguments,
+but not required, so we will proceed.  For best results, install the program.
 '''
 
 old_version = '''
@@ -77,20 +84,15 @@ OCRmyPDF requires '{program}' {need_version} or higher.  Your system appears
 to have {found_version}.  Please update this program.
 '''
 
-okay_its_optional = '''
-This program is OPTIONAL, so installation of OCRmyPDF can proceed, but
-some functionality may be missing.
-'''
-
-not_okay_its_required = '''
-This program is REQUIRED for OCRmyPDF to work.  Installation will abort.
+old_version_required_for = '''
+OCRmyPDF requires '{program}' {need_version} or higher when run with the
+{required_for} arguments.  If you omit these arguments, OCRmyPDF may be able to
+proceed.  For best results, install the program.
 '''
 
 osx_install_advice = '''
 If you have homebrew installed, try these command to install the missing
-packages:
-    brew update
-    brew upgrade
+package:
     brew install {package}
 '''
 
@@ -105,7 +107,7 @@ installing the RPM for {program}.
 '''
 
 
-def get_platform():
+def _get_platform():
     if sys.platform.startswith('freebsd'):
         return 'freebsd'
     elif sys.platform.startswith('linux'):
@@ -113,48 +115,59 @@ def get_platform():
     return sys.platform
 
 
-def _error_trailer(log, program, package, optional, **kwargs):
-    if optional:
-        log.error(okay_its_optional.format(**locals()), file=sys.stderr)
-    else:
-        log.error(not_okay_its_required.format(**locals()), file=sys.stderr)
-
+def _error_trailer(log, program, package, **kwargs):
     if isinstance(package, Mapping):
-        package = package[get_platform()]
+        package = package[_get_platform()]
 
-    if get_platform() == 'darwin':
-        log.error(osx_install_advice.format(**locals()), file=sys.stderr)
-    elif get_platform() == 'linux':
-        log.error(linux_install_advice.format(**locals()), file=sys.stderr)
+    if _get_platform() == 'darwin':
+        log.info(osx_install_advice.format(**locals()))
+    elif _get_platform() == 'linux':
+        log.info(linux_install_advice.format(**locals()))
 
 
-def error_missing_program(log, program, package, optional):
-    log.error(missing_program.format(**locals()), file=sys.stderr)
-    _error_trailer(log, **locals())
+def _error_missing_program(log, program, package, required_for, recommended):
+    if required_for:
+        log.error(missing_optional_program.format(**locals()))
+    elif recommended:
+        log.info(missing_recommend_program.format(**locals()))
+    else:
+        log.error(missing_program.format(**locals()))
+    _error_trailer(**locals())
 
 
-def error_unknown_version(log, program, package, optional, need_version):
-    log.error(unknown_version.format(**locals()), file=sys.stderr)
-    _error_trailer(log, **locals())
-
-
-def error_old_version(log, program, package, optional, need_version, found_version):
-    log.error(old_version.format(**locals()), file=sys.stderr)
-    _error_trailer(log, **locals())
+def _error_old_version(
+    log, program, package, need_version, found_version, required_for
+):
+    if required_for:
+        log.error(old_version_required_for.format(**locals()))
+    else:
+        log.error(old_version.format(**locals()))
+    _error_trailer(**locals())
 
 
 def check_external_program(
-    log, program, package, version_checker, need_version, optional=False
+    *,
+    log,
+    program,
+    package,
+    version_checker,
+    need_version,
+    required_for=None,
+    recommended=False,
 ):
     try:
         found_version = version_checker()
     except (CalledProcessError, FileNotFoundError, MissingDependencyError):
-        error_missing_program(log, program, package, optional)
-        if not optional:
-            sys.exit(1)
+        _error_missing_program(log, program, package, required_for, recommended)
+        if not recommended:
+            sys.exit(ExitCode.missing_dependency)
         return
 
     if found_version < need_version:
-        error_old_version(log, program, package, optional, need_version, found_version)
+        _error_old_version(
+            log, program, package, need_version, found_version, required_for
+        )
+        if not recommended:
+            sys.exit(ExitCode.missing_dependency)
 
     log.debug(f'Found {program} {found_version}')

tests/conftest.py

@@ -19,7 +19,7 @@ import os
 import platform
 import sys
 from pathlib import Path
-from subprocess import PIPE, Popen
+from subprocess import PIPE, run
 
 import pytest
 
@@ -72,6 +72,17 @@ def needs_pdfminer(fn):
     return fn
 
 
+@pytest.helpers.register
+def have_unpaper():
+    try:
+        from ocrmypdf.exec import unpaper
+
+        unpaper.version()
+    except Exception:
+        return False
+    return True
+
+
 TESTS_ROOT = os.path.abspath(os.path.dirname(__file__))
 SPOOF_PATH = os.path.join(TESTS_ROOT, 'spoof')
 PROJECT_ROOT = os.path.dirname(TESTS_ROOT)
@@ -171,19 +182,16 @@ def run_ocrmypdf(input_file, output_file, *args, env=None, universal_newlines=Tr
     if env is None:
         env = os.environ
 
-    p_args = OCRMYPDF + [str(arg) for arg in args] + [str(input_file), str(output_file)]
-    p = Popen(
-        p_args,
-        close_fds=True,
-        stdout=PIPE,
-        stderr=PIPE,
-        universal_newlines=universal_newlines,
-        env=env,
+    p_args = (
+        OCRMYPDF
+        + [str(arg) for arg in args if arg is not None]
+        + [str(input_file), str(output_file)]
     )
-    out, err = p.communicate()
-    # print(err)
-
-    return p, out, err
+    p = run(
+        p_args, stdout=PIPE, stderr=PIPE, universal_newlines=universal_newlines, env=env
+    )
+    # print(p.stderr)
+    return p, p.stdout, p.stderr
 
 
 @pytest.helpers.register

tests/test_main.py

@@ -21,14 +21,14 @@ import shutil
 import sys
 from math import isclose
 from pathlib import Path
-from subprocess import DEVNULL, PIPE, Popen
+from subprocess import DEVNULL, PIPE, run, Popen
 
 import PIL
 import pytest
 from PIL import Image
 
 from ocrmypdf.exceptions import ExitCode, MissingDependencyError
-from ocrmypdf.exec import ghostscript, qpdf, tesseract
+from ocrmypdf.exec import ghostscript, qpdf, tesseract, unpaper
 from ocrmypdf.leptonica import Pix
 from ocrmypdf.pdfa import file_claims_pdfa
 from ocrmypdf.pdfinfo import Colorspace, Encoding, PdfInfo
@@ -164,7 +164,7 @@ def test_exotic_image(
     check_ocrmypdf(
         resources / pdf,
         outfile,
-        '-dc',
+        '-dc' if pytest.helpers.have_unpaper() else '-d',
         '-v',
         '1',
         '--output-type',
@@ -282,8 +282,7 @@ def test_maximum_options(
         resources / 'multipage.pdf',
         outpdf,
         '-d',
-        '-c',
-        '-i',
+        '-ci' if pytest.helpers.have_unpaper() else None,
         '-f',
         '-k',
         '--oversample',
@@ -542,7 +541,6 @@ def test_jbig2_passthrough(spoof_tesseract_cache, resources, outpdf):
         'hocr',
         env=spoof_tesseract_cache,
     )
-
     out_pageinfo = PdfInfo(out)
     assert out_pageinfo[0].images[0].enc == Encoding.jbig2
 
@@ -554,16 +552,13 @@ def test_stdin(spoof_tesseract_noop, ocrmypdf_exec, resources, outpdf):
     # Runs: ocrmypdf - output.pdf < testfile.pdf
     with open(input_file, 'rb') as input_stream:
         p_args = ocrmypdf_exec + ['-', output_file]
-        p = Popen(
+        p = run(
             p_args,
-            close_fds=True,
             stdout=PIPE,
             stderr=PIPE,
             stdin=input_stream,
             env=spoof_tesseract_noop,
         )
-        out, err = p.communicate()
-
         assert p.returncode == ExitCode.ok
 
 
@@ -574,16 +569,13 @@ def test_stdout(spoof_tesseract_noop, ocrmypdf_exec, resources, outpdf):
     # Runs: ocrmypdf francais.pdf - > test_stdout.pdf
     with open(output_file, 'wb') as output_stream:
         p_args = ocrmypdf_exec + [input_file, '-']
-        p = Popen(
+        p = run(
             p_args,
-            close_fds=True,
             stdout=output_stream,
             stderr=PIPE,
             stdin=DEVNULL,
             env=spoof_tesseract_noop,
         )
-        out, err = p.communicate()
-
         assert p.returncode == ExitCode.ok
 
     assert qpdf.check(output_file, log=None)
@@ -781,10 +773,10 @@ def test_pagesize_consistency(renderer, resources, outpdf):
         outpdf,
         '--pdf-renderer',
         renderer,
-        '--clean',
+        '--clean' if pytest.helpers.have_unpaper() else None,
         '--deskew',
         '--remove-background',
-        '--clean-final',
+        '--clean-final' if pytest.helpers.have_unpaper() else None,
     )
 
     after_dims = first_page_dimensions(outpdf)
@@ -852,22 +844,21 @@ def test_compression_preserved(
             '-',
             output_file,
         ]
-        p = Popen(
+        p = run(
             p_args,
-            close_fds=True,
             stdout=PIPE,
             stderr=PIPE,
             stdin=input_stream,
+            universal_newlines=True,
             env=spoof_tesseract_noop,
         )
-        out, err = p.communicate()
 
         if im.mode in ('RGBA', 'LA'):
             # If alpha image is input, expect an error
-            assert p.returncode != ExitCode.ok and b'alpha' in err
+            assert p.returncode != ExitCode.ok and 'alpha' in p.stderr
             return
 
-        assert p.returncode == ExitCode.ok, err.decode('utf-8')
+        assert p.returncode == ExitCode.ok, p.stderr
 
     pdfinfo = PdfInfo(output_file)
 
@@ -913,17 +904,15 @@ def test_compression_changed(
             '-',
             output_file,
         ]
-        p = Popen(
+        p = run(
             p_args,
-            close_fds=True,
             stdout=PIPE,
             stderr=PIPE,
             stdin=input_stream,
+            universal_newlines=True,
             env=spoof_tesseract_noop,
         )
-        out, err = p.communicate()
-
-        assert p.returncode == ExitCode.ok, err
+        assert p.returncode == ExitCode.ok, p.stderr
 
     pdfinfo = PdfInfo(output_file)
 

tests/test_tess4.py

@@ -117,10 +117,10 @@ def test_pagesize_consistency_tess4(ensure_tess4, resources, outpdf):
         outpdf,
         '--pdf-renderer',
         'sandwich',
-        '--clean',
+        '--clean' if pytest.helpers.have_unpaper() else None,
         '--deskew',
         '--remove-background',
-        '--clean-final',
+        '--clean-final' if pytest.helpers.have_unpaper() else None,
         env=ensure_tess4,
     )
 

tests/test_userunit.py

@@ -15,20 +15,12 @@
 # You should have received a copy of the GNU General Public License
 # along with OCRmyPDF.  If not, see <http://www.gnu.org/licenses/>.
 
-import logging
-import os
-import shutil
 from math import isclose
-from subprocess import DEVNULL, PIPE, Popen, check_call, check_output
 
-import PyPDF2 as pypdf
 import pytest
 
-from ocrmypdf import leptonica
 from ocrmypdf.exceptions import ExitCode
-from ocrmypdf.exec import ghostscript
-from ocrmypdf.pdfa import file_claims_pdfa
-from ocrmypdf.pdfinfo import Colorspace, Encoding, PdfInfo
+from ocrmypdf.pdfinfo import PdfInfo
 
 check_ocrmypdf = pytest.helpers.check_ocrmypdf
 run_ocrmypdf = pytest.helpers.run_ocrmypdf