Differential D668

Replace urlgrabber with requests
ClosedPublic
Actions

Authored by mkrizek on Nov 30 2015, 2:41 PM.

Details

Reviewers
lbrabec
kparal
tflink
jskladan
Commits
rLTRNd107d7155aa8: Replace urlgrabber with requests
Summary

Also replace dingus with mock in unittests.

Test Plan

Functests pass. Rpmlint worked. Downloading a 1gig file worked as well.

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mkrizek retitled this revision from to Replace urlgrabber with requests.Nov 30 2015, 2:41 PM
mkrizek updated this object.
mkrizek edited the test plan for this revision. (Show Details)
mkrizek added reviewers: tflink, kparal, jskladan, lbrabec.
mkrizek added inline comments.Nov 30 2015, 2:43 PM
libtaskotron/file_utils.py
174

I am more than fine with getting rid of the progressbar and not adding another dep.

tflink requested changes to this revision.Nov 30 2015, 3:47 PM

When I run tasks locally with this patch, execution blows up with:

$ python runtask.py -i python-faker-0.5.3-7.fc24 -t koji_build --local ../task-rpmlint/rpmlint.yml
[libtaskotron] 15:43:05 INFO    Execution started at: 2015-11-30 15:43:05 UTC
[libtaskotron] 15:43:05 DEBUG   Using libtaskotron 0.4.5
[libtaskotron] 15:43:05 DEBUG   Parsed arguments: Namespace(arch=None, debug=False, item='python-faker-0.5.3-7.fc24', jobid='-1', libvirt=False, local=True, no_destroy=False, override=[], patch=None, ssh=None, ssh_privkey=None, task=['../task-rpmlint/rpmlint.yml'], type='koji_build', uuid='20151130_154305_785666')
[libtaskotron] 15:43:05 DEBUG   Using config file: /home/tflink/code/taskotron/libtaskotron/conf/taskotron.yaml
[libtaskotron] 15:43:05 WARNING Unknown option "bodhi_server" in the config file /home/tflink/code/taskotron/libtaskotron/conf/taskotron.yaml
[libtaskotron] 15:43:05 DEBUG   Using config profile: development
[libtaskotron:logger.py:184] 2015-11-30 15:43:05 DEBUG   Stream logging enabled with level: DEBUG
[libtaskotron:logger.py:261] 2015-11-30 15:43:05 DEBUG   File logging enabled with level DEBUG into: /var/log/taskotron/taskotron-tflink.log
[libtaskotron:overlord.py:89] 2015-11-30 15:43:05 INFO    Task artifacts will be saved in: /var/lib/taskotron/artifacts/20151130_154305_785666
[libtaskotron:overlord.py:60] 2015-11-30 15:43:05 DEBUG   Forcing local execution (option --local)
[libtaskotron:overlord.py:73] 2015-11-30 15:43:05 DEBUG   Execution mode: local
[libtaskotron:logger.py:261] 2015-11-30 15:43:05 DEBUG   File logging enabled with level DEBUG into: /var/lib/taskotron/artifacts/20151130_154305_785666/taskotron.log
[libtaskotron:rpm_utils.py:146] 2015-11-30 15:43:05 DEBUG   Running command: dnf --assumeno --disableplugin=noroot install rpmlint
[libtaskotron:rpm_utils.py:150] 2015-11-30 15:43:06 DEBUG   Last metadata expiration check performed 0:01:59 ago on Mon Nov 30 08:41:06 2015.
Package rpmlint-1.8-2.fc23.noarch is already installed, skipping.
Dependencies resolved.
Nothing to do.
Complete!

[libtaskotron:executor.py:84] 2015-11-30 15:43:06 DEBUG   Current workdir: /var/tmp/taskotron/tflink/task-ipYhQS
[libtaskotron:executor.py:143] 2015-11-30 15:43:06 DEBUG   Executing directive: koji
[libtaskotron:koji_utils.py:160] 2015-11-30 15:43:06 INFO    Querying Koji for a list of RPMS for: python-faker-0.5.3-7.fc24
[libtaskotron:koji_utils.py:220] 2015-11-30 15:43:07 INFO    Fetching 4 RPMs for: python-faker-0.5.3-7.fc24 (into /var/tmp/taskotron/tflink/task-ipYhQS)
[libtaskotron:file_utils.py:102] 2015-11-30 15:43:07 DEBUG   Cached file /var/cache/taskotron/python-faker-doc-0.5.3-7.fc24.noarch.rpm differs from its online version. Redownloading.
[libtaskotron:file_utils.py:106] 2015-11-30 15:43:07 DEBUG   Downloading (cached): http://kojipkgs.fedoraproject.org/packages/python-faker/0.5.3/7.fc24/noarch/python-faker-doc-0.5.3-7.fc24.noarch.rpm
[libtaskotron:logger.py:88] 2015-11-30 15:43:07 CRITICAL Traceback (most recent call last):
  File "runtask.py", line 10, in <module>
    main.main()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/main.py", line 151, in main
    overlord.start()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/overlord.py", line 95, in start
    runner.execute()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 56, in execute
    self._run()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 93, in _run
    self._do_actions()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 131, in _do_actions
    self._do_single_action(action)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 152, in _do_single_action
    self.arg_data)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/directives/koji_directive.py", line 165, in process
    src=self.src)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/ext/fedora/koji_utils.py", line 228, in get_nvr_rpms
    rpm_file = file_utils.download(url, dest, cachedir=cachedir)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/file_utils.py", line 108, in download
    download_file(url, dl_dest)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/file_utils.py", line 159, in download_file
    max_value=(int(total_len) / chunk_size))  
TypeError: __init__() got an unexpected keyword argument 'max_value'

Is anyone else seeing this or is this a local setup issue?

libtaskotron.spec
6–7

This is a code change, it should have a version bump instead of just a revision bump

libtaskotron/file_utils.py
174

I have a slight preference for not adding another dep but it's not a huge deal

This revision now requires changes to proceed.Nov 30 2015, 3:47 PM
mkrizek added a comment.Nov 30 2015, 4:54 PM
In D668#12754, @tflink wrote:

When I run tasks locally with this patch, execution blows up with:

$ python runtask.py -i python-faker-0.5.3-7.fc24 -t koji_build --local ../task-rpmlint/rpmlint.yml
[libtaskotron] 15:43:05 INFO    Execution started at: 2015-11-30 15:43:05 UTC
[libtaskotron] 15:43:05 DEBUG   Using libtaskotron 0.4.5
[libtaskotron] 15:43:05 DEBUG   Parsed arguments: Namespace(arch=None, debug=False, item='python-faker-0.5.3-7.fc24', jobid='-1', libvirt=False, local=True, no_destroy=False, override=[], patch=None, ssh=None, ssh_privkey=None, task=['../task-rpmlint/rpmlint.yml'], type='koji_build', uuid='20151130_154305_785666')
[libtaskotron] 15:43:05 DEBUG   Using config file: /home/tflink/code/taskotron/libtaskotron/conf/taskotron.yaml
[libtaskotron] 15:43:05 WARNING Unknown option "bodhi_server" in the config file /home/tflink/code/taskotron/libtaskotron/conf/taskotron.yaml
[libtaskotron] 15:43:05 DEBUG   Using config profile: development
[libtaskotron:logger.py:184] 2015-11-30 15:43:05 DEBUG   Stream logging enabled with level: DEBUG
[libtaskotron:logger.py:261] 2015-11-30 15:43:05 DEBUG   File logging enabled with level DEBUG into: /var/log/taskotron/taskotron-tflink.log
[libtaskotron:overlord.py:89] 2015-11-30 15:43:05 INFO    Task artifacts will be saved in: /var/lib/taskotron/artifacts/20151130_154305_785666
[libtaskotron:overlord.py:60] 2015-11-30 15:43:05 DEBUG   Forcing local execution (option --local)
[libtaskotron:overlord.py:73] 2015-11-30 15:43:05 DEBUG   Execution mode: local
[libtaskotron:logger.py:261] 2015-11-30 15:43:05 DEBUG   File logging enabled with level DEBUG into: /var/lib/taskotron/artifacts/20151130_154305_785666/taskotron.log
[libtaskotron:rpm_utils.py:146] 2015-11-30 15:43:05 DEBUG   Running command: dnf --assumeno --disableplugin=noroot install rpmlint
[libtaskotron:rpm_utils.py:150] 2015-11-30 15:43:06 DEBUG   Last metadata expiration check performed 0:01:59 ago on Mon Nov 30 08:41:06 2015.
Package rpmlint-1.8-2.fc23.noarch is already installed, skipping.
Dependencies resolved.
Nothing to do.
Complete!

[libtaskotron:executor.py:84] 2015-11-30 15:43:06 DEBUG   Current workdir: /var/tmp/taskotron/tflink/task-ipYhQS
[libtaskotron:executor.py:143] 2015-11-30 15:43:06 DEBUG   Executing directive: koji
[libtaskotron:koji_utils.py:160] 2015-11-30 15:43:06 INFO    Querying Koji for a list of RPMS for: python-faker-0.5.3-7.fc24
[libtaskotron:koji_utils.py:220] 2015-11-30 15:43:07 INFO    Fetching 4 RPMs for: python-faker-0.5.3-7.fc24 (into /var/tmp/taskotron/tflink/task-ipYhQS)
[libtaskotron:file_utils.py:102] 2015-11-30 15:43:07 DEBUG   Cached file /var/cache/taskotron/python-faker-doc-0.5.3-7.fc24.noarch.rpm differs from its online version. Redownloading.
[libtaskotron:file_utils.py:106] 2015-11-30 15:43:07 DEBUG   Downloading (cached): http://kojipkgs.fedoraproject.org/packages/python-faker/0.5.3/7.fc24/noarch/python-faker-doc-0.5.3-7.fc24.noarch.rpm
[libtaskotron:logger.py:88] 2015-11-30 15:43:07 CRITICAL Traceback (most recent call last):
  File "runtask.py", line 10, in <module>
    main.main()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/main.py", line 151, in main
    overlord.start()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/overlord.py", line 95, in start
    runner.execute()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 56, in execute
    self._run()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 93, in _run
    self._do_actions()
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 131, in _do_actions
    self._do_single_action(action)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/executor.py", line 152, in _do_single_action
    self.arg_data)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/directives/koji_directive.py", line 165, in process
    src=self.src)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/ext/fedora/koji_utils.py", line 228, in get_nvr_rpms
    rpm_file = file_utils.download(url, dest, cachedir=cachedir)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/file_utils.py", line 108, in download
    download_file(url, dl_dest)
  File "/home/tflink/code/taskotron/libtaskotron/libtaskotron/file_utils.py", line 159, in download_file
    max_value=(int(total_len) / chunk_size))  
TypeError: __init__() got an unexpected keyword argument 'max_value'

Is anyone else seeing this or is this a local setup issue?

ok, I was able to reproduce it with python-progressbar rpm. It doesn't happen with the version from pip, hm. Will investigate.

mkrizek updated this revision to Diff 1730.Nov 30 2015, 6:10 PM

Fixed progressbar crash

mkrizek updated this object.Nov 30 2015, 6:13 PM
mkrizek edited the test plan for this revision. (Show Details)
tflink accepted this revision.Nov 30 2015, 6:55 PM
This revision is now accepted and ready to land.Nov 30 2015, 6:55 PM
jskladan accepted this revision.Dec 1 2015, 10:23 AM
kparal added inline comments.Dec 2 2015, 2:14 PM
libtaskotron/file_utils.py
17–19

It might be more obvious what these variables refer to if we prefixed them with REQUESTS_.

Personally I'd go with 30sec timeout. Long timeouts are a drag.

68–69

I guess this doesn't use keep-alive connection. It would be great if we could use it here as well. It would speed up downloading files when caching is used (developer machines).

76–77

Resuming partially downloaded files is supported.

I'm not saying this is important, but is it really supported with requests? If not, we should remove this comment.

148–150

I find a bit confusing to have download() and download_file() functions with the same description Download a file. Would it make sense to make this a private function?

159

We have to keep session around in order to use HTTP Keep-Alive:
http://docs.python-requests.org/en/latest/user/advanced/#keep-alive

Therefore as a global attribute, I guess.

kparal added inline comments.Dec 2 2015, 2:26 PM
libtaskotron/file_utils.py
76–77

I tested this and it seems it can't resume a download by default. It seems it can be done manually:
http://stackoverflow.com/questions/22894211/how-to-resume-file-download-in-python
But I consider this not that important and it's better to not support it than to have more complex code to maintain, I'd say.

mkrizek updated this revision to Diff 1741.Dec 2 2015, 9:06 PM
mkrizek edited the test plan for this revision. (Show Details)

Keep using one session

kparal accepted this revision.Dec 3 2015, 10:14 AM

I tested the new patch and it's very fast now, even for a large number of small files. Great job. A few nitpicks below, but feel free to commit right away, whether you adjust it or not.

libtaskotron/file_utils.py
20

Similarly to above, I'd call this _requests_session to indicate what this is related to :) This is file_utils,py, so this is not directly related to just downloading (we might even argue it's not the right module for it), and we have requests-unrelated methods here as well.

23

It might be a good idea to mention in the docstring that this is a singleton getter and that max_retries argument is applied only if the session doesn't exist and is created for the very first time. Alternatively, mounting of those adapters would have to be adjusting to be applied every time somebody calls this method.

Closed by commit rLTRNd107d7155aa8: Replace urlgrabber with requests (authored by mkrizek). · Explain WhyDec 3 2015, 3:38 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mlibtaskotron.spec (12 lines)
Mlibtaskotron/file_utils.py (105 lines)
Mreadme.rst (1 line)
Mrequirements.txt (2 lines)
Mtesting/functest_file_utils.py (97 lines)

Diff 1742

View Options

libtaskotron.spec

1# sitelib for noarch packages, sitearch for others (remove the unneeded one) 1​# sitelib for noarch packages, sitearch for others (remove the unneeded one)
2%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")} 2​%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
3%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")} 3​%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")}
4 4​
5Name: libtaskotron 5​Name: libtaskotron
6Version: 0.4.5 6​Version: 0.4.6
7Release: 2%{?dist} 7​Release: 1%{?dist}
tflinkUnsubmitted
Not Done

This is a code change, it should have a version bump instead of just a revision bump

8Summary: Taskotron Support Library 8​Summary: Taskotron Support Library
9 9​
10License: GPLv3 10​License: GPLv3
11URL: https://bitbucket.org/fedoraqa/libtaskotron 11​URL: https://bitbucket.org/fedoraqa/libtaskotron
12Source0: http://qadevel.fedoraproject.org/releases/%{name}/%{name}-%{version}.tar.gz 12​Source0: http://qadevel.fedoraproject.org/releases/%{name}/%{name}-%{version}.tar.gz
13 13​
14BuildArch: noarch 14​BuildArch: noarch
15 15​
16# sorted alphabetically 16​# sorted alphabetically
17Requires: libtaskotron-config = %{version}-%{release} 17​Requires: libtaskotron-config = %{version}-%{release}
18Requires: libtaskotron-core = %{version}-%{release} 18​Requires: libtaskotron-core = %{version}-%{release}
19Requires: libtaskotron-fedora = %{version}-%{release} 19​Requires: libtaskotron-fedora = %{version}-%{release}
20Requires: libtaskotron-disposable = %{version}-%{release} 20​Requires: libtaskotron-disposable = %{version}-%{release}
21 21​
22%description 22​%description
23Libtaskotron is a support library for running taskotron tasks. The top level libtaskotron package is a meta-package which will install all sub-packages 23​Libtaskotron is a support library for running taskotron tasks. The top level libtaskotron package is a meta-package which will install all sub-packages
24 24​
25%package -n libtaskotron-core 25​%package -n libtaskotron-core
26Summary: The minimal, core parts of libtaskotron 26​Summary: The minimal, core parts of libtaskotron
27 27​
28Requires: libtaskotron-config = %{version}-%{release} 28​Requires: libtaskotron-config = %{version}-%{release}
29​Requires: python-progressbar >= 2.3
30​Requires: python-requests >= 2.7.0
29Requires: python-setuptools 31​Requires: python-setuptools
30Requires: python-urlgrabber ​
31Requires: PyYAML >= 3.11 32​Requires: PyYAML >= 3.11
32Requires: resultsdb_api >= 1.2.1 33​Requires: resultsdb_api >= 1.2.1
33BuildRequires: pytest >= 2.6.4 34​BuildRequires: pytest >= 2.6.4
34BuildRequires: python-devel 35​BuildRequires: python-devel
35BuildRequires: python-dingus >= 0.3.4 36​BuildRequires: python-dingus >= 0.3.4
36BuildRequires: python-mock >= 1.0.1 37​BuildRequires: python-mock >= 1.0.1
38​BuildRequires: python-requests >= 2.7.0
37BuildRequires: python-setuptools 39​BuildRequires: python-setuptools
38BuildRequires: python-urlgrabber ​
39BuildRequires: PyYAML >= 3.11 40​BuildRequires: PyYAML >= 3.11
40BuildRequires: resultsdb_api >= 1.2.1 41​BuildRequires: resultsdb_api >= 1.2.1
41 42​
42%description -n libtaskotron-core 43​%description -n libtaskotron-core
43The minimal, core parts of libtaskotron that are needed to run tasks 44​The minimal, core parts of libtaskotron that are needed to run tasks
44 45​
45%package -n libtaskotron-config 46​%package -n libtaskotron-config
46Summary: Configuration files needed for using libtaskotron 47​Summary: Configuration files needed for using libtaskotron
▲ Show 20 Lines • Show All 99 Lines • ▼ Show 20 Line(s)
146%dir %{python_sitelib}/libtaskotron/ext/fedora 147​%dir %{python_sitelib}/libtaskotron/ext/fedora
147%{python_sitelib}/libtaskotron/ext/fedora/* 148​%{python_sitelib}/libtaskotron/ext/fedora/*
148 149​
149%files -n libtaskotron-disposable 150​%files -n libtaskotron-disposable
150%dir %{python_sitelib}/libtaskotron/ext/disposable 151​%dir %{python_sitelib}/libtaskotron/ext/disposable
151%{python_sitelib}/libtaskotron/ext/disposable/* 152​%{python_sitelib}/libtaskotron/ext/disposable/*
152 153​
153%changelog 154​%changelog
155​* Mon Nov 23 2015 Martin Krizek <mkrizek@redhat.com> - 0.4.6-1
156​- urlgrabber replaced with requests
157​
154* Fri Nov 20 2015 Martin Krizek <mkrizek@redhat.com> - 0.4.5-2 158​* Fri Nov 20 2015 Martin Krizek <mkrizek@redhat.com> - 0.4.5-2
155- Require the exact same version of subpackages 159​- Require the exact same version of subpackages
156- Remove unused deps 160​- Remove unused deps
157 161​
158* Thu Nov 19 2015 Martin Krizek <mkrizek@redhat.com> - 0.4.5-1 162​* Thu Nov 19 2015 Martin Krizek <mkrizek@redhat.com> - 0.4.5-1
159- Do not fail when copying a task from cwd (D656) 163​- Do not fail when copying a task from cwd (D656)
160- We need -core not -disposable on disposable clients (D652) 164​- We need -core not -disposable on disposable clients (D652)
161- Don't require -disposable in -core (D652) 165​- Don't require -disposable in -core (D652)
▲ Show 20 Lines • Show All 153 Lines • Show Last 20 Lines
View Options

libtaskotron/file_utils.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2014, Red Hat, Inc. 2​# Copyright 2009-2014, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6from __future__ import absolute_import 6​from __future__ import absolute_import
7import os 7​import os
8import sys 8​import sys
9import urllib2 9​
10import urlgrabber.grabber 10​import requests
11import urlgrabber.progress 11​import progressbar
12​
12from libtaskotron.logger import log 13​from libtaskotron.logger import log
13from libtaskotron.exceptions import TaskotronRemoteError 14​from libtaskotron.exceptions import TaskotronRemoteError
14 15​
15 16​
17​REQUESTS_MAX_RETRIES = 3
18​REQUESTS_TIMEOUT = 30
19​REQUESTS_CHUNK_SIZE = 1024 * 1024 # 1MB
kparalUnsubmitted
Not Done

It might be more obvious what these variables refer to if we prefixed them with REQUESTS_.

Personally I'd go with 30sec timeout. Long timeouts are a drag.

20​_requests_session = None
kparalUnsubmitted
Not Done

Similarly to above, I'd call this _requests_session to indicate what this is related to :) This is file_utils,py, so this is not directly related to just downloading (we might even argue it's not the right module for it), and we have requests-unrelated methods here as well.

21​
22​
23​def _get_session(max_retries=REQUESTS_MAX_RETRIES):
kparalUnsubmitted
Not Done

It might be a good idea to mention in the docstring that this is a singleton getter and that max_retries argument is applied only if the session doesn't exist and is created for the very first time. Alternatively, mounting of those adapters would have to be adjusting to be applied every time somebody calls this method.

24​ """Note that a singleton is returned and so the max_retries is
25​ set only the first time this functions is called.
26​ """
27​ global _requests_session
28​ if not _requests_session:
29​ _requests_session = requests.Session()
30​ _requests_session.mount('http://', requests.adapters.HTTPAdapter(max_retries=max_retries))
31​ _requests_session.mount('https://', requests.adapters.HTTPAdapter(max_retries=max_retries))
32​
33​ return _requests_session
34​
35​
16def makedirs(fullpath): 36​def makedirs(fullpath):
17 '''This is the same as :meth:`os.makedirs`, but does not raise an exception 37​ '''This is the same as :meth:`os.makedirs`, but does not raise an exception
18 when the destination directory already exists. 38​ when the destination directory already exists.
19 39​
20 :raise OSError: if directory doesn't exist and can't be created 40​ :raise OSError: if directory doesn't exist and can't be created
21 ''' 41​ '''
22 try: 42​ try:
23 os.makedirs(fullpath) 43​ os.makedirs(fullpath)
24 assert os.path.isdir(fullpath) 44​ assert os.path.isdir(fullpath)
25 except OSError, e: 45​ except OSError, e:
26 if e.errno == 17: # "[Errno 17] File exists" 46​ if e.errno == 17: # "[Errno 17] File exists"
27 # if it is a directory everything is ok 47​ # if it is a directory everything is ok
28 if os.path.isdir(fullpath): 48​ if os.path.isdir(fullpath):
29 return 49​ return
30 # otherwise it is a file/socket/etc and it is an error 50​ # otherwise it is a file/socket/etc and it is an error
31 else: 51​ else:
32 log.warn("Can't create directory, something else already exists: %s", 52​ log.warn("Can't create directory, something else already exists: %s",
33 fullpath) 53​ fullpath)
34 raise 54​ raise
35 else: 55​ else:
36 log.warn("Can't create directory: %s", fullpath) 56​ log.warn("Can't create directory: %s", fullpath)
37 raise 57​ raise
38 58​
39 59​
40def get_urlgrabber(**kwargs): ​
41 '''Get a new instance of :class:`URLGrabber` that is reasonably configured ​
42 (progress bar when in terminal, automatic retries, connection timeout) ​
43 ​
44 :param kwargs: additional kwargs to add to the URLGrabber constructor ​
45 :return: :class:`urlgrabber.grabber.URLGrabber` instance ​
46 ''' ​
47 grabber_args = {'retry': 3, 'timeout': 120} ​
48 grabber_args.update(kwargs) ​
49 ​
50 grabber = urlgrabber.grabber.URLGrabber(**grabber_args) ​
51 ​
52 # progress bar when in terminal ​
53 if hasattr(sys.stdout, "fileno") and os.isatty(sys.stdout.fileno()): ​
54 grabber.opts.progress_obj = urlgrabber.progress.TextMeter() ​
55 ​
56 # retry on socket timeout ​
57 if 12 not in grabber.opts.retrycodes: ​
58 grabber.opts.retrycodes.append(12) ​
59 ​
60 return grabber ​
61 ​
62 ​
63def _same_length(filepath, url): 60​def _same_length(filepath, url):
64 '''Determine whether a local file and a file referred by HTTP URL have the 61​ '''Determine whether a local file and a file referred by HTTP URL have the
65 same length. If any exception occurs, ``False`` is returned. 62​ same length. If any exception occurs, ``False`` is returned.
66 :rtype: bool 63​ :rtype: bool
67 ''' 64​ '''
68 try: 65​ try:
69 local_size = os.path.getsize(filepath) 66​ local_size = os.path.getsize(filepath)
70 67​
71 remote_file = urllib2.urlopen(url, timeout=120) 68​ remote_file = _get_session().head(url, timeout=REQUESTS_TIMEOUT)
72 remote_file.close() 69​ remote_size = int(remote_file.headers.get('Content-Length'))
kparalUnsubmitted
Not Done

I guess this doesn't use keep-alive connection. It would be great if we could use it here as well. It would speed up downloading files when caching is used (developer machines).

73 remote_size = int(remote_file.headers['Content-Length']) ​
74 70​
75 return local_size == remote_size 71​ return local_size == remote_size
76 except: 72​ except:
77 return False 73​ return False
78 74​
79 75​
80def download(url, dirname, filename=None, cachedir=None, grabber=None): 76​def download(url, dirname, filename=None, cachedir=None):
81 '''Download a file. Resuming partially downloaded files is supported. 77​ '''Download a file.
kparalUnsubmitted
Not Done

Resuming partially downloaded files is supported.

I'm not saying this is important, but is it really supported with requests? If not, we should remove this comment.

kparalUnsubmitted
Not Done

I tested this and it seems it can't resume a download by default. It seems it can be done manually:
http://stackoverflow.com/questions/22894211/how-to-resume-file-download-in-python
But I consider this not that important and it's better to not support it than to have more complex code to maintain, I'd say.

82 78​
83 :param str url: file URL to download 79​ :param str url: file URL to download
84 :param str dirname: directory path; if the directory does not exist, it gets 80​ :param str dirname: directory path; if the directory does not exist, it gets
85 created (and all its parent directories). 81​ created (and all its parent directories).
86 :param str filename: name of downloaded file; if not provided, the basename 82​ :param str filename: name of downloaded file; if not provided, the basename
87 is extracted from URL 83​ is extracted from URL
88 :param str cachedir: If set, the file will be downloaded to a cache 84​ :param str cachedir: If set, the file will be downloaded to a cache
89 directory specified by this parameter. If the file is 85​ directory specified by this parameter. If the file is
90 already present and of the same length, download is skipped. 86​ already present and of the same length, download is skipped.
91 The requested destination file (``dirname/filename``) 87​ The requested destination file (``dirname/filename``)
92 will be a symlink to the cached file. 88​ will be a symlink to the cached file.
93 This directory is automatically created if not present. 89​ This directory is automatically created if not present.
94 :param grabber: custom :class:`urlgrabber.grabber.URLGrabber` instance to be ​
95 used for actual downloading ​
96 :return: the path to the downloaded file 90​ :return: the path to the downloaded file
97 :rtype: str 91​ :rtype: str
98 :raise TaskotronRemoteError: if download fails 92​ :raise TaskotronRemoteError: if download fails
99 ''' 93​ '''
100 94​
101 if not grabber: ​
102 grabber = get_urlgrabber() ​
103 ​
104 if not filename: 95​ if not filename:
105 filename = os.path.basename(url) 96​ filename = os.path.basename(url)
106 97​
107 dl_dest = dest = os.path.abspath(os.path.join(dirname, filename)) 98​ dl_dest = dest = os.path.abspath(os.path.join(dirname, filename))
108 dirs_to_create = [dirname] 99​ dirs_to_create = [dirname]
109 100​
110 if cachedir: 101​ if cachedir:
111 dl_dest = os.path.join(cachedir, filename) 102​ dl_dest = os.path.join(cachedir, filename)
Show All 11 Lines110 if os.path.exists(dl_dest):
123 else: 114​ else:
124 log.debug('Cached file %s differs from its online version. ' 115​ log.debug('Cached file %s differs from its online version. '
125 'Redownloading.', dl_dest) 116​ 'Redownloading.', dl_dest)
126 117​
127 # download the file 118​ # download the file
128 if download: 119​ if download:
129 log.debug('Downloading%s: %s', ' (cached)' if cachedir else '', url) 120​ log.debug('Downloading%s: %s', ' (cached)' if cachedir else '', url)
130 try: 121​ try:
131 grabber.urlgrab(url=url, filename=dl_dest) 122​ _download(url, dl_dest)
132 except urlgrabber.grabber.URLGrabError, e: 123​ except requests.exceptions.RequestException, e:
133 log.exception('Download failed for: %s', url) 124​ log.exception('Download failed for: %s', url)
134 # the file can be incomplete, remove 125​ # the file can be incomplete, remove
135 if os.path.exists(dl_dest): 126​ if os.path.exists(dl_dest):
136 try: 127​ try:
137 os.remove(dl_dest) 128​ os.remove(dl_dest)
138 except OSError, e: 129​ except OSError, e:
139 log.exception('Could not delete incomplete file: %s', dl_dest) 130​ log.exception('Could not delete incomplete file: %s', dl_dest)
140 raise TaskotronRemoteError(e) 131​ raise TaskotronRemoteError(e)
141 132​
142 # create a symlink if the download was cached 133​ # create a symlink if the download was cached
143 if cachedir: 134​ if cachedir:
144 try: 135​ try:
145 if (os.path.exists(dest)): 136​ if os.path.exists(dest):
146 # if there already is something at the destination, we need to 137​ # if there already is something at the destination, we need to
147 # remove it first 138​ # remove it first
148 os.remove(dest) 139​ os.remove(dest)
149 os.symlink(dl_dest, dest) 140​ os.symlink(dl_dest, dest)
150 except OSError, e: 141​ except OSError:
151 log.exception("Can't create symlink %s -> %s", dl_dest, dest) 142​ log.exception("Can't create symlink %s -> %s", dl_dest, dest)
152 raise 143​ raise
153 144​
154 return dest 145​ return dest
155 146​
156 147​
148​def _download(url, dest, timeout=REQUESTS_TIMEOUT, chunk_size=REQUESTS_CHUNK_SIZE):
149​ """
150​ A download helper function.
kparalUnsubmitted
Not Done

I find a bit confusing to have download() and download_file() functions with the same description Download a file. Would it make sense to make this a private function?

151​
152​ :param str url: an url to be downloaded
153​ :param str dest: a destination of the downloaded file
154​ :param float timeout: how long to wait for the server to send data before giving up
155​ :param int chunk_size: chunk size of the file downloaded (loaded into memory) at a time
156​ :raise requests.exceptions.RequestException: if the download request failed
157​ """
158​
159​ r = _get_session().get(url, timeout=timeout, stream=True)
kparalUnsubmitted
Not Done

We have to keep session around in order to use HTTP Keep-Alive:
http://docs.python-requests.org/en/latest/user/advanced/#keep-alive

Therefore as a global attribute, I guess.

160​ with open(dest, 'wb') as f:
161​ total_len = r.headers.get('Content-Length')
162​
163​ if total_len is None:
164​ for chunk in r.iter_content(chunk_size):
165​ # filter out keep-alive new lines
166​ if chunk:
167​ f.write(chunk)
168​ else:
169​ widgets = [progressbar.Percentage(), progressbar.Bar(), progressbar.ETA(),
170​ progressbar.FileTransferSpeed()]
171​ pbar = progressbar.ProgressBar(widgets=widgets, maxval=int(total_len)).start()
172​
173​ read = 0
174​ for chunk in pbar(r.iter_content(chunk_size)):
mkrizekAuthorUnsubmitted
Not Done

I am more than fine with getting rid of the progressbar and not adding another dep.

tflinkUnsubmitted
Not Done

I have a slight preference for not adding another dep but it's not a huge deal

175​ # filter out keep-alive new lines
176​ if chunk:
177​ f.write(chunk)
178​ read += len(chunk)
179​ pbar.update(read)
180​
181​
157# http://stackoverflow.com/questions/11325019 182​# http://stackoverflow.com/questions/11325019
158class Tee(object): 183​class Tee(object):
159 '''Helper class for writing data to different streams.''' 184​ '''Helper class for writing data to different streams.'''
160 185​
161 def __init__(self, *files): 186​ def __init__(self, *files):
162 self._files = list(files) 187​ self._files = list(files)
163 188​
164 def add(self, file_): 189​ def add(self, file_):
Show All 14 Lines
View Options

readme.rst

Show First 20 Lines • Show All 46 Lines • ▼ Show 20 Line(s)42Once the copr repo is available, install the necessary packages::
47 git \ 47​ git \
48 koji \ 48​ koji \
49 libtaskotron-config \ 49​ libtaskotron-config \
50 mash \ 50​ mash \
51 python-doit \ 51​ python-doit \
52 python-hawkey \ 52​ python-hawkey \
53 python-pip \ 53​ python-pip \
54 python-rpmfluff \ 54​ python-rpmfluff \
55 python-urlgrabber \ ​
56 python-virtualenv \ 55​ python-virtualenv \
57 rpm-build \ 56​ rpm-build \
58 rpm-python 57​ rpm-python
59 58​
60If you have not yet cloned the repository, do it now:: 59​If you have not yet cloned the repository, do it now::
61 60​
62 git clone https://bitbucket.org/fedoraqa/libtaskotron 61​ git clone https://bitbucket.org/fedoraqa/libtaskotron
63 cd libtaskotron 62​ cd libtaskotron
▲ Show 20 Lines • Show All 148 Lines • Show Last 20 Lines
View Options

requirements.txt

Show All 11 Lines
12 12​
13dingus == 0.3.4 13​dingus == 0.3.4
14munch == 2.0.2 14​munch == 2.0.2
15mock == 1.0.1 15​mock == 1.0.1
16paramiko == 1.15.1 16​paramiko == 1.15.1
17pytest == 2.6.4 17​pytest == 2.6.4
18pytest-cov == 1.8.1 18​pytest-cov == 1.8.1
19python-fedora == 0.5.5 19​python-fedora == 0.5.5
20​progressbar == 2.3
21​requests == 2.7.0
20PyYAML == 3.11 22​PyYAML == 3.11
21resultsdb_api == 1.2.1 23​resultsdb_api == 1.2.1
22Sphinx == 1.2.3 24​Sphinx == 1.2.3
View Options

testing/functest_file_utils.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2014, Red Hat, Inc. 2​# Copyright 2014, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6'''Functional tests for libtaskotron/file_utils.py''' 6​'''Functional tests for libtaskotron/file_utils.py'''
7 7​
8import os 8​import os
9import pytest 9​import pytest
10from dingus import Dingus 10​import mock
11from urlgrabber.grabber import URLGrabError 11​import requests
12 ​
13from libtaskotron import file_utils 12​from libtaskotron import file_utils
14from libtaskotron import exceptions as exc 13​from libtaskotron import exceptions as exc
15 14​
16 15​
17class TestSameLength(): 16​class TestSameLength():
18 17​
19 def test_nonexistent_file(self, tmpdir): 18​ def test_nonexistent_file(self, tmpdir):
20 '''Should not crash if the file doesn't exist''' 19​ """Should not crash if the file doesn't exist"""
21 fpath = tmpdir.join('nonexistent.file').strpath 20​ fpath = tmpdir.join('nonexistent.file').strpath
22 assert file_utils._same_length(fpath, None) is False 21​ assert file_utils._same_length(fpath, None) is False
23 22​
24 23​
25@pytest.mark.usefixtures('setup') 24​@pytest.mark.usefixtures('setup')
26class TestDownload(): 25​class TestDownload():
27 26​
28 @pytest.fixture 27​ @pytest.fixture
29 def setup(self, tmpdir): 28​ def setup(self, tmpdir, monkeypatch):
30 '''Run this before every test method execution start''' 29​ """Run this before every test method execution start"""
31 self.dirname = tmpdir.join('download').strpath 30​ self.dirname = tmpdir.join('download').strpath
32 self.cachedir = tmpdir.join('cachedir').strpath 31​ self.cachedir = tmpdir.join('cachedir').strpath
33 self.stub_grabber = Dingus() 32​ self._download_mocked = mock.Mock()
33​ monkeypatch.setattr(file_utils, '_download', self._download_mocked)
34 34​
35 def test_create_dirname(self, tmpdir): 35​ def test_create_dirname(self, tmpdir):
36 '''output dir should get created if it doesn't exist, multiple levels''' 36​ """output dir should get created if it doesn't exist, multiple levels"""
37 dirname = tmpdir.join('parentdir/subdir').strpath 37​ dirname = tmpdir.join('parentdir/subdir').strpath
38 assert not os.path.exists(dirname) 38​ assert not os.path.exists(dirname)
39 file_utils.download(url='http://localhost/file', dirname=dirname, 39​ file_utils.download(url='http://localhost/file', dirname=dirname)
40 grabber=self.stub_grabber) ​
41 assert os.path.isdir(dirname) 40​ assert os.path.isdir(dirname)
42 41​
43 def test_filename_missing(self): 42​ def test_filename_missing(self):
44 '''filename should be automatically derived''' 43​ """filename should be automatically derived"""
45 file_utils.download(url='http://localhost/file.xyz', dirname=self.dirname, 44​ file_utils.download(url='http://localhost/file.xyz', dirname=self.dirname)
46 grabber=self.stub_grabber) 45​ self._download_mocked.assert_called_with('http://localhost/file.xyz',
47 call = self.stub_grabber.calls[0] 46​ os.path.join(self.dirname, 'file.xyz'))
48 assert call[0] == 'urlgrab' ​
49 assert call[2]['filename'] == os.path.join(self.dirname, 'file.xyz') ​
50 47​
51 def test_filename_provided(self): 48​ def test_filename_provided(self):
52 '''filename should be respected when provided''' 49​ """filename should be respected when provided"""
53 file_utils.download(url='http://localhost/file.xyz', dirname=self.dirname, 50​ file_utils.download(url='http://localhost/file.xyz', dirname=self.dirname,
54 filename='important.file', grabber=self.stub_grabber) 51​ filename='important.file')
55 call = self.stub_grabber.calls[0] 52​
56 assert call[0] == 'urlgrab' 53​ self._download_mocked.assert_called_with('http://localhost/file.xyz',
57 assert call[2]['filename'] == os.path.join(self.dirname, 'important.file') 54​ os.path.join(self.dirname, 'important.file'))
58 55​
59 def test_create_cachedir(self): 56​ def test_create_cachedir(self):
60 '''cachedir should be created if it doesn't exist''' 57​ """cachedir should be created if it doesn't exist"""
61 assert not os.path.exists(self.cachedir) 58​ assert not os.path.exists(self.cachedir)
62 file_utils.download(url='http://localhost/file', dirname=self.dirname, 59​ file_utils.download(url='http://localhost/file', dirname=self.dirname,
63 cachedir=self.cachedir, grabber=self.stub_grabber) 60​ cachedir=self.cachedir)
64 assert os.path.isdir(self.cachedir) 61​ assert os.path.isdir(self.cachedir)
65 62​
66 def test_cache_used(self): 63​ def test_cache_used(self):
67 '''file should be downloaded to cache if cachedir is defined''' 64​ """file should be downloaded to cache if cachedir is defined"""
68 file_utils.download(url='http://localhost/file', dirname=self.dirname, 65​ file_utils.download(url='http://localhost/file.xyz', dirname=self.dirname,
69 cachedir=self.cachedir, grabber=self.stub_grabber) 66​ cachedir=self.cachedir)
70 call = self.stub_grabber.calls[0] 67​ self._download_mocked.assert_called_with('http://localhost/file.xyz',
71 assert call[0] == 'urlgrab' 68​ os.path.join(self.cachedir, 'file.xyz'))
72 assert call[2]['filename'] == os.path.join(self.cachedir, 'file') ​
73 69​
74 def test_cache_not_used(self): 70​ def test_cache_not_used(self):
75 '''file should not be downloaded to cache if cachedir is not defined''' 71​ """file should not be downloaded to cache if cachedir is not defined"""
76 file_utils.download(url='http://localhost/file', dirname=self.dirname, 72​ file_utils.download(url='http://localhost/file', dirname=self.dirname)
77 grabber=self.stub_grabber) 73​ assert not self._download_mocked.call_args[0][1].startswith(self.cachedir)
78 call = self.stub_grabber.calls[0] ​
79 assert call[0] == 'urlgrab' ​
80 assert not call[2]['filename'].startswith(self.cachedir) ​
81 74​
82 def test_skip_download_if_cached(self, tmpdir, monkeypatch): 75​ def test_skip_download_if_cached(self, tmpdir, monkeypatch):
83 '''if the file is already in cache, it should not be downloaded again''' 76​ """if the file is already in cache, it should not be downloaded again"""
84 tmpdir.mkdir('download').join('file').write('data') 77​ tmpdir.mkdir('download').join('file').write('data')
85 stub_same_length = lambda *args, **kwargs: True 78​ stub_same_length = lambda *args, **kwargs: True
86 monkeypatch.setattr(file_utils, '_same_length', stub_same_length) 79​ monkeypatch.setattr(file_utils, '_same_length', stub_same_length)
87 80​
88 file_utils.download(url='http://localhost/file', dirname=self.dirname, 81​ file_utils.download(url='http://localhost/file', dirname=self.dirname)
89 grabber=self.stub_grabber) 82​ assert self._download_mocked.call_count == 0
90 assert len(self.stub_grabber.calls) == 0 ​
91 83​
92 def test_create_symlink(self): 84​ def test_create_symlink(self):
93 '''symlink should be created if the download is cached''' 85​ """symlink should be created if the download is cached"""
94 file_utils.download(url='http://localhost/file', dirname=self.dirname, 86​ file_utils.download(url='http://localhost/file', dirname=self.dirname,
95 cachedir=self.cachedir, grabber=self.stub_grabber) 87​ cachedir=self.cachedir)
96 assert os.path.islink(os.path.join(self.dirname, 'file')) 88​ assert os.path.islink(os.path.join(self.dirname, 'file'))
97 89​
98 def test_skip_symlink(self): 90​ def test_skip_symlink(self):
99 '''symlink should not be in dirname if the download is not cached''' 91​ """symlink should not be in dirname if the download is not cached"""
100 file_utils.download(url='http://localhost/file', dirname=self.dirname, 92​ file_utils.download(url='http://localhost/file', dirname=self.dirname)
101 grabber=self.stub_grabber) ​
102 assert not os.path.islink(os.path.join(self.dirname, 'file')) 93​ assert not os.path.islink(os.path.join(self.dirname, 'file'))
103 94​
104 def test_return_value(self): 95​ def test_return_value(self):
105 '''downloaded file path should be returned''' 96​ """downloaded file path should be returned"""
106 path = file_utils.download(url='http://localhost/file', 97​ path = file_utils.download(url='http://localhost/file', dirname=self.dirname)
107 dirname=self.dirname, grabber=self.stub_grabber) ​
108 assert path == os.path.join(self.dirname, 'file') 98​ assert path == os.path.join(self.dirname, 'file')
109 99​
110 def test_raise(self): 100​ def test_raise(self, monkeypatch):
111 '''download errors should be raised''' 101​ """download errors should be raised"""
112 def raise_urlgrab_error(*args, **kwargs): 102​ _download_mocked = mock.Mock(side_effect=(
113 raise URLGrabError('fake download failed') 103​ requests.exceptions.RequestException('fake download failed')))
114 stub_grabber = Dingus(urlgrab=raise_urlgrab_error) 104​ monkeypatch.setattr(file_utils, '_download', _download_mocked)
115 105​
116 with pytest.raises(exc.TaskotronRemoteError): 106​ with pytest.raises(exc.TaskotronRemoteError):
117 file_utils.download(url='http://localhost/file', dirname=self.dirname, 107​ file_utils.download(url='http://localhost/file', dirname=self.dirname)
118 grabber=stub_grabber) ​