From 729c954f3d315d8c6de0fcdc8373375d904b510a Mon Sep 17 00:00:00 2001 From: Jana Librova Date: Nov 13 2024 19:12:18 +0000 Subject: [PATCH 1/2] Fix for reading config files when contains UTF-8 chars Fixes: https://pagure.io/koji/issue/4191 --- diff --git a/koji/__init__.py b/koji/__init__.py index 7a8111e..39ee74c 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -25,6 +25,7 @@ from __future__ import absolute_import, division import base64 +import codecs import datetime import errno import hashlib @@ -2339,7 +2340,11 @@ def read_config_files(config_files, raw=False): elif strict: raise ConfigurationError("Config file %s can't be opened." % config_file) - config.read(cfgs) + if six.PY3: + config.read(cfgs, encoding="utf8") + else: + for cfg in cfgs: + config.readfp(codecs.open(cfg, 'r', 'utf8')) return config diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py index 691dcb4..42dd145 100644 --- a/tests/test_lib/test_utils.py +++ b/tests/test_lib/test_utils.py @@ -272,7 +272,6 @@ class ConfigFileTestCase(unittest.TestCase): mock.patch.stopall() def test_read_config_files(self): - # bad config_files for files in [0, False, @@ -298,7 +297,7 @@ class ConfigFileTestCase(unittest.TestCase): self.assertTrue(isinstance(conf, six.moves.configparser.ConfigParser.__class__)) self.real_parser_clz.assert_called_once() - self.real_parser_clz.return_value.read.assert_called_once_with([files]) + self.real_parser_clz.return_value.read.assert_called_once_with([files], encoding='utf8') # list as config_files self.reset_mock() @@ -306,7 +305,7 @@ class ConfigFileTestCase(unittest.TestCase): koji.read_config_files(files) self.real_parser_clz.assert_called_once() - self.real_parser_clz.return_value.read.assert_called_once_with(files) + self.real_parser_clz.return_value.read.assert_called_once_with(files, encoding='utf8') # tuple as config_files self.reset_mock() @@ -369,7 +368,8 @@ class ConfigFileTestCase(unittest.TestCase): ['test1.conf', 'gooddir/test1-1.conf', 'gooddir/test1-2.conf', - 'test2.conf']) + 'test2.conf'], + encoding='utf8') self.assertEqual(self.manager.isdir.call_count, 5) self.assertEqual(self.manager.isfile.call_count, 6) self.assertEqual(self.manager.access.call_count, 4) @@ -1282,7 +1282,8 @@ class TestRmtree(unittest.TestCase): @patch('koji.util._rmtree') def test_rmtree_directory(self, _rmtree): - """ Tests that the koji.util._rmtree_nofork function returns nothing when the path is a directory. + """ Tests that the koji.util._rmtree_nofork function returns nothing + when the path is a directory. """ stat = mock.MagicMock() stat.st_dev = 'dev' @@ -1552,7 +1553,8 @@ class TestRmtree(unittest.TestCase): @mock.patch('os.unlink') @mock.patch('os.waitpid') @mock.patch('os._exit') - def test_rmtree_parent_logfail(self, _exit, waitpid, unlink, fork, rmtree_nofork, logsend, mkstemp): + def test_rmtree_parent_logfail(self, _exit, waitpid, unlink, fork, rmtree_nofork, logsend, + mkstemp): log = self.tempdir + '/rmtree-log.jsonl' fd = os.open(log, os.O_RDWR | os.O_CREAT) mkstemp.return_value = fd, log @@ -1844,6 +1846,7 @@ class TestRmtree2(unittest.TestCase): os.makedirs('%s/a/%s/c/d/%s/e/f/%s/g/h' % (dirname, i, j, k)) sync = threading.Event() + def do_rmtree(dirname): sync.wait() koji.util.rmtree(dirname) @@ -1870,6 +1873,7 @@ class TestRmtree2(unittest.TestCase): os.makedirs('%s/a/%s/c/d/%s/e/f/%s/g/h' % (dirname, i, j, k)) sync = threading.Event() + def do_rmtree(dirname): sync.wait() koji.util.rmtree(dirname) @@ -1895,6 +1899,7 @@ class TestRmtree2(unittest.TestCase): os.makedirs('%s/a/%s/c/d/%s/e/f/%s/g/h' % (dirname, i, j, k)) sync = multiprocessing.Event() + def do_rmtree(dirname): sync.wait() koji.util.rmtree(dirname) @@ -2048,6 +2053,7 @@ class TestFormatShellCmd(unittest.TestCase): for inp, out in cases: self.assertEqual(koji.util.format_shell_cmd(inp, text_width=40), out) + class TestExtractBuildTask(unittest.TestCase): def test_valid_binfos(self): binfos = [ diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py index f945925..e63af95 100644 --- a/tests/test_plugins/test_runroot_builder.py +++ b/tests/test_plugins/test_runroot_builder.py @@ -24,47 +24,47 @@ else: CONFIG1 = { - 'paths': { - 'default_mounts': '/mnt/archive,/mnt/workdir', - 'safe_roots': '/mnt/workdir/tmp', - 'path_subs': - '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory', - }, - 'path0': { - 'mountpoint': '/mnt/archive', - 'path': 'archive.org:/vol/archive', - 'fstype': 'nfs', - 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', - }} + 'paths': { + 'default_mounts': '/mnt/archive,/mnt/workdir', + 'safe_roots': '/mnt/workdir/tmp', + 'path_subs': + '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory', + }, + 'path0': { + 'mountpoint': '/mnt/archive', + 'path': 'archive.org:/vol/archive', + 'fstype': 'nfs', + 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + }} CONFIG2 = { - 'paths': { - 'default_mounts': '/mnt/archive,/mnt/workdir', - 'safe_roots': '/mnt/workdir/tmp', - 'path_subs': - '\n' - '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory\n' - '/mnt/archve/workdir,/mnt/workdir\n', - }, - 'path0': { - 'mountpoint': '/mnt/archive', - 'path': 'archive.org:/vol/archive', - 'fstype': 'nfs', - 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', - }, - 'path1': { - 'mountpoint': '/mnt/workdir', - 'path': 'archive.org:/vol/workdir', - 'fstype': 'nfs', - 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', - }, - 'path2': { - 'mountpoint': '/mnt/prehistoric_disk', - 'path': 'archive.org:/vol/prehistoric_disk', - 'fstype': 'nfs', - 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', - }} + 'paths': { + 'default_mounts': '/mnt/archive,/mnt/workdir', + 'safe_roots': '/mnt/workdir/tmp', + 'path_subs': + '\n' + '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory\n' + '/mnt/archve/workdir,/mnt/workdir\n', + }, + 'path0': { + 'mountpoint': '/mnt/archive', + 'path': 'archive.org:/vol/archive', + 'fstype': 'nfs', + 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + }, + 'path1': { + 'mountpoint': '/mnt/workdir', + 'path': 'archive.org:/vol/workdir', + 'fstype': 'nfs', + 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + }, + 'path2': { + 'mountpoint': '/mnt/prehistoric_disk', + 'path': 'archive.org:/vol/prehistoric_disk', + 'fstype': 'nfs', + 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + }} class FakeConfigParser(object): @@ -75,7 +75,7 @@ class FakeConfigParser(object): else: self.CONFIG = copy.deepcopy(config) - def read(self, path): + def read(self, path, encoding): return def sections(self): @@ -105,8 +105,7 @@ class TestRunrootConfig(unittest.TestCase): options.workdir = '/tmp/nonexistentdirectory' with self.assertRaises(koji.GenericError) as cm: runroot.RunRootTask(123, 'runroot', {}, session, options) - self.assertEqual(cm.exception.args[0], - "bad config: missing options in path0 section") + self.assertEqual(cm.exception.args[0], "bad config: missing options in path0 section") @mock.patch(CONFIG_PARSER) def test_bad_config_absolute_path(self, config_parser): @@ -119,7 +118,8 @@ class TestRunrootConfig(unittest.TestCase): with self.assertRaises(koji.GenericError) as cm: runroot.RunRootTask(123, 'runroot', {}, session, options) self.assertEqual(cm.exception.args[0], - "bad config: all paths (default_mounts, safe_roots, path_subs) needs to be absolute: ") + "bad config: all paths (default_mounts, safe_roots, path_subs) " + "needs to be absolute: ") @mock.patch(CONFIG_PARSER) def test_valid_config(self, config_parser): @@ -186,13 +186,14 @@ class TestMounts(unittest.TestCase): # valid item self.assertEqual(self.t._get_path_params('/mnt/archive', 'rw'), - ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) + ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', + 'rw,hard,intr,nosuid,nodev,noatime,tcp')) @mock.patch('koji._open_text_file') @mock.patch('os.path.isdir') @mock.patch('runroot.log_output') def test_do_mounts(self, log_output, is_dir, open_mock): - log_output.return_value = 0 # successful mount + log_output.return_value = 0 # successful mount # no mounts, don't do anything self.t.logger = mock.MagicMock() @@ -202,8 +203,7 @@ class TestMounts(unittest.TestCase): # mountpoint has no absolute_path with self.assertRaises(koji.GenericError) as cm: self.t.do_mounts('rootdir', [('nfs:nfs', 'relative_path', 'nfs', '')]) - self.assertEqual(cm.exception.args[0], - "invalid mount point: relative_path") + self.assertEqual(cm.exception.args[0], "invalid mount point: relative_path") # cover missing opts self.t.do_mounts('rootdir', [('nfs:nfs', '/mnt/archive', 'nfs', None)]) @@ -212,23 +212,24 @@ class TestMounts(unittest.TestCase): log_output.reset_mock() mounts = [self.t._get_path_params('/mnt/archive')] self.t.do_mounts('rootdir', mounts) - log_output.assert_called_once_with(self.session, 'mount', - ['mount', '-t', 'nfs', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp', - 'archive.org:/vol/archive/', 'rootdir/mnt/archive'], - '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log', - 'tasks/123/123', append=True, logerror=True) + log_output.assert_called_once_with( + self.session, 'mount', + ['mount', '-t', 'nfs', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp', + 'archive.org:/vol/archive/', 'rootdir/mnt/archive'], + '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log', 'tasks/123/123', + append=True, logerror=True) # mount command failed log_output.reset_mock() log_output.return_value = 1 - #self.t.undo_mounts = mock.MagicMock() + # self.t.undo_mounts = mock.MagicMock() mounts = [self.t._get_path_params('/mnt/archive')] with self.assertRaises(koji.GenericError) as cm: self.t.do_mounts('rootdir', mounts) self.assertEqual(cm.exception.args[0], - 'Unable to mount rootdir/mnt/archive: mount -t nfs -o' - ' ro,hard,intr,nosuid,nodev,noatime,tcp archive.org:/vol/archive/' - ' rootdir/mnt/archive was killed by signal 1') + 'Unable to mount rootdir/mnt/archive: mount -t nfs -o' + ' ro,hard,intr,nosuid,nodev,noatime,tcp archive.org:/vol/archive/' + ' rootdir/mnt/archive was killed by signal 1') # bind ok log_output.return_value = 0 @@ -237,11 +238,12 @@ class TestMounts(unittest.TestCase): mount[3] += ',bind' is_dir.return_value = True self.t.do_mounts('rootdir', [mount]) - log_output.assert_called_once_with(self.session, 'mount', - ['mount', '-t', 'none', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp,bind', - 'archive.org:/vol/archive/', 'rootdir/mnt/archive'], - '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log', - 'tasks/123/123', append=True, logerror=True) + log_output.assert_called_once_with( + self.session, 'mount', + ['mount', '-t', 'none', '-o', 'ro,hard,intr,nosuid,nodev,noatime,tcp,bind', + 'archive.org:/vol/archive/', 'rootdir/mnt/archive'], + '/tmp/nonexistentdirectory/tasks/123/123/do_mounts.log', 'tasks/123/123', + append=True, logerror=True) # bind - target doesn't exist mount = list(self.t._get_path_params('/mnt/archive')) @@ -250,7 +252,7 @@ class TestMounts(unittest.TestCase): with self.assertRaises(koji.GenericError) as cm: self.t.do_mounts('rootdir', [mount]) self.assertEqual(cm.exception.args[0], - "No such directory or mount: archive.org:/vol/archive/") + "No such directory or mount: archive.org:/vol/archive/") # bg option forbidden log_output.reset_mock() @@ -259,8 +261,7 @@ class TestMounts(unittest.TestCase): is_dir.return_value = False with self.assertRaises(koji.GenericError) as cm: self.t.do_mounts('rootdir', [mount]) - self.assertEqual(cm.exception.args[0], - "bad config: background mount not allowed") + self.assertEqual(cm.exception.args[0], "bad config: background mount not allowed") def test_do_extra_mounts(self): self.t.do_mounts = mock.MagicMock() @@ -288,7 +289,6 @@ class TestMounts(unittest.TestCase): self.t.do_extra_mounts('rootdir', ['/mnt/workdir/tmp/../xyz']) self.t.do_mounts.assert_not_called() - @mock.patch('koji._open_text_file') @mock.patch('runroot.scan_mounts') @mock.patch('os.unlink') @@ -318,10 +318,11 @@ class TestMounts(unittest.TestCase): with self.assertRaises(koji.GenericError) as cm: self.t.undo_mounts('rootdir') self.assertEqual(cm.exception.args[0], - 'Unable to unmount: mountpoint: error, mount_2: error, mount_1: error') + 'Unable to unmount: mountpoint: error, mount_2: error, mount_1: error') os_unlink.assert_not_called() + class TestHandler(unittest.TestCase): @mock.patch(CONFIG_PARSER) def setUp(self, config_parser): @@ -335,7 +336,7 @@ class TestHandler(unittest.TestCase): options = mock.MagicMock() options.workdir = '/tmp/nonexistentdirectory' - #options.topurl = 'http://topurl' + # options.topurl = 'http://topurl' options.topurls = None self.t = runroot.RunRootTask(123, 'runroot', {}, self.session, options) self.t.config['default_mounts'] = ['default_mount'] @@ -371,10 +372,9 @@ class TestHandler(unittest.TestCase): 'id': 1, } self.session.host.getHost.return_value = {'arches': 'x86_64'} - self.t.handler('tag_name', 'noarch', 'command', weight=10.0, - repo_id=1, packages=['rpm_a', 'rpm_b'], new_chroot=True, - mounts=['/mnt/a'], skip_setarch=True, - upload_logs=['log_1', 'log_2']) + self.t.handler('tag_name', 'noarch', 'command', weight=10.0, repo_id=1, + packages=['rpm_a', 'rpm_b'], new_chroot=True, mounts=['/mnt/a'], + skip_setarch=True, upload_logs=['log_1', 'log_2']) # calls self.session.host.setTaskWeight.assert_called_once_with(self.t.id, 10.0) @@ -382,15 +382,18 @@ class TestHandler(unittest.TestCase): self.session.getBuildConfig.assert_called_once_with('tag_name') self.session.repoInfo.assert_called_once_with(1, strict=True) self.session.host.subtask.assert_not_called() - runroot.BuildRoot.assert_called_once_with(self.session, self.t.options, - 'tag_name', 'x86_64', self.t.id, repo_id=1, setup_dns=True, - internal_dev_setup=None) + runroot.BuildRoot.assert_called_once_with( + self.session, self.t.options, 'tag_name', 'x86_64', self.t.id, repo_id=1, + setup_dns=True, internal_dev_setup=None) self.session.host.setBuildRootState.assert_called_once_with(678, 'BUILDING') self.br.mock.assert_has_calls([ mock.call(['--install', 'rpm_a', 'rpm_b']), - mock.call(['--new-chroot', '--arch', 'arch', '--chroot', '--', '/bin/sh', '-c', '{ command; } < /dev/null 2>&1 | /usr/bin/tee /builddir/runroot.log; exit ${PIPESTATUS[0]}']), + mock.call(['--new-chroot', '--arch', 'arch', '--chroot', '--', '/bin/sh', '-c', + '{ command; } < /dev/null 2>&1 | /usr/bin/tee /builddir/runroot.log; ' + 'exit ${PIPESTATUS[0]}']), ]) - self.session.host.updateBuildRootList.assert_called_once_with(678, self.br.getPackageList()) + self.session.host.updateBuildRootList.assert_called_once_with( + 678, self.br.getPackageList()) self.t.do_mounts.assert_called_once_with('/rootdir', ['default_mount']) self.t.do_extra_mounts.assert_called_once_with('/rootdir', ['/mnt/a']) self.t.uploadFile.assert_has_calls([ From e1b55ec23cced14562980f83929a50b8358e99f1 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 20 2024 14:37:07 +0000 Subject: [PATCH 2/2] add unit test --- diff --git a/tests/common.py b/tests/common.py new file mode 100644 index 0000000..47c4fdb --- /dev/null +++ b/tests/common.py @@ -0,0 +1,32 @@ +import locale +from functools import wraps + + +class mylocale: + """Locale context manager. Resets to user default at exit.""" + + def __init__(self, value=None): + self.value = value + + def __enter__(self): + locale.setlocale(locale.LC_ALL, locale=self.value) + + def __exit__(self, _type, value, traceback): + # This resets to user default. Implementing a proper save and restore + # for locale is quite tricky, and this serves our testing needs. + locale.setlocale(locale.LC_ALL, "") + # return false so we don't eat exceptions + return False + + def __call__(self, func): + """When called, act as a decorator""" + + @wraps(func) + def wrapped(*args, **kwargs): + with self: + return func(*args, **kwargs) + + return wrapped + + +# the end diff --git a/tests/test_lib/data/cfg/uni1.conf b/tests/test_lib/data/cfg/uni1.conf new file mode 100644 index 0000000..e5b76ea --- /dev/null +++ b/tests/test_lib/data/cfg/uni1.conf @@ -0,0 +1,7 @@ +# comment ěščřčž +[koji] +# key_file = äöüß +; test comment with unicode 🫠 +# test comment with unicode ☢️ +setting = hello +value = world 🌍 diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py index 42dd145..9d59ba1 100644 --- a/tests/test_lib/test_utils.py +++ b/tests/test_lib/test_utils.py @@ -3,7 +3,6 @@ from __future__ import absolute_import import calendar from datetime import datetime import errno -import locale import logging import tempfile import threading @@ -26,6 +25,7 @@ import six import koji import koji.util +from ..common import mylocale class EnumTestCase(unittest.TestCase): @@ -407,6 +407,23 @@ class ConfigFileTestCase(unittest.TestCase): self.assertEqual(listdir_mock.call_count, 2) +class ConfigFileTestCase2(unittest.TestCase): + """Additional tests for config file reading functions""" + + def setUp(self): + self.datadir = os.path.dirname(__file__) + '/data/cfg' + + def tearDown(self): + mock.patch.stopall() + + def test_unicode(self): + fn = self.datadir + '/uni1.conf' + if not os.path.exists(fn): + raise Exception('missing config') + with mylocale(value='C'): + koji.read_config_files(fn) + + class MavenUtilTestCase(unittest.TestCase): """Test maven relative functions""" maxDiff = None @@ -750,10 +767,9 @@ class MavenUtilTestCase(unittest.TestCase): config.read_file(conf_file) return config + @mylocale(('en_US', 'UTF-8')) def test_formatChangelog(self): """Test formatChangelog function""" - # force locale to compare 'expect' value - locale.setlocale(locale.LC_ALL, ('en_US', 'UTF-8')) data = [ { 'author': 'Happy Koji User - 1.1-1', @@ -787,8 +803,6 @@ class MavenUtilTestCase(unittest.TestCase): result = koji.util.formatChangelog(data) self.assertMultiLineEqual(expect, result) - locale.setlocale(locale.LC_ALL, "") - def test_parseTime(self): """Test parseTime function""" now = datetime.now()