Fix python 3 issues with apt* modules. (#4848)

- Use range instead of xrange.
- Use python3-apt package for python 3.
- Eliminate unsupported for/else/raise usage.
- Use list on dict.items when modifying dict.
- Update requirements documentation.

Also made non-intrustive style fixes (adding blank lines).

3 files changed, 58 insertions, 18 deletions

diff --git a/packaging/os/apt.py b/packaging/os/apt.py
index 9dd54adf..29b8fb8f 100644
--- a/packaging/os/apt.py
+++ b/packaging/os/apt.py
@@ -116,8 +116,10 @@ options:
     required: false
     default: false
     version_added: "2.1"
-
-requirements: [ python-apt, aptitude ]
+requirements:
+   - python-apt (python 2)
+   - python3-apt (python 3)
+   - aptitude
 author: "Matthew Williams (@mgwilliams)"
 notes:
    - Three of the upgrade modes (C(full), C(safe) and its alias C(yes)) require C(aptitude), otherwise
@@ -197,6 +199,7 @@ import os
 import datetime
 import fnmatch
 import itertools
+import sys
 
 from ansible.module_utils._text import to_native
 
@@ -226,6 +229,12 @@ try:
 except ImportError:
     HAS_PYTHON_APT = False
 
+if sys.version_info[0] < 3:
+    PYTHON_APT = 'python-apt'
+else:
+    PYTHON_APT = 'python3-apt'
+
+
 def package_split(pkgspec):
     parts = pkgspec.split('=', 1)
     if len(parts) > 1:
@@ -233,6 +242,7 @@ def package_split(pkgspec):
     else:
         return parts[0], None
 
+
 def package_versions(pkgname, pkg, pkg_cache):
     try:
         versions = set(p.version for p in pkg.versions)
@@ -245,12 +255,14 @@ def package_versions(pkgname, pkg, pkg_cache):
 
     return versions
 
+
 def package_version_compare(version, other_version):
     try:
         return apt_pkg.version_compare(version, other_version)
     except AttributeError:
         return apt_pkg.VersionCompare(version, other_version)
 
+
 def package_status(m, pkgname, version, cache, state):
     try:
         # get the package from the cache, as well as the
@@ -327,6 +339,7 @@ def package_status(m, pkgname, version, cache, state):
 
     return package_is_installed, package_is_upgradable, has_files
 
+
 def expand_dpkg_options(dpkg_options_compressed):
     options_list = dpkg_options_compressed.split(',')
     dpkg_options = ""
@@ -335,6 +348,7 @@ def expand_dpkg_options(dpkg_options_compressed):
                        % (dpkg_options, dpkg_option)
     return dpkg_options.strip()
 
+
 def expand_pkgspec_from_fnmatches(m, pkgspec, cache):
     # Note: apt-get does implicit regex matching when an exact package name
     # match is not found.  Something like this:
@@ -373,6 +387,7 @@ def expand_pkgspec_from_fnmatches(m, pkgspec, cache):
             new_pkgspec.append(pkgspec_pattern)
     return new_pkgspec
 
+
 def parse_diff(output):
     diff = to_native(output).splitlines()
     try:
@@ -394,6 +409,7 @@ def parse_diff(output):
     diff_end += 1
     return {'prepared': '\n'.join(diff[diff_start:diff_end])}
 
+
 def install(m, pkgspec, cache, upgrade=False, default_release=None,
             install_recommends=None, force=False,
             dpkg_options=expand_dpkg_options(DPKG_OPTIONS),
@@ -471,6 +487,7 @@ def install(m, pkgspec, cache, upgrade=False, default_release=None,
     else:
         return (True, dict(changed=False))
 
+
 def get_field_of_deb(m, deb_file, field="Version"):
     cmd_dpkg = m.get_bin_path("dpkg", True)
     cmd = cmd_dpkg + " --field %s %s" % (deb_file, field)
@@ -479,6 +496,7 @@ def get_field_of_deb(m, deb_file, field="Version"):
         m.fail_json(msg="%s failed" % cmd, stdout=stdout, stderr=stderr)
     return to_native(stdout).strip('\n')
 
+
 def install_deb(m, debs, cache, force, install_recommends, allow_unauthenticated, dpkg_options):
     changed=False
     deps_to_install = []
@@ -553,6 +571,7 @@ def install_deb(m, debs, cache, force, install_recommends, allow_unauthenticated
     else:
         m.exit_json(changed=changed, stdout=retvals.get('stdout',''), stderr=retvals.get('stderr',''), diff=retvals.get('diff', ''))
 
+
 def remove(m, pkgspec, cache, purge=False, force=False,
            dpkg_options=expand_dpkg_options(DPKG_OPTIONS), autoremove=False):
     pkg_list = []
@@ -598,6 +617,7 @@ def remove(m, pkgspec, cache, purge=False, force=False,
             m.fail_json(msg="'apt-get remove %s' failed: %s" % (packages, err), stdout=out, stderr=err)
         m.exit_json(changed=True, stdout=out, stderr=err, diff=diff)
 
+
 def upgrade(m, mode="yes", force=False, default_release=None,
             dpkg_options=expand_dpkg_options(DPKG_OPTIONS)):
     if m.check_mode:
@@ -648,6 +668,7 @@ def upgrade(m, mode="yes", force=False, default_release=None,
         m.exit_json(changed=False, msg=out, stdout=out, stderr=err)
     m.exit_json(changed=True, msg=out, stdout=out, stderr=err, diff=diff)
 
+
 def download(module, deb):
     tempdir = os.path.dirname(__file__)
     package = os.path.join(tempdir, str(deb.rsplit('/', 1)[1]))
@@ -674,6 +695,7 @@ def download(module, deb):
 
     return deb
 
+
 def main():
     module = AnsibleModule(
         argument_spec = dict(
@@ -701,16 +723,18 @@ def main():
 
     if not HAS_PYTHON_APT:
         if module.check_mode:
-            module.fail_json(msg="python-apt must be installed to use check mode. If run normally this module can autoinstall it")
+            module.fail_json(msg="%s must be installed to use check mode. "
+                                 "If run normally this module can auto-install it." % PYTHON_APT)
         try:
-            module.run_command('apt-get update', check_rc=True)
-            module.run_command('apt-get install python-apt -y -q', check_rc=True)
+            module.run_command(['apt-get', 'update'], check_rc=True)
+            module.run_command(['apt-get', 'install', PYTHON_APT, '-y', '-q'], check_rc=True)
             global apt, apt_pkg
             import apt
             import apt.debfile
             import apt_pkg
         except ImportError:
-            module.fail_json(msg="Could not import python modules: apt, apt_pkg. Please install python-apt package.")
+            module.fail_json(msg="Could not import python modules: apt, apt_pkg. "
+                                 "Please install %s package." % PYTHON_APT)
 
     global APTITUDE_CMD
     APTITUDE_CMD = module.get_bin_path("aptitude", False)
@@ -772,15 +796,14 @@ def main():
                         updated_cache_time = int(time.mktime(mtimestamp.timetuple()))
 
             if cache_valid is not True:
-                for retry in xrange(3):
+                for retry in range(3):
                     try:
                         cache.update()
                         break
                     except apt.cache.FetchFailedException:
                         pass
                 else:
-                    #out of retries, pass on the exception
-                    raise
+                    module.fail_json(msg='Failed to update apt cache.')
                 cache.open(progress=None)
                 updated_cache = True
                 updated_cache_time = int(time.mktime(now.timetuple()))
diff --git a/packaging/os/apt_key.py b/packaging/os/apt_key.py
index c2993be1..129c467e 100644
--- a/packaging/os/apt_key.py
+++ b/packaging/os/apt_key.py
@@ -123,6 +123,7 @@ def check_missing_binaries(module):
     if len(missing):
         module.fail_json(msg="binaries are missing", names=missing)
 
+
 def all_keys(module, keyring, short_format):
     if keyring:
         cmd = "apt-key --keyring %s adv --list-public-keys --keyid-format=long" % keyring
@@ -141,6 +142,7 @@ def all_keys(module, keyring, short_format):
         results = shorten_key_ids(results)
     return results
 
+
 def shorten_key_ids(key_id_list):
     """
     Takes a list of key ids, and converts them to the 'short' format,
@@ -151,6 +153,7 @@ def shorten_key_ids(key_id_list):
         short.append(key[-8:])
     return short
 
+
 def download_key(module, url):
     # FIXME: move get_url code to common, allow for in-memory D/L, support proxies
     # and reuse here
@@ -166,12 +169,13 @@ def download_key(module, url):
     except Exception:
         module.fail_json(msg="error getting key id from url: %s" % url, traceback=format_exc())
 
+
 def import_key(module, keyring, keyserver, key_id):
     if keyring:
         cmd = "apt-key --keyring %s adv --keyserver %s --recv %s" % (keyring, keyserver, key_id)
     else:
         cmd = "apt-key adv --keyserver %s --recv %s" % (keyserver, key_id)
-    for retry in xrange(5):
+    for retry in range(5):
         (rc, out, err) = module.run_command(cmd)
         if rc == 0:
             break
@@ -181,6 +185,7 @@ def import_key(module, keyring, keyserver, key_id):
                          rc=rc, stdout=out, stderr=err)
     return True
 
+
 def add_key(module, keyfile, keyring, data=None):
     if data is not None:
         if keyring:
@@ -196,6 +201,7 @@ def add_key(module, keyfile, keyring, data=None):
         (rc, out, err) = module.run_command(cmd, check_rc=True)
     return True
 
+
 def remove_key(module, key_id, keyring):
     # FIXME: use module.run_command, fail at point of error and don't discard useful stdin/stdout
     if keyring:
@@ -205,6 +211,7 @@ def remove_key(module, key_id, keyring):
     (rc, out, err) = module.run_command(cmd, check_rc=True)
     return True
 
+
 def main():
     module = AnsibleModule(
         argument_spec=dict(
diff --git a/packaging/os/apt_repository.py b/packaging/os/apt_repository.py
index cecd94ac..eae62280 100644
--- a/packaging/os/apt_repository.py
+++ b/packaging/os/apt_repository.py
@@ -28,7 +28,7 @@ short_description: Add and remove APT repositories
 description:
     - Add or remove an APT repositories in Ubuntu and Debian.
 notes:
-    - This module works on Debian and Ubuntu and requires C(python-apt).
+    - This module works on Debian and Ubuntu.
     - This module supports Debian Squeeze (version 6) as well as its successors.
     - This module treats Debian and Ubuntu distributions separately. So PPA could be installed only on Ubuntu machines.
 options:
@@ -72,7 +72,9 @@ options:
         required: false
 author: "Alexander Saltanov (@sashka)"
 version_added: "0.7"
-requirements: [ python-apt ]
+requirements:
+   - python-apt (python 2)
+   - python3-apt (python 3)
 '''
 
 EXAMPLES = '''
@@ -96,6 +98,7 @@ apt_repository: repo='ppa:nginx/stable'
 import glob
 import os
 import re
+import sys
 import tempfile
 
 try:
@@ -108,10 +111,16 @@ except ImportError:
     distro = None
     HAVE_PYTHON_APT = False
 
+if sys.version_info[0] < 3:
+    PYTHON_APT = 'python-apt'
+else:
+    PYTHON_APT = 'python3-apt'
+
 DEFAULT_SOURCES_PERM = int('0644', 8)
 
 VALID_SOURCE_TYPES = ('deb', 'deb-src')
 
+
 def install_python_apt(module):
 
     if not module.check_mode:
@@ -119,8 +128,8 @@ def install_python_apt(module):
         if apt_get_path:
             rc, so, se = module.run_command([apt_get_path, 'update'])
             if rc != 0:
-                module.fail_json(msg="Failed to auto-install python-apt. Error was: '%s'" % se.strip())
-            rc, so, se = module.run_command([apt_get_path, 'install', 'python-apt', '-y', '-q'])
+                module.fail_json(msg="Failed to auto-install %s. Error was: '%s'" % (PYTHON_APT, se.strip()))
+            rc, so, se = module.run_command([apt_get_path, 'install', PYTHON_APT, '-y', '-q'])
             if rc == 0:
                 global apt, apt_pkg, aptsources_distro, distro, HAVE_PYTHON_APT
                 import apt
@@ -129,9 +138,10 @@ def install_python_apt(module):
                 distro = aptsources_distro.get_distro()
                 HAVE_PYTHON_APT = True
             else:
-                module.fail_json(msg="Failed to auto-install python-apt. Error was: '%s'" % se.strip())
+                module.fail_json(msg="Failed to auto-install %s. Error was: '%s'" % (PYTHON_APT, se.strip()))
     else:
-        module.fail_json(msg="python-apt must be installed to use check mode")
+        module.fail_json(msg="%s must be installed to use check mode" % PYTHON_APT)
+
 
 class InvalidSource(Exception):
     pass
@@ -255,7 +265,7 @@ class SourcesList(object):
         self.files[file] = group
 
     def save(self):
-        for filename, sources in self.files.items():
+        for filename, sources in list(self.files.items()):
             if sources:
                 d, fn = os.path.split(filename)
                 fd, tmp_path = tempfile.mkstemp(prefix=".%s-" % fn, dir=d)
@@ -475,7 +485,7 @@ def main():
         if params['install_python_apt']:
             install_python_apt(module)
         else:
-            module.fail_json(msg='python-apt is not installed, and install_python_apt is False')
+            module.fail_json(msg='%s is not installed, and install_python_apt is False' % PYTHON_APT)
 
     if isinstance(distro, aptsources_distro.UbuntuDistribution):
         sourceslist = UbuntuSourcesList(module,