Handle output to /dev/null or directory (#219)

Previously we threw an exception if the output name was a directory (only after doing OCR) and would trigger a PermissionError on trying to flip permission bits of /dev/null due to shutil.copyfile implementation. Instead of copying file use shutil.copyfileobj which should also respect umask etc.

ocrmypdf/__main__.py

@@ -757,7 +757,7 @@ def run_pipeline():
         elif not is_file_writable(options.output_file):
             _log.error(
                 "Output file location (" + options.output_file + ") " +
-                "is not writable.")
+                "is not a writable file.")
             return ExitCode.file_access_error
 
         manager = JobContextManager()
@@ -804,7 +804,11 @@ def run_pipeline():
 
     if options.flowchart:
         _log.info("Flowchart saved to {}".format(options.flowchart))
-    elif options.output_file != '-':
+    elif os.path.samefile(options.output_file, os.devnull):
+        pass  # Say nothing when sending to dev null
+    elif options.output_file == '-':
+        _log.info("Output sent to stdout")
+    else:
         if options.output_type.startswith('pdfa'):
             pdfa_info = file_claims_pdfa(options.output_file)
             if pdfa_info['pass']:
@@ -817,8 +821,6 @@ def run_pipeline():
         if not qpdf.check(options.output_file, _log):
             _log.warning('Output file: The generated PDF is INVALID')
             return ExitCode.invalid_output_pdf
-    else:
-        _log.info("Output sent to stdout")
 
     pdfinfo = context.get_pdfinfo()
     if options.verbose:

ocrmypdf/helpers.py

@@ -4,6 +4,7 @@
 from functools import partial
 from collections.abc import Iterable
 from contextlib import suppress, contextmanager
+from pathlib import Path
 import sys
 import os
 
@@ -63,19 +64,22 @@ def is_file_writable(test_file):
     can replace it atomically. Before doing the OCR work, make sure
     the location is writable.
     """
-    if os.path.exists(test_file):
+    p = Path(test_file)
+    if p.is_symlink():
+        p = p.resolve()
+    if p.is_file():
         return os.access(
-            test_file, os.W_OK,
+            str(p), os.W_OK,
             effective_ids=(os.access in os.supports_effective_ids))
     else:
         try:
-            fp = open(test_file, 'wb')
+            fp = p.open('wb')
         except OSError as e:
             return False
         else:
             fp.close()
             with suppress(OSError):
-                os.unlink(test_file)
+                p.unlink()
         return True
 
 

ocrmypdf/pipeline.py

@@ -2,6 +2,7 @@
 # © 2016 James R. Barlow: github.com/jbarlow83
 
 from contextlib import suppress
+from shutil import copyfileobj
 import sys
 import os
 import shutil
@@ -1001,12 +1002,16 @@ def copy_final(
     input_file = next((ii for ii in input_files if ii.endswith('.pdf')))
 
     if output_file == '-':
-        from shutil import copyfileobj
         with open(input_file, 'rb') as input_stream:
             copyfileobj(input_stream, sys.stdout.buffer)
             sys.stdout.flush()
     else:
-        shutil.copy(input_file, output_file)
+        # At this point we overwrite the output_file specified by the user
+        # use copyfileobj because then we use open() to create the file and 
+        # get the appropriate umask, ownership, etc.
+        with open(input_file, 'rb') as input_stream, \
+                open(output_file, 'wb') as output_stream:
+            copyfileobj(input_stream, output_stream)
 
 
 def build_pipeline(options, work_folder, log, context):

tests/test_main.py

@@ -1,6 +1,7 @@
 # © 2015-17 James R. Barlow: github.com/jbarlow83
 
 from subprocess import Popen, PIPE, check_output, check_call, DEVNULL
+from pathlib import Path
 import os
 import shutil
 import resource
@@ -1098,3 +1099,38 @@ def test_text_curves(spoof_tesseract_noop, resources, outpdf):
     info = PdfInfo(outpdf)
     assert len(info.pages[0].images) != 0, "force did not rasterize"
     
+
+def test_dev_null(spoof_tesseract_noop, resources):
+    p, out, err = run_ocrmypdf(
+        resources / 'trivial.pdf',
+        os.devnull,
+        '--force-ocr',
+        env=spoof_tesseract_noop
+    )
+    assert p.returncode == 0, "could not send output to /dev/null"
+    assert len(out) == 0, "wrote to stdout"
+
+
+def test_output_is_dir(spoof_tesseract_noop, resources, outdir):
+    p, out, err = run_ocrmypdf(
+        resources / 'trivial.pdf',
+        outdir,
+        '--force-ocr',
+        env=spoof_tesseract_noop
+    )
+    assert p.returncode == ExitCode.file_access_error
+    assert 'is not a writable file' in err
+
+
+def test_output_is_symlink(spoof_tesseract_noop, resources, outdir):
+    sym = Path(outdir / 'this_is_a_symlink')
+    sym.symlink_to(outdir / 'out.pdf')
+    p, out, err = run_ocrmypdf(
+        resources / 'trivial.pdf',
+        sym,
+        '--force-ocr',
+        env=spoof_tesseract_noop
+    )
+    assert p.returncode == ExitCode.ok
+    assert (outdir / 'out.pdf').stat().st_size > 0, 'target file not created'
+