7824: Use logging facility to show error messages.

diff --git a/sdk/python/arvados/commands/get.py b/sdk/python/arvados/commands/get.py
index d762bbd0ef7fc7f5e863e99b54a1fa2f6e2a1f29..67f38c4cbd635c64d4a0c7821ae50310b307de0c 100755 (executable)
--- a/sdk/python/arvados/commands/get.py
+++ b/sdk/python/arvados/commands/get.py
@@ -14,10 +14,7 @@ import arvados.commands._util as arv_cmd
 from arvados._version import __version__
 
 api_client = None
-
-def abort(msg, code=1):
-    print >>sys.stderr, "arv-get:", msg
-    exit(code)
+logger = logging.getLogger('arvados.arv-get')
 
 parser = argparse.ArgumentParser(
     description='Copy data from Keep to a local file or pipe.',
@@ -88,8 +85,8 @@ overwritten. This option causes even devices, sockets, and fifos to be
 skipped.
 """)
 
-def parse_arguments(arguments, logger):
-    args = parser.parse_args()
+def parse_arguments(arguments, stdout, stderr):
+    args = parser.parse_args(arguments)
 
     if args.locator[-1] == os.sep:
         args.r = True
@@ -120,17 +117,16 @@ def parse_arguments(arguments, logger):
     # either going to a named file, or going (via stdout) to something
     # that isn't a tty.
     if (not (args.batch_progress or args.no_progress)
-        and sys.stderr.isatty()
+        and stderr.isatty()
         and (args.destination != '-'
-             or not sys.stdout.isatty())):
+             or not stdout.isatty())):
         args.progress = True
     return args
 
 def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
     global api_client
     
-    logger = logging.getLogger('arvados.arv-get')
-    args = parse_arguments(arguments, logger)
+    args = parse_arguments(arguments, stdout, stderr)
     if api_client is None:
         api_client = arvados.api('v1')
 
@@ -148,16 +144,18 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
                 open_flags |= os.O_EXCL
             try:
                 if args.destination == "-":
-                    sys.stdout.write(reader.manifest_text())
+                    stdout.write(reader.manifest_text())
                 else:
                     out_fd = os.open(args.destination, open_flags)
                     with os.fdopen(out_fd, 'wb') as out_file:
                         out_file.write(reader.manifest_text())
             except (IOError, OSError) as error:
-                abort("can't write to '{}': {}".format(args.destination, error))
+                logger.error("can't write to '{}': {}".format(args.destination, error))
+                return 1
             except (arvados.errors.ApiError, arvados.errors.KeepReadError) as error:
-                abort("failed to download '{}': {}".format(collection, error))
-        sys.exit(0)
+                logger.error("failed to download '{}': {}".format(collection, error))
+                return 1
+        return 0
 
     # Scan the collection. Make an array of (stream, file, local
     # destination filename) tuples, and add up total size to extract.
@@ -177,7 +175,8 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
                         os.path.join(s.stream_name(), f.name)[len(get_prefix)+1:])
                     if (not (args.n or args.f or args.skip_existing) and
                         os.path.exists(dest_path)):
-                        abort('Local file %s already exists.' % (dest_path,))
+                        logger.error('Local file %s already exists.' % (dest_path,))
+                        return 1
             else:
                 if os.path.join(s.stream_name(), f.name) != '.' + get_prefix:
                     continue
@@ -185,7 +184,8 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
             todo += [(s, f, dest_path)]
             todo_bytes += f.size()
     except arvados.errors.NotFoundError as e:
-        abort(e)
+        logger.error(e)
+        return 1
 
     out_bytes = 0
     for s, f, outfilename in todo:
@@ -193,7 +193,7 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
         digestor = None
         if not args.n:
             if outfilename == "-":
-                outfile = sys.stdout
+                outfile = stdout
             else:
                 if args.skip_existing and os.path.exists(outfilename):
                     logger.debug('Local file %s exists. Skipping.', outfilename)
@@ -202,13 +202,15 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
                                    os.path.isdir(outfilename)):
                     # Good thing we looked again: apparently this file wasn't
                     # here yet when we checked earlier.
-                    abort('Local file %s already exists.' % (outfilename,))
+                    logger.error('Local file %s already exists.' % (outfilename,))
+                    return 1
                 if args.r:
                     arvados.util.mkdir_dash_p(os.path.dirname(outfilename))
                 try:
                     outfile = open(outfilename, 'wb')
                 except Exception as error:
-                    abort('Open(%s) failed: %s' % (outfilename, error))
+                    logger.error('Open(%s) failed: %s' % (outfilename, error))
+                    return 1
         if args.hash:
             digestor = hashlib.new(args.hash)
         try:
@@ -220,26 +222,26 @@ def main(arguments=None, stdout=sys.stdout, stderr=sys.stderr):
                         digestor.update(data)
                     out_bytes += len(data)
                     if args.progress:
-                        sys.stderr.write('\r%d MiB / %d MiB %.1f%%' %
-                                         (out_bytes >> 20,
-                                          todo_bytes >> 20,
-                                          (100
-                                           if todo_bytes==0
-                                           else 100.0*out_bytes/todo_bytes)))
+                        stderr.write('\r%d MiB / %d MiB %.1f%%' %
+                                     (out_bytes >> 20,
+                                      todo_bytes >> 20,
+                                      (100
+                                       if todo_bytes==0
+                                       else 100.0*out_bytes/todo_bytes)))
                     elif args.batch_progress:
-                        sys.stderr.write('%s %d read %d total\n' %
-                                         (sys.argv[0], os.getpid(),
-                                          out_bytes, todo_bytes))
+                        stderr.write('%s %d read %d total\n' %
+                                     (sys.argv[0], os.getpid(),
+                                      out_bytes, todo_bytes))
             if digestor:
-                sys.stderr.write("%s  %s/%s\n"
-                                 % (digestor.hexdigest(), s.stream_name(), f.name))
+                stderr.write("%s  %s/%s\n"
+                             % (digestor.hexdigest(), s.stream_name(), f.name))
         except KeyboardInterrupt:
             if outfile and (outfile.fileno() > 2) and not outfile.closed:
                 os.unlink(outfile.name)
             break
 
     if args.progress:
-        sys.stderr.write('\n')
+        stderr.write('\n')
 
 def files_in_collection(c):
     # Sort first by file type, then alphabetically by file path.

diff --git a/sdk/python/arvados/commands/ls.py b/sdk/python/arvados/commands/ls.py
index cff7b55b297d5b7f1e10ba8d49980097e475059c..918ce5eac09ba4e5edb54002d938ec3780eecf0a 100755 (executable)
--- a/sdk/python/arvados/commands/ls.py
+++ b/sdk/python/arvados/commands/ls.py
@@ -4,6 +4,7 @@ from __future__ import print_function
 
 import argparse
 import collections
+import logging
 import sys
 
 import arvados
@@ -34,19 +35,21 @@ def size_formatter(coll_file):
 def name_formatter(coll_file):
     return "{}/{}".format(coll_file.stream_name, coll_file.name)
 
-def main(args, stdout, stderr, api_client=None):
+def main(args, stdout, stderr, api_client=None, logger=None):
     args = parse_args(args)
 
     if api_client is None:
         api_client = arvados.api('v1')
 
+    if logger is None:
+        logger = logging.getLogger('arvados.arv-ls')
+
     try:
         cr = arvados.CollectionReader(args.locator, api_client=api_client,
                                       num_retries=args.retries)
     except (arvados.errors.ArgumentError,
             arvados.errors.NotFoundError) as error:
-        print("arv-ls: error fetching collection: {}".format(error),
-              file=stderr)
+        logger.error("error fetching collection: {}".format(error))
         return 1
 
     formatters = []

diff --git a/sdk/python/tests/test_arv_ls.py b/sdk/python/tests/test_arv_ls.py
index 5064f07d722ee77efc0c8a4f733eaf86d02b8b39..99b551082f8c7399500e9b71f3338050f33fea02 100644 (file)
--- a/sdk/python/tests/test_arv_ls.py
+++ b/sdk/python/tests/test_arv_ls.py
@@ -35,10 +35,10 @@ class ArvLsTestCase(run_test_server.TestCaseWithServers):
         api_client.collections().get().execute.return_value = coll_info
         return coll_info, api_client
 
-    def run_ls(self, args, api_client):
+    def run_ls(self, args, api_client, logger=None):
         self.stdout = io.BytesIO()
         self.stderr = io.BytesIO()
-        return arv_ls.main(args, self.stdout, self.stderr, api_client)
+        return arv_ls.main(args, self.stdout, self.stderr, api_client, logger)
 
     def test_plain_listing(self):
         collection, api_client = self.mock_api_for_manifest(
@@ -76,10 +76,13 @@ class ArvLsTestCase(run_test_server.TestCaseWithServers):
 
     def test_locator_failure(self):
         api_client = mock.MagicMock(name='mock_api_client')
+        error_mock = mock.MagicMock()
+        logger = mock.MagicMock()
+        logger.error = error_mock
         api_client.collections().get().execute.side_effect = (
             arv_error.NotFoundError)
-        self.assertNotEqual(0, self.run_ls([self.FAKE_UUID], api_client))
-        self.assertNotEqual('', self.stderr.getvalue())
+        self.assertNotEqual(0, self.run_ls([self.FAKE_UUID], api_client, logger))
+        self.assertEqual(1, error_mock.call_count)
 
     def test_version_argument(self):
         err = io.BytesIO()