From 5d29a2eabdb4d85419e11d4bf20f316d6222d142 Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 24 Sep 2015 12:26:10 +0300 Subject: [PATCH 1/5] Python 3: shlex.split() wants unicode On Python 2, shlex.split() raises if you pass it a unicode object with non-ASCII characters in it. The Ansible codebase copes by explicitly converting the string using to_bytes() before passing it to shlex.split(). On Python 3, shlex.split() raises ('bytes' object has no attribute 'read') if you pass a bytes object. Oops. This commit introduces a new wrapper function, shlex_split, that transparently performs the to_bytes/to_unicode conversions only on Python 2. Currently I've only converted one call site (the one that was causing a unit test to fail on Python 3). If this approach is deemed suitable, I'll convert them all. --- lib/ansible/inventory/ini.py | 8 +++---- lib/ansible/utils/shlex.py | 33 ++++++++++++++++++++++++++++ test/units/utils/test_shlex.py | 39 ++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 lib/ansible/utils/shlex.py create mode 100644 test/units/utils/test_shlex.py diff --git a/lib/ansible/inventory/ini.py b/lib/ansible/inventory/ini.py index a2a90c76cfc..f29a5f73ec5 100644 --- a/lib/ansible/inventory/ini.py +++ b/lib/ansible/inventory/ini.py @@ -20,7 +20,6 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type import ast -import shlex import re from ansible import constants as C @@ -30,7 +29,8 @@ from ansible.inventory.group import Group from ansible.inventory.expand_hosts import detect_range from ansible.inventory.expand_hosts import expand_hostname_range from ansible.parsing.utils.addresses import parse_address -from ansible.utils.unicode import to_unicode, to_bytes +from ansible.utils.shlex import shlex_split +from ansible.utils.unicode import to_unicode class InventoryParser(object): """ @@ -231,13 +231,11 @@ class InventoryParser(object): # beta:2345 user=admin # we'll tell shlex # gamma sudo=True user=root # to ignore comments - line = to_bytes(line) try: - tokens = shlex.split(line, comments=True) + tokens = shlex_split(line, comments=True) except ValueError as e: self._raise_error("Error parsing host definition '%s': %s" % (varstring, e)) - tokens = [ to_unicode(t) for t in tokens] (hostnames, port) = self._expand_hostpattern(tokens[0]) hosts = self._Hosts(hostnames, port) diff --git a/lib/ansible/utils/shlex.py b/lib/ansible/utils/shlex.py new file mode 100644 index 00000000000..79a170402cd --- /dev/null +++ b/lib/ansible/utils/shlex.py @@ -0,0 +1,33 @@ +# (c) 2015, Marius Gedminas +# +# This file is part of Ansible +# +# Ansible is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Ansible is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# alongwith Ansible. If not, see . + +from __future__ import absolute_import + +import shlex +from six import PY3 + +from ansible.utils.unicode import to_bytes, to_unicode + + +if PY3: + # shlex.split() wants Unicode (i.e. ``str``) input on Python 3 + shlex_split = shlex.split +else: + # shlex.split() wants bytes (i.e. ``str``) input on Python 2 + def shlex_split(s, comments=False, posix=True): + return map(to_unicode, shlex.split(to_bytes(s), comments, posix)) + shlex_split.__doc__ = shlex.split.__doc__ diff --git a/test/units/utils/test_shlex.py b/test/units/utils/test_shlex.py new file mode 100644 index 00000000000..ef1fc28a66e --- /dev/null +++ b/test/units/utils/test_shlex.py @@ -0,0 +1,39 @@ +# (c) 2015, Marius Gedminas +# +# This file is part of Ansible +# +# Ansible is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Ansible is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Ansible. If not, see . + +import unittest + +from ansible.utils.shlex import shlex_split + + +class TestSplit(unittest.TestCase): + + def test_trivial(self): + self.assertEqual(shlex_split("a b c"), ["a", "b", "c"]) + + def test_unicode(self): + self.assertEqual(shlex_split(u"a b \u010D"), [u"a", u"b", u"\u010D"]) + + def test_quoted(self): + self.assertEqual(shlex_split('"a b" c'), ["a b", "c"]) + + def test_comments(self): + self.assertEqual(shlex_split('"a b" c # d', comments=True), ["a b", "c"]) + + def test_error(self): + self.assertRaises(ValueError, shlex_split, 'a "b') + From a2bc6b4b265f13ab099c50c2ccf768c64ec7eacf Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 24 Sep 2015 12:43:33 +0300 Subject: [PATCH 2/5] Bugfix: if you define __eq__, you should define __ne__ too --- lib/ansible/inventory/host.py | 3 +++ test/units/inventory/test_host.py | 35 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 test/units/inventory/test_host.py diff --git a/lib/ansible/inventory/host.py b/lib/ansible/inventory/host.py index 77a0b21f50e..fa2433956d2 100644 --- a/lib/ansible/inventory/host.py +++ b/lib/ansible/inventory/host.py @@ -38,6 +38,9 @@ class Host: def __eq__(self, other): return self.name == other.name + def __ne__(self, other): + return not self.__eq__(other) + def serialize(self): groups = [] for group in self.groups: diff --git a/test/units/inventory/test_host.py b/test/units/inventory/test_host.py new file mode 100644 index 00000000000..f9f500a63e6 --- /dev/null +++ b/test/units/inventory/test_host.py @@ -0,0 +1,35 @@ +# Copyright 2015 Marius Gedminas +# +# This file is part of Ansible +# +# Ansible is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Ansible is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Ansible. If not, see . + +import unittest + +from ansible.inventory.host import Host + + +class TestHost(unittest.TestCase): + + def setUp(self): + self.hostA = Host('a') + self.hostB = Host('b') + + def test_equality(self): + self.assertEqual(self.hostA, self.hostA) + self.assertNotEqual(self.hostA, self.hostB) + self.assertEqual(self.hostA, Host('a')) + # __ne__ is a separate method + self.assertFalse(self.hostA != Host('a')) + From 0624797375d676732fcb2c20d289386ba7d1a527 Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 24 Sep 2015 12:46:06 +0300 Subject: [PATCH 3/5] Bugfix: if you define a custom __eq__, you must define a __hash__ too Also, on Python 3 the stock object.__hash__ raises an error ("unhashable type"), and we have code that uses Host instances as dict keys. --- lib/ansible/inventory/host.py | 3 +++ test/units/inventory/test_host.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/lib/ansible/inventory/host.py b/lib/ansible/inventory/host.py index fa2433956d2..f6089f07ed9 100644 --- a/lib/ansible/inventory/host.py +++ b/lib/ansible/inventory/host.py @@ -41,6 +41,9 @@ class Host: def __ne__(self, other): return not self.__eq__(other) + def __hash__(self): + return hash(self.name) + def serialize(self): groups = [] for group in self.groups: diff --git a/test/units/inventory/test_host.py b/test/units/inventory/test_host.py index f9f500a63e6..078d4321b57 100644 --- a/test/units/inventory/test_host.py +++ b/test/units/inventory/test_host.py @@ -33,3 +33,6 @@ class TestHost(unittest.TestCase): # __ne__ is a separate method self.assertFalse(self.hostA != Host('a')) + def test_hashability(self): + # equality implies the hash values are the same + self.assertEqual(hash(self.hostA), hash(Host('a'))) From 6d4618f46f20f0e02922441f9253c24b1c6749bb Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 24 Sep 2015 12:50:00 +0300 Subject: [PATCH 4/5] Python 3: there's no dict.iteritems() --- lib/ansible/vars/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ansible/vars/__init__.py b/lib/ansible/vars/__init__.py index 0d5ea5a09d1..06e8876d519 100644 --- a/lib/ansible/vars/__init__.py +++ b/lib/ansible/vars/__init__.py @@ -24,6 +24,7 @@ import os from collections import defaultdict from collections import MutableMapping +from six import iteritems from jinja2.exceptions import UndefinedError try: @@ -285,7 +286,7 @@ class VariableManager: if self._inventory is not None: all_vars['groups'] = dict() - for (group_name, group) in self._inventory.groups.iteritems(): + for (group_name, group) in iteritems(self._inventory.groups): all_vars['groups'][group_name] = [h.name for h in group.get_hosts()] if include_hostvars: From 56f2a25bff43eaf5ba9339a97303e25841fa8b79 Mon Sep 17 00:00:00 2001 From: Marius Gedminas Date: Thu, 24 Sep 2015 12:52:51 +0300 Subject: [PATCH 5/5] Python 3: there's no 'unicode' --- lib/ansible/template/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index dc74d2cb566..20287e078c6 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -22,7 +22,7 @@ __metaclass__ = type import ast import re -from six import string_types +from six import string_types, text_type, binary_type from jinja2 import Environment from jinja2.loaders import FileSystemLoader from jinja2.exceptions import TemplateSyntaxError, UndefinedError @@ -217,10 +217,10 @@ class Templar: # Don't template unsafe variables, instead drop them back down to # their constituent type. if hasattr(variable, '__UNSAFE__'): - if isinstance(variable, unicode): - return unicode(variable) - elif isinstance(variable, str): - return str(variable) + if isinstance(variable, text_type): + return text_type(variable) + elif isinstance(variable, binary_type): + return bytes(variable) else: return variable