From 7e2cb6b156a149795440c6876949233a6fa55f1e Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 16 2017 14:03:42 +0000 Subject: replace pycurl with requests Related: https://pagure.io/koji/issue/600 --- diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py index 68b43b3..d991d93 100644 --- a/cli/koji_cli/lib.py +++ b/cli/koji_cli/lib.py @@ -4,12 +4,12 @@ from __future__ import division import optparse import os import random +import requests import six import socket import string import sys import time -import pycurl from six.moves import range try: @@ -487,33 +487,35 @@ def download_file(url, relpath, quiet=False, noprogress=False, size=None, num=No 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): - 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) + + with requests.get(url, stream=True) as response: + length = response.headers.get('content-length') + f = open(relpath, 'wb') + if length is None: + f.write(response.content) + length = len(response.content) + if not (quiet or noprogress): + _download_progress(length, length) else: - c.close() - error(_('Error: XFERINFOFUNCTION and PROGRESSFUNCTION are not supported by pyCurl. Quit download progress')) - c.perform() - c.close() + l = 0 + length = int(length) + for chunk in response.iter_content(chunk_size=65536): + l += len(chunk) + f.write(chunk) + if not (quiet or noprogress): + _download_progress(length, l) + f.close() + if not (quiet or noprogress): print('') -def _download_progress(download_t, download_d, upload_t, upload_d): +def _download_progress(download_t, download_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) + percent_done_str = "%3d%%" % (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)) diff --git a/docs/source/writing_koji_code.rst b/docs/source/writing_koji_code.rst index 7f25f31..ee00c59 100644 --- a/docs/source/writing_koji_code.rst +++ b/docs/source/writing_koji_code.rst @@ -676,7 +676,6 @@ You will need to install the following packages to actually run the tests. * ``python-mock`` * ``python-simplejson`` * ``python-psycopg2`` - * ``python-pycurl`` * ``python-requests`` * ``python-qpid-proton`` diff --git a/koji.spec b/koji.spec index 30f5266..23888d5 100644 --- a/koji.spec +++ b/koji.spec @@ -70,7 +70,6 @@ Requires: python2-rpm Requires: rpm-python %endif Requires: pyOpenSSL -Requires: python-pycurl Requires: python-requests Requires: python-requests-kerberos Requires: python-dateutil @@ -89,7 +88,6 @@ Requires: python%{python3_pkgversion}-rpm %else Requires: rpm-python%{python3_pkgversion} %endif -Requires: python%{python3_pkgversion}-pycurl Requires: python%{python3_pkgversion}-pyOpenSSL Requires: python%{python3_pkgversion}-requests Requires: python%{python3_pkgversion}-requests-kerberos diff --git a/tests/test_cli/test_download_file.py b/tests/test_cli/test_download_file.py index e2b5783..2987e8e 100644 --- a/tests/test_cli/test_download_file.py +++ b/tests/test_cli/test_download_file.py @@ -1,6 +1,5 @@ from __future__ import absolute_import import mock -from mock import call import six import shutil import tempfile @@ -8,6 +7,13 @@ import unittest from koji_cli.lib import download_file, _download_progress +def mock_open(): + """Return the right patch decorator for open""" + if six.PY2: + return mock.patch('__builtin__.open') + else: + return mock.patch('builtins.open') + class TestDownloadFile(unittest.TestCase): # Show long diffs in error output... @@ -18,16 +24,15 @@ class TestDownloadFile(unittest.TestCase): self.stdout.truncate() self.stderr.seek(0) self.stderr.truncate() - # self.curl.reset_mock() - self.curlClass.reset_mock() + self.requests_get.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.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 + self.requests_get = mock.patch('requests.get', create=True, name='requests.get').start() + self.requests_get = self.requests_get.return_value.__enter__ def tearDown(self): mock.patch.stopall() @@ -40,30 +45,55 @@ class TestDownloadFile(unittest.TestCase): 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() + self.requests_get.assert_called_once() + + @mock_open() + def test_handle_download_file(self, m_open): + self.reset_mock() + response = mock.MagicMock() + self.requests_get.return_value = response + response.headers.get.return_value = '5' # content-length + response.iter_content.return_value = ['abcde'] - def test_handle_download_file(self): rv = download_file("http://url", self.filename) + actual = self.stdout.getvalue() - expected = 'Downloading: %s\n\n' % self.filename + expected = 'Downloading: %s\n[====================================] 100%% 5.00 B\r\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.requests_get.assert_called_once() + m_open.assert_called_once() + response.headers.get.assert_called_once() + response.iter_content.assert_called_once() self.assertIsNone(rv) + @mock_open() + def test_handle_download_file_undefined_length(self, m_open): + self.reset_mock() + response = mock.MagicMock() + self.requests_get.return_value = response + response.headers.get.return_value = None # content-length + response.content = 'abcdef' + + rv = download_file("http://url", self.filename) + + actual = self.stdout.getvalue() + expected = 'Downloading: %s\n[====================================] 100%% 6.00 B\r\n' % self.filename + self.assertMultiLineEqual(actual, expected) + + self.requests_get.assert_called_once() + m_open.assert_called_once() + response.headers.get.assert_called_once() + response.iter_content.assert_not_called() + 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.requests_get.assert_called_once() self.assertIsNone(rv) def test_handle_download_file_quiet_noprogress(self): @@ -71,45 +101,24 @@ class TestDownloadFile(unittest.TestCase): 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' % self.filename 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) - + ''' + possible tests + - handling redirect headers + - http vs https + ''' class TestDownloadProgress(unittest.TestCase): # Show long diffs in error output... @@ -122,14 +131,14 @@ class TestDownloadProgress(unittest.TestCase): 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) + _download_progress(0, 0) + _download_progress(1024 * 92, 1024) + _download_progress(1024 * 1024 * 23, 1024 * 1024 * 11) + _download_progress(1024 * 1024 * 1024 * 35, 1024 * 1024 * 1024 * 30) + _download_progress(318921, 318921) actual = self.stdout.getvalue() - expected = '[ ] 00% 0.00 B\r' + \ - '[ ] 01% 1.00 KiB\r' + \ + expected = '[ ] 0% 0.00 B\r' + \ + '[ ] 1% 1.00 KiB\r' + \ '[================= ] 47% 11.00 MiB\r' + \ '[============================== ] 85% 30.00 GiB\r' + \ '[====================================] 100% 311.45 KiB\r' diff --git a/vm/kojivmd b/vm/kojivmd index 5491ee9..b678179 100755 --- a/vm/kojivmd +++ b/vm/kojivmd @@ -40,7 +40,7 @@ import SimpleXMLRPCServer import threading import base64 import pwd -import pycurl +import requests import fnmatch from ConfigParser import ConfigParser from optparse import OptionParser @@ -676,10 +676,16 @@ class VMExecTask(BaseTaskHandler): else: raise koji.BuildError('unsupported file type: %s' % type) koji.ensuredir(os.path.dirname(localpath)) - c = pycurl.Curl() - c.setopt(c.URL, remote_url) - c.setopt(c.WRITEDATA, open(localpath, 'wb')) - c.perform() + with requests.get(remote_url, stream=True) as response: + f = open(localpath, 'wb') + length = response.headers.get('content-length') + if length is None: + f.write(response.content) + else: + for chunk in response.iter_content(chunk_size=65536): + f.write(chunk) + f.close() + return file(localpath, 'r')