From 7db52c6f67b4cec23eb3e31cce89c931e8d3d4b7 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 1/9] cli: change download-task to regular curl download


            also make 301/302 redirect acceptable by download-build & download-task

---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 0e85b05..bb87725 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -9,7 +9,6 @@ import logging
 import optparse
 import os
 import pprint
-import pycurl
 import random
 import re
 import six
@@ -37,7 +36,7 @@ from koji.util import md5_constructor
 from koji_cli.lib import _, OptionParser, activate_session, parse_arches, \
         _unique_path, _running_in_bg, _progress_callback, watch_tasks, \
         arg_filter, linked_upload, list_task_output_all_volumes, \
-        print_task_headers, print_task_recurse, _format_size, watch_logs, \
+        print_task_headers, print_task_recurse, download_file, watch_logs, \
         error, greetings
 
 
@@ -6455,31 +6454,8 @@ def anon_handle_download_build(options, session, args):
             url = pathinfo.build(info) + '/' + fname
             urls.append((url, os.path.basename(fname)))
 
-    def _progress(download_t, download_d, upload_t, upload_d):
-        if download_t == 0:
-            percent_done = 0.0
-        else:
-            percent_done = float(download_d)/float(download_t)
-        percent_done_str = "%02d%%" % (percent_done * 100)
-        data_done = _format_size(download_d)
-
-        sys.stdout.write("[% -36s] % 4s % 10s\r" % ('='*(int(percent_done * 36)), percent_done_str, data_done))
-        sys.stdout.flush()
-
     for url, relpath in urls:
-        if '/' in relpath:
-            koji.ensuredir(os.path.dirname(relpath))
-        if not suboptions.quiet:
-            print(relpath)
-        c = pycurl.Curl()
-        c.setopt(c.URL, url)
-        c.setopt(c.WRITEDATA, open(relpath, 'wb'))
-        if not (suboptions.quiet or suboptions.noprogress):
-            c.setopt(c.NOPROGRESS, False)
-            c.setopt(c.XFERINFOFUNCTION, _progress)
-        c.perform()
-        if not (suboptions.quiet or suboptions.noprogress):
-            print('')
+        download_file(url, relpath, suboptions.quiet)
 
 
 def anon_handle_download_logs(options, session, args):
@@ -6604,13 +6580,19 @@ def anon_handle_download_logs(options, session, args):
 
 
 def anon_handle_download_task(options, session, args):
-    "[download] Download the output of a build task "
+    "[download] Download the output of a build task"
     usage = _("usage: %prog download-task <task_id>")
     usage += _("\n(Specify the --help global option for a list of other help options)")
     parser = OptionParser(usage=usage)
     parser.add_option("--arch", dest="arches", metavar="ARCH", action="append", default=[],
                       help=_("Only download packages for this arch (may be used multiple times)"))
     parser.add_option("--logs", dest="logs", action="store_true", default=False, help=_("Also download build logs"))
+    parser.add_option("--topurl", metavar="URL", default=options.topurl,
+                      help=_("URL under which Koji files are accessible"))
+    parser.add_option("--noprogress", action="store_true",
+                      help=_("Do not display progress meter"))
+    parser.add_option("-q", "--quiet", action="store_true",
+                      help=_("Suppress output"), default=options.quiet)
 
     (suboptions, args) = parser.parse_args(args)
     if len(args) == 0:
@@ -6622,6 +6604,8 @@ def anon_handle_download_task(options, session, args):
     if len(suboptions.arches) > 0:
         suboptions.arches = ",".join(suboptions.arches).split(",")
 
+    activate_session(session, options)
+
     # get downloadable tasks
 
     base_task = session.getTaskInfo(base_task_id)
@@ -6672,17 +6656,17 @@ def anon_handle_download_task(options, session, args):
                 error(_("Child task %d has not finished yet.") % task_id)
 
     # perform the download
-
     number = 0
+    pathinfo = koji.PathInfo(topdir=suboptions.topurl)
     for (task, filename, volume, new_filename) in downloads:
         number += 1
         if volume not in (None, 'DEFAULT'):
             koji.ensuredir(volume)
             new_filename = os.path.join(volume, new_filename)
-        print(_("Downloading [%d/%d]: %s") % (number, len(downloads), new_filename))
-        output_file = open(new_filename, "wb")
-        output_file.write(session.downloadTaskOutput(task["id"], filename, volume=volume))
-        output_file.close()
+        if '..' in filename:
+            error(_('Invalid file name: %s') % filename)
+        url = '%s/%s/%s' % (pathinfo.work(volume), pathinfo.taskrelpath(task["id"]), filename)
+        download_file(url, new_filename, suboptions.quiet, suboptions.noprogress, len(downloads), number)
 
 
 def anon_handle_wait_repo(options, session, args):
diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py
index 6b3e8a6..6c110ff 100644
--- a/cli/koji_cli/lib.py
+++ b/cli/koji_cli/lib.py
@@ -9,6 +9,7 @@ import socket
 import string
 import sys
 import time
+import pycurl
 from six.moves import range
 
 try:
@@ -470,6 +471,39 @@ def linked_upload(localfile, path, name=None):
         os.umask(old_umask)
 
 
+def download_file(url, relpath, quiet=False, noprogress=False, size=None, num=None):
+    """Download files from remote"""
+    def _progress(download_t, download_d, upload_t, upload_d):
+        if download_t == 0:
+            percent_done = 0.0
+        else:
+            percent_done = float(download_d) / float(download_t)
+        percent_done_str = "%02d%%" % (percent_done * 100)
+        data_done = _format_size(download_d)
+
+        sys.stdout.write("[% -36s] % 4s % 10s\r" % ('=' * (int(percent_done * 36)), percent_done_str, data_done))
+        sys.stdout.flush()
+
+    if '/' in relpath:
+        koji.ensuredir(os.path.dirname(relpath))
+    if not quiet:
+        if size and num:
+            print(_("Downloading [%d/%d]: %s") % (num, size, relpath))
+        else:
+            print(_("Downloading: %s") % relpath)
+    c = pycurl.Curl()
+    c.setopt(c.URL, url)
+    # allow 301/302 redirect
+    c.setopt(pycurl.FOLLOWLOCATION, 1)
+    c.setopt(c.WRITEDATA, open(relpath, 'wb'))
+    if not (quiet or noprogress):
+        c.setopt(c.NOPROGRESS, False)
+        c.setopt(c.XFERINFOFUNCTION, _progress)
+    c.perform()
+    if not quiet:
+        print('')
+
+
 def error(msg=None, code=1):
     if msg:
         sys.stderr.write(msg + "\n")
diff --git a/tests/test_cli/data/list-commands.txt b/tests/test_cli/data/list-commands.txt
index 8e4f5fe..b1e8b40 100644
--- a/tests/test_cli/data/list-commands.txt
+++ b/tests/test_cli/data/list-commands.txt
@@ -86,7 +86,7 @@ build commands:
 download commands:
         download-build            Download a built package
         download-logs             Download a logs for package
-        download-task             Download the output of a build task 
+        download-task             Download the output of a build task
 
 info commands:
         buildinfo                 Print basic information about a build

From d07312ef3c30f2eea5292e7d622a47c71283be38 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 2/9] cli: remove unnecessary continue in download_task


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index bb87725..744d0f1 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -6620,25 +6620,21 @@ def anon_handle_download_task(options, session, args):
         downloadable_tasks.extend(list(filter(check_downloadable, subtasks)))
 
     # get files for download
-
     downloads = []
 
     for task in downloadable_tasks:
         files = list_task_output_all_volumes(session, task["id"])
         for filename in files:
-            if filename.endswith(".log") and suboptions.logs:
-                for volume in files[filename]:
-                    # rename logs, they would conflict
-                    new_filename = "%s.%s.log" % (filename.rstrip(".log"), task["arch"])
-                    downloads.append((task, filename, volume, new_filename))
-                    continue
-
             if filename.endswith(".rpm"):
                 for volume in files[filename]:
                     filearch = filename.split(".")[-2]
                     if len(suboptions.arches) == 0 or filearch in suboptions.arches:
                         downloads.append((task, filename, volume, filename))
-                    continue
+            elif filename.endswith(".log") and suboptions.logs:
+                for volume in files[filename]:
+                    # rename logs, they would conflict
+                    new_filename = "%s.%s.log" % (filename.rstrip(".log"), task["arch"])
+                    downloads.append((task, filename, volume, new_filename))
 
     if len(downloads) == 0:
         error(_("No files for download found."))

From e6e2d5a346af46aaaaaf452a4eeba075a9279a90 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 3/9] unittest for cli:download-task


---

diff --git a/tests/test_cli/test_download_task.py b/tests/test_cli/test_download_task.py
new file mode 100644
index 0000000..53736c0
--- /dev/null
+++ b/tests/test_cli/test_download_task.py
@@ -0,0 +1,359 @@
+from __future__ import absolute_import
+import mock
+from mock import call
+import os
+import six
+import sys
+import unittest
+
+from koji_cli.commands import anon_handle_download_task
+
+progname = os.path.basename(sys.argv[0]) or 'koji'
+
+
+class TestDownloadTask(unittest.TestCase):
+    # Show long diffs in error output...
+    maxDiff = None
+
+    def setUp(self):
+        # Mock out the options parsed in main
+        self.options = mock.MagicMock()
+        self.options.quiet = None
+        self.options.topurl = 'https://topurl'
+        # Mock out the xmlrpc server
+        self.session = mock.MagicMock()
+        self.list_task_output_all_volumes = mock.patch('koji_cli.commands.list_task_output_all_volumes').start()
+        self.ensuredir = mock.patch('koji.ensuredir').start()
+        self.download_file = mock.patch('koji_cli.commands.download_file').start()
+        self.activate_session = mock.patch('koji_cli.commands.activate_session').start()
+        self.stdout = mock.patch('sys.stdout', new_callable=six.StringIO).start()
+        self.stderr = mock.patch('sys.stderr', new_callable=six.StringIO).start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_handle_download_task_single(self):
+        task_id = 123333
+        args = [str(task_id)]
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'buildArch',
+                                                 'arch': 'taskarch',
+                                                 'state': 2}
+        self.list_task_output_all_volumes.return_value = {
+            'somerpm.src.rpm': ['DEFAULT', 'vol1'],
+            'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
+            'somerpm.noarch.rpm': ['vol3'],
+            'somelog.log': ['DEFAULT', 'vol1']}
+        # Run it and check immediate output
+        # args: task_id
+        # expected: success
+        rv = anon_handle_download_task(self.options, self.session, args)
+
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_not_called()
+        self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
+        self.assertEqual(self.download_file.mock_calls, [
+            call('https://topurl/work/tasks/3333/123333/somerpm.src.rpm',
+                 'somerpm.src.rpm', None, None, 5, 1),
+            call('https://topurl/vol/vol1/work/tasks/3333/123333/somerpm.src.rpm',
+                 'vol1/somerpm.src.rpm', None, None, 5, 2),
+            call('https://topurl/work/tasks/3333/123333/somerpm.x86_64.rpm',
+                 'somerpm.x86_64.rpm', None, None, 5, 3),
+            call('https://topurl/vol/vol2/work/tasks/3333/123333/somerpm.x86_64.rpm',
+                 'vol2/somerpm.x86_64.rpm', None, None, 5, 4),
+            call('https://topurl/vol/vol3/work/tasks/3333/123333/somerpm.noarch.rpm',
+                 'vol3/somerpm.noarch.rpm', None, None, 5, 5)])
+        self.assertIsNone(rv)
+
+    def test_handle_download_task_parent(self):
+        task_id = 123333
+        args = [str(task_id), '--arch=noarch,x86_64']
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'build',
+                                                 'arch': 'taskarch',
+                                                 'state': 2}
+        self.session.getTaskChildren.return_value = [{'id': 22222,
+                                                      'method': 'buildArch',
+                                                      'arch': 'noarch',
+                                                      'state': 2},
+                                                     {'id': 33333,
+                                                      'method': 'buildArch',
+                                                      'arch': 'x86_64',
+                                                      'state': 2},
+                                                     {'id': 44444,
+                                                      'method': 'buildArch',
+                                                      'arch': 's390',
+                                                      'state': 2},
+                                                     {'id': 55555,
+                                                      'method': 'tagBuild',
+                                                      'arch': 'noarch',
+                                                      'state': 2}
+                                                     ]
+        self.list_task_output_all_volumes.side_effect = [
+            {'somerpm.src.rpm': ['DEFAULT', 'vol1']},
+            {'somerpm.x86_64.rpm': ['DEFAULT', 'vol2']},
+            {'somerpm.noarch.rpm': ['vol3'],
+             'somelog.log': ['DEFAULT', 'vol1']}]
+        # Run it and check immediate output
+        # args: task_id --arch=noarch,x86_64
+        # expected: success
+        rv = anon_handle_download_task(self.options, self.session, args)
+
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_called_once_with(task_id)
+        self.assertEqual(self.list_task_output_all_volumes.mock_calls, [
+            call(self.session, 22222),
+            call(self.session, 33333),
+            call(self.session, 44444)])
+        self.assertEqual(self.download_file.mock_calls, [
+            call('https://topurl/work/tasks/3333/33333/somerpm.x86_64.rpm',
+                 'somerpm.x86_64.rpm', None, None, 3, 1),
+            call('https://topurl/vol/vol2/work/tasks/3333/33333/somerpm.x86_64.rpm',
+                 'vol2/somerpm.x86_64.rpm', None, None, 3, 2),
+            call('https://topurl/vol/vol3/work/tasks/4444/44444/somerpm.noarch.rpm',
+                 'vol3/somerpm.noarch.rpm', None, None, 3, 3)])
+        self.assertIsNone(rv)
+
+    def test_handle_download_task_log(self):
+        task_id = 123333
+        args = [str(task_id), '--log']
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'buildArch',
+                                                 'arch': 'taskarch',
+                                                 'state': 2}
+        self.list_task_output_all_volumes.return_value = {
+            'somerpm.src.rpm': ['DEFAULT', 'vol1'],
+            'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
+            'somerpm.noarch.rpm': ['vol3'],
+            'somelog.log': ['DEFAULT', 'vol1']}
+        # Run it and check immediate output
+        # args: task_id --log
+        # expected: success
+        rv = anon_handle_download_task(self.options, self.session, args)
+
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_not_called()
+        self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
+        self.assertEqual(self.download_file.mock_calls, [
+            call('https://topurl/work/tasks/3333/123333/somerpm.src.rpm',
+                 'somerpm.src.rpm', None, None, 7, 1),
+            call('https://topurl/vol/vol1/work/tasks/3333/123333/somerpm.src.rpm',
+                 'vol1/somerpm.src.rpm', None, None, 7, 2),
+            call('https://topurl/work/tasks/3333/123333/somerpm.x86_64.rpm',
+                 'somerpm.x86_64.rpm', None, None, 7, 3),
+            call('https://topurl/vol/vol2/work/tasks/3333/123333/somerpm.x86_64.rpm',
+                 'vol2/somerpm.x86_64.rpm', None, None, 7, 4),
+            call('https://topurl/vol/vol3/work/tasks/3333/123333/somerpm.noarch.rpm',
+                 'vol3/somerpm.noarch.rpm', None, None, 7, 5),
+            call('https://topurl/work/tasks/3333/123333/somelog.log',
+                 'some.taskarch.log', None, None, 7, 6),
+            call('https://topurl/vol/vol1/work/tasks/3333/123333/somelog.log',
+                 'vol1/some.taskarch.log', None, None, 7, 7)])
+        self.assertIsNone(rv)
+
+    def test_handle_download_no_download(self):
+        task_id = 123333
+        args = [str(task_id), '--arch=s390,ppc']
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'buildArch',
+                                                 'arch': 'taskarch',
+                                                 'state': 2}
+        self.list_task_output_all_volumes.return_value = {
+            'somerpm.src.rpm': ['DEFAULT', 'vol1'],
+            'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
+            'somerpm.noarch.rpm': ['vol3'],
+            'somelog.log': ['DEFAULT', 'vol1'],
+            'somezip.zip': ['DEFAULT']
+        }
+        # Run it and check immediate output
+        # args: task_id --arch=s390,ppc
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = 'No files for download found.\n'
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_not_called()
+        self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
+        self.download_file.assert_not_called()
+        self.assertEqual(cm.exception.code, 1)
+
+    def test_handle_download_parent_not_finished(self):
+        task_id = 123333
+        args = [str(task_id)]
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'buildArch',
+                                                 'arch': 'taskarch',
+                                                 'state': 3}
+        self.list_task_output_all_volumes.return_value = {
+            'somerpm.src.rpm': ['DEFAULT', 'vol1'],
+            'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
+            'somerpm.noarch.rpm': ['vol3'],
+            'somelog.log': ['DEFAULT', 'vol1'],
+            'somezip.zip': ['DEFAULT']
+        }
+        # Run it and check immediate output
+        # args: task_id
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = 'Task 123333 has not finished yet.\n'
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_not_called()
+        self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
+        self.download_file.assert_not_called()
+        self.assertEqual(cm.exception.code, 1)
+
+    def test_handle_download_child_not_finished(self):
+        task_id = 123333
+        args = [str(task_id)]
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'build',
+                                                 'arch': 'taskarch',
+                                                 'state': 2}
+        self.session.getTaskChildren.return_value = [{'id': 22222,
+                                                      'method': 'buildArch',
+                                                      'arch': 'noarch',
+                                                      'state': 3}]
+        self.list_task_output_all_volumes.return_value = {'somerpm.src.rpm': ['DEFAULT', 'vol1']}
+        # Run it and check immediate output
+        # args: task_id
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = 'Child task 22222 has not finished yet.\n'
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_called_once_with(task_id)
+        self.list_task_output_all_volumes.assert_called_once_with(self.session, 22222)
+        self.download_file.assert_not_called()
+        self.assertEqual(cm.exception.code, 1)
+
+    def test_handle_download_invalid_file_name(self):
+        task_id = 123333
+        args = [str(task_id)]
+        self.session.getTaskInfo.return_value = {'id': task_id,
+                                                 'method': 'buildArch',
+                                                 'arch': 'taskarch',
+                                                 'state': 2}
+        self.list_task_output_all_volumes.return_value = {'somerpm..src.rpm': ['DEFAULT', 'vol1']}
+        # Run it and check immediate output
+        # args: task_id
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = 'Invalid file name: somerpm..src.rpm\n'
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.activate_session.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(task_id)
+        self.session.getTaskChildren.assert_not_called()
+        self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
+        self.download_file.assert_not_called()
+        self.assertEqual(cm.exception.code, 1)
+
+    def test_handle_download_help(self):
+        args = ['--help']
+        # Run it and check immediate output
+        # args: --help
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = """Usage: %s download-task <task_id>
+(Specify the --help global option for a list of other help options)
+
+Options:
+  -h, --help    show this help message and exit
+  --arch=ARCH   Only download packages for this arch (may be used multiple
+                times)
+  --logs        Also download build logs
+  --topurl=URL  URL under which Koji files are accessible
+  --noprogress  Do not display progress meter
+  -q, --quiet   Suppress output
+""" % progname
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = ''
+        self.assertEqual(actual, expected)
+        self.assertEqual(cm.exception.code, 0)
+
+    def test_handle_download_no_task_id(self):
+        args = []
+        # Run it and check immediate output
+        # no args
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = """Usage: %s download-task <task_id>
+(Specify the --help global option for a list of other help options)
+
+%s: error: Please specify a task ID
+""" % (progname, progname)
+        self.assertEqual(actual, expected)
+        self.assertEqual(cm.exception.code, 2)
+
+    def test_handle_download_multi_task_id(self):
+        args = ["123", "456"]
+        # Run it and check immediate output
+        # args: 123 456
+        # expected: failure
+        with self.assertRaises(SystemExit) as cm:
+            anon_handle_download_task(self.options, self.session, args)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = """Usage: %s download-task <task_id>
+(Specify the --help global option for a list of other help options)
+
+%s: error: Only one task ID may be specified
+""" % (progname, progname)
+        self.assertEqual(actual, expected)
+        self.assertEqual(cm.exception.code, 2)
+
+
+if __name__ == '__main__':
+    unittest.main()

From b402051cbf0b89ec51b81957374ea577a7eb23e5 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 4/9] cli: add c.close() in download_file and move inner function outside


---

diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py
index 6c110ff..26bec95 100644
--- a/cli/koji_cli/lib.py
+++ b/cli/koji_cli/lib.py
@@ -473,16 +473,6 @@ def linked_upload(localfile, path, name=None):
 
 def download_file(url, relpath, quiet=False, noprogress=False, size=None, num=None):
     """Download files from remote"""
-    def _progress(download_t, download_d, upload_t, upload_d):
-        if download_t == 0:
-            percent_done = 0.0
-        else:
-            percent_done = float(download_d) / float(download_t)
-        percent_done_str = "%02d%%" % (percent_done * 100)
-        data_done = _format_size(download_d)
-
-        sys.stdout.write("[% -36s] % 4s % 10s\r" % ('=' * (int(percent_done * 36)), percent_done_str, data_done))
-        sys.stdout.flush()
 
     if '/' in relpath:
         koji.ensuredir(os.path.dirname(relpath))
@@ -498,12 +488,25 @@ def download_file(url, relpath, quiet=False, noprogress=False, size=None, num=No
     c.setopt(c.WRITEDATA, open(relpath, 'wb'))
     if not (quiet or noprogress):
         c.setopt(c.NOPROGRESS, False)
-        c.setopt(c.XFERINFOFUNCTION, _progress)
+        c.setopt(c.XFERINFOFUNCTION, _download_progress)
     c.perform()
+    c.close()
     if not quiet:
         print('')
 
 
+def _download_progress(download_t, download_d, upload_t, upload_d):
+    if download_t == 0:
+        percent_done = 0.0
+    else:
+        percent_done = float(download_d) / float(download_t)
+    percent_done_str = "%02d%%" % (percent_done * 100)
+    data_done = _format_size(download_d)
+
+    sys.stdout.write("[% -36s] % 4s % 10s\r" % ('=' * (int(percent_done * 36)), percent_done_str, data_done))
+    sys.stdout.flush()
+
+
 def error(msg=None, code=1):
     if msg:
         sys.stderr.write(msg + "\n")

From 887a08d23241f3f57784e58ee13588835f62f2b4 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 5/9] cli: unittest for download_file


---

diff --git a/tests/test_cli/test_download_file.py b/tests/test_cli/test_download_file.py
new file mode 100644
index 0000000..02ce2ab
--- /dev/null
+++ b/tests/test_cli/test_download_file.py
@@ -0,0 +1,113 @@
+from __future__ import absolute_import
+import mock
+import six
+import shutil
+import tempfile
+import unittest
+
+from koji_cli.lib import download_file, _download_progress
+
+
+class TestDownloadFile(unittest.TestCase):
+    # Show long diffs in error output...
+    maxDiff = None
+
+    def reset_mock(self):
+        self.stdout.seek(0)
+        self.stdout.truncate()
+        # self.curl.reset_mock()
+        self.curlClass.reset_mock()
+
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+        self.filename = self.tempdir + "/filename"
+        self.stdout = mock.patch('sys.stdout', new_callable=six.StringIO).start()
+        self.curlClass = mock.patch('pycurl.Curl', create=True).start()
+        self.curl = self.curlClass.return_value
+
+    def tearDown(self):
+        mock.patch.stopall()
+        shutil.rmtree(self.tempdir)
+
+    def test_handle_download_file_dir(self):
+        with self.assertRaises(IOError) as cm:
+            download_file("http://url", self.tempdir)
+        actual = self.stdout.getvalue()
+        expected = 'Downloading: %s\n' % self.tempdir
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(cm.exception.args, (21, 'Is a directory'))
+        self.curlClass.assert_called_once()
+        self.assertEqual(self.curl.setopt.call_count, 2)
+        self.curl.perform.assert_not_called()
+
+    def test_handle_download_file(self):
+        rv = download_file("http://url", self.filename)
+        actual = self.stdout.getvalue()
+        expected = 'Downloading: %s\n\n' % self.filename
+        self.assertMultiLineEqual(actual, expected)
+        self.curlClass.assert_called_once()
+        self.assertEqual(self.curl.setopt.call_count, 5)
+        self.curl.perform.assert_called_once()
+        self.curl.close.assert_called_once()
+        self.assertIsNone(rv)
+
+    def test_handle_download_file_with_size(self):
+        rv = download_file("http://url", self.filename, size=10, num=8)
+        actual = self.stdout.getvalue()
+        expected = 'Downloading [8/10]: %s\n\n' % self.filename
+        self.assertMultiLineEqual(actual, expected)
+        self.curlClass.assert_called_once()
+        self.assertEqual(self.curl.setopt.call_count, 5)
+        self.curl.perform.assert_called_once()
+        self.curl.close.assert_called_once()
+        self.assertIsNone(rv)
+
+    def test_handle_download_file_quiet_noprogress(self):
+        download_file("http://url", self.filename, quiet=True, noprogress=False)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(self.curl.setopt.call_count, 3)
+
+        self.reset_mock()
+        download_file("http://url", self.filename, quiet=True, noprogress=True)
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(self.curl.setopt.call_count, 3)
+
+        self.reset_mock()
+        download_file("http://url", self.filename, quiet=False, noprogress=True)
+        actual = self.stdout.getvalue()
+        expected = 'Downloading: %s\n\n' % self.filename
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(self.curl.setopt.call_count, 3)
+
+
+class TestDownloadProgress(unittest.TestCase):
+    # Show long diffs in error output...
+    maxDiff = None
+
+    def setUp(self):
+        self.stdout = mock.patch('sys.stdout', new_callable=six.StringIO).start()
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_download_progress(self):
+        _download_progress(0, 0, None, None)
+        _download_progress(1024 * 92, 1024, None, None)
+        _download_progress(1024 * 1024 * 23, 1024 * 1024 * 11, None, None)
+        _download_progress(1024 * 1024 * 1024 * 35, 1024 * 1024 * 1024 * 30, None, None)
+        _download_progress(318921, 318921, None, None)
+        actual = self.stdout.getvalue()
+        expected = '[                                    ]  00%     0.00 B\r' + \
+                   '[                                    ]  01%   1.00 KiB\r' + \
+                   '[=================                   ]  47%  11.00 MiB\r' + \
+                   '[==============================      ]  85%  30.00 GiB\r' + \
+                   '[====================================] 100% 311.45 KiB\r'
+        self.assertMultiLineEqual(actual, expected)
+
+
+if __name__ == '__main__':
+    unittest.main()

From c052b4248a585ac21bef8f627e2c62ca29638870 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 6/9] fix cli unittest: test_download_task.py for py3


---

diff --git a/tests/test_cli/test_download_task.py b/tests/test_cli/test_download_task.py
index 53736c0..d698a4e 100644
--- a/tests/test_cli/test_download_task.py
+++ b/tests/test_cli/test_download_task.py
@@ -15,6 +15,27 @@ class TestDownloadTask(unittest.TestCase):
     # Show long diffs in error output...
     maxDiff = None
 
+    def gen_calls(self, task_output, pattern, blacklist=[], arch=None):
+
+        params = [(k, v) for k, vl in
+                  six.iteritems(task_output)
+                  if k not in blacklist
+                  for v in vl]
+        total = len(params)
+        calls = []
+        for i, (k, v) in enumerate(params):
+            target = k
+            if v == 'DEFAULT':
+                subpath = ''
+            else:
+                subpath = 'vol/%s/' % v
+                target = '%s/%s' % (v, k)
+            url = pattern % (subpath, k)
+            if target.endswith('.log') and arch is not None:
+                target = "%s.%s.log" % (target.rstrip(".log"), arch)
+            calls.append(call(url, target, None, None, total, i + 1))
+        return calls
+
     def setUp(self):
         # Mock out the options parsed in main
         self.options = mock.MagicMock()
@@ -44,6 +65,11 @@ class TestDownloadTask(unittest.TestCase):
             'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
             'somerpm.noarch.rpm': ['vol3'],
             'somelog.log': ['DEFAULT', 'vol1']}
+
+        calls = self.gen_calls(self.list_task_output_all_volumes.return_value,
+                               'https://topurl/%swork/tasks/3333/123333/%s',
+                               ['somelog.log'])
+
         # Run it and check immediate output
         # args: task_id
         # expected: success
@@ -57,17 +83,7 @@ class TestDownloadTask(unittest.TestCase):
         self.session.getTaskInfo.assert_called_once_with(task_id)
         self.session.getTaskChildren.assert_not_called()
         self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
-        self.assertEqual(self.download_file.mock_calls, [
-            call('https://topurl/work/tasks/3333/123333/somerpm.src.rpm',
-                 'somerpm.src.rpm', None, None, 5, 1),
-            call('https://topurl/vol/vol1/work/tasks/3333/123333/somerpm.src.rpm',
-                 'vol1/somerpm.src.rpm', None, None, 5, 2),
-            call('https://topurl/work/tasks/3333/123333/somerpm.x86_64.rpm',
-                 'somerpm.x86_64.rpm', None, None, 5, 3),
-            call('https://topurl/vol/vol2/work/tasks/3333/123333/somerpm.x86_64.rpm',
-                 'vol2/somerpm.x86_64.rpm', None, None, 5, 4),
-            call('https://topurl/vol/vol3/work/tasks/3333/123333/somerpm.noarch.rpm',
-                 'vol3/somerpm.noarch.rpm', None, None, 5, 5)])
+        self.assertListEqual(self.download_file.mock_calls, calls)
         self.assertIsNone(rv)
 
     def test_handle_download_task_parent(self):
@@ -115,7 +131,7 @@ class TestDownloadTask(unittest.TestCase):
             call(self.session, 22222),
             call(self.session, 33333),
             call(self.session, 44444)])
-        self.assertEqual(self.download_file.mock_calls, [
+        self.assertListEqual(self.download_file.mock_calls, [
             call('https://topurl/work/tasks/3333/33333/somerpm.x86_64.rpm',
                  'somerpm.x86_64.rpm', None, None, 3, 1),
             call('https://topurl/vol/vol2/work/tasks/3333/33333/somerpm.x86_64.rpm',
@@ -136,6 +152,10 @@ class TestDownloadTask(unittest.TestCase):
             'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
             'somerpm.noarch.rpm': ['vol3'],
             'somelog.log': ['DEFAULT', 'vol1']}
+
+        calls = self.gen_calls(self.list_task_output_all_volumes.return_value,
+                               'https://topurl/%swork/tasks/3333/123333/%s', arch='taskarch')
+
         # Run it and check immediate output
         # args: task_id --log
         # expected: success
@@ -149,21 +169,7 @@ class TestDownloadTask(unittest.TestCase):
         self.session.getTaskInfo.assert_called_once_with(task_id)
         self.session.getTaskChildren.assert_not_called()
         self.list_task_output_all_volumes.assert_called_once_with(self.session, task_id)
-        self.assertEqual(self.download_file.mock_calls, [
-            call('https://topurl/work/tasks/3333/123333/somerpm.src.rpm',
-                 'somerpm.src.rpm', None, None, 7, 1),
-            call('https://topurl/vol/vol1/work/tasks/3333/123333/somerpm.src.rpm',
-                 'vol1/somerpm.src.rpm', None, None, 7, 2),
-            call('https://topurl/work/tasks/3333/123333/somerpm.x86_64.rpm',
-                 'somerpm.x86_64.rpm', None, None, 7, 3),
-            call('https://topurl/vol/vol2/work/tasks/3333/123333/somerpm.x86_64.rpm',
-                 'vol2/somerpm.x86_64.rpm', None, None, 7, 4),
-            call('https://topurl/vol/vol3/work/tasks/3333/123333/somerpm.noarch.rpm',
-                 'vol3/somerpm.noarch.rpm', None, None, 7, 5),
-            call('https://topurl/work/tasks/3333/123333/somelog.log',
-                 'some.taskarch.log', None, None, 7, 6),
-            call('https://topurl/vol/vol1/work/tasks/3333/123333/somelog.log',
-                 'vol1/some.taskarch.log', None, None, 7, 7)])
+        self.assertListEqual(self.download_file.mock_calls, calls)
         self.assertIsNone(rv)
 
     def test_handle_download_no_download(self):
@@ -180,6 +186,7 @@ class TestDownloadTask(unittest.TestCase):
             'somelog.log': ['DEFAULT', 'vol1'],
             'somezip.zip': ['DEFAULT']
         }
+
         # Run it and check immediate output
         # args: task_id --arch=s390,ppc
         # expected: failure

From c7cf5ae91e852ad450827c8c88b9017d8621e062 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 7/9] cli: add noprogress argument for download_file in download_build


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 744d0f1..1ed92c3 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -6455,7 +6455,7 @@ def anon_handle_download_build(options, session, args):
             urls.append((url, os.path.basename(fname)))
 
     for url, relpath in urls:
-        download_file(url, relpath, suboptions.quiet)
+        download_file(url, relpath, suboptions.quiet, suboptions.noprogress)
 
 
 def anon_handle_download_logs(options, session, args):

From 4cfde28884a0b384ec7e6e4e517a338328327639 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 06:15:44 +0000
Subject: [PATCH 8/9] cli: do not print extra blank line when noprogress passed


---

diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py
index 26bec95..58f0340 100644
--- a/cli/koji_cli/lib.py
+++ b/cli/koji_cli/lib.py
@@ -491,7 +491,7 @@ def download_file(url, relpath, quiet=False, noprogress=False, size=None, num=No
         c.setopt(c.XFERINFOFUNCTION, _download_progress)
     c.perform()
     c.close()
-    if not quiet:
+    if not (quiet or noprogress):
         print('')
 
 
diff --git a/tests/test_cli/test_download_file.py b/tests/test_cli/test_download_file.py
index 02ce2ab..8b8cfdb 100644
--- a/tests/test_cli/test_download_file.py
+++ b/tests/test_cli/test_download_file.py
@@ -79,7 +79,7 @@ class TestDownloadFile(unittest.TestCase):
         self.reset_mock()
         download_file("http://url", self.filename, quiet=False, noprogress=True)
         actual = self.stdout.getvalue()
-        expected = 'Downloading: %s\n\n' % self.filename
+        expected = 'Downloading: %s\n' % self.filename
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(self.curl.setopt.call_count, 3)
 

From 8bf4c4338660678ca9150fb28d4ea1a864b9eec0 Mon Sep 17 00:00:00 2001
From: Yuming Zhu <yzhu@redhat.com>
Date: Aug 03 2017 09:29:43 +0000
Subject: [PATCH 9/9] cli: using PROGRESSFUNCTION before libcurl 7.32.0 in download_file


---

diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py
index 58f0340..4b4a504 100644
--- a/cli/koji_cli/lib.py
+++ b/cli/koji_cli/lib.py
@@ -487,8 +487,15 @@ def download_file(url, relpath, quiet=False, noprogress=False, size=None, num=No
     c.setopt(pycurl.FOLLOWLOCATION, 1)
     c.setopt(c.WRITEDATA, open(relpath, 'wb'))
     if not (quiet or noprogress):
-        c.setopt(c.NOPROGRESS, False)
-        c.setopt(c.XFERINFOFUNCTION, _download_progress)
+        proc_func_param = getattr(c, 'XFERINFOFUNCTION', None)
+        if proc_func_param is None:
+            proc_func_param = getattr(c, 'PROGRESSFUNCTION', None)
+        if proc_func_param is not None:
+            c.setopt(c.NOPROGRESS, False)
+            c.setopt(proc_func_param, _download_progress)
+        else:
+            c.close()
+            error(_('Error: XFERINFOFUNCTION and PROGRESSFUNCTION are not supported by pyCurl. Quit download progress'))
     c.perform()
     c.close()
     if not (quiet or noprogress):
diff --git a/tests/test_cli/test_download_file.py b/tests/test_cli/test_download_file.py
index 8b8cfdb..e2b5783 100644
--- a/tests/test_cli/test_download_file.py
+++ b/tests/test_cli/test_download_file.py
@@ -1,5 +1,6 @@
 from __future__ import absolute_import
 import mock
+from mock import call
 import six
 import shutil
 import tempfile
@@ -15,6 +16,8 @@ class TestDownloadFile(unittest.TestCase):
     def reset_mock(self):
         self.stdout.seek(0)
         self.stdout.truncate()
+        self.stderr.seek(0)
+        self.stderr.truncate()
         # self.curl.reset_mock()
         self.curlClass.reset_mock()
 
@@ -22,6 +25,7 @@ class TestDownloadFile(unittest.TestCase):
         self.tempdir = tempfile.mkdtemp()
         self.filename = self.tempdir + "/filename"
         self.stdout = mock.patch('sys.stdout', new_callable=six.StringIO).start()
+        self.stderr = mock.patch('sys.stderr', new_callable=six.StringIO).start()
         self.curlClass = mock.patch('pycurl.Curl', create=True).start()
         self.curl = self.curlClass.return_value
 
@@ -83,6 +87,29 @@ class TestDownloadFile(unittest.TestCase):
         self.assertMultiLineEqual(actual, expected)
         self.assertEqual(self.curl.setopt.call_count, 3)
 
+    def test_handle_download_file_curl_version(self):
+        self.curl.XFERINFOFUNCTION = None
+        download_file("http://url", self.filename, quiet=False, noprogress=False)
+        actual = self.stdout.getvalue()
+        expected = 'Downloading: %s\n\n' % self.filename
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(self.curl.setopt.call_count, 5)
+        self.curl.setopt.assert_has_calls([call(self.curl.PROGRESSFUNCTION, _download_progress)])
+
+        self.reset_mock()
+        self.curl.PROGRESSFUNCTION = None
+        with self.assertRaises(SystemExit) as cm:
+            download_file("http://url", self.filename, quiet=False, noprogress=False)
+        actual = self.stdout.getvalue()
+        expected = 'Downloading: %s\n' % self.filename
+        self.assertMultiLineEqual(actual, expected)
+        actual = self.stderr.getvalue()
+        expected = 'Error: XFERINFOFUNCTION and PROGRESSFUNCTION are not supported by pyCurl. Quit download progress\n'
+        self.assertMultiLineEqual(actual, expected)
+        self.assertEqual(self.curl.setopt.call_count, 3)
+        self.assertEqual(cm.exception.code, 1)
+
+
 
 class TestDownloadProgress(unittest.TestCase):
     # Show long diffs in error output...