PR#207: Fix unicode issue for update command in Python 3 - fedpkg

fedpkg

#207 Fix unicode issue for update command in Python 3

Merged 6 years ago by cqi. Opened 6 years ago by cqi.

cqi/fedpkg fix-fedpkg-update-206 into master

Copy pip-pycurl to ensure pycurl is installed correctly

Chenxiong Qi • 6 years ago

045350d

Fix unicode issue for update command in Python 3

Chenxiong Qi • 6 years ago

fdeeaf0

MANIFEST.in

file modified

		`@@ -3,6 +3,7 @@`
		`include doc/release-guide.markdown`
		`include test/utils.py`
		`include test *.conf`
		`+ include pip-pycurl`
		`recursive-include conf *`
		`include tox.ini`
		`include requirements.txt tests-requirements.txt`

fedpkg/cli.py

file modified

+11 -13

		`@@ -14,6 +14,7 @@`
		`from pyrpkg.cli import cliClient`
		`import argparse`
		`import hashlib`
		`+ import io`
		`import os`
		`import re`
		`import json`
		`@@ -286,14 +287,17 @@`
		`suggest_reboot=False`
		`"""`

		`- bodhi_args = {'nvr': self.cmd.nvr,`
		`- 'bugs': '',`
		`- 'descr': 'Here is where you give an explanation'`
		`- ' of your update.'}`
		`+ bodhi_args = {`
		`+ 'nvr': self.cmd.nvr,`
		`+ 'bugs': six.u(''),`
		`+ 'descr': six.u(`
		`+ 'Here is where you give an explanation of your update.')`
		`+ }`

		`# Extract bug numbers from the latest changelog entry`
		`self.cmd.clog()`
		`- with open('clog', 'r') as f:`
		`+ clog_file = os.path.join(self.cmd.path, 'clog')`
		`+ with io.open(clog_file, encoding='utf-8') as f:`
		`clog = f.read()`
		`bugs = re.findall(r'#([0-9]*)', clog)`
		`if bugs:`
		`@@ -303,12 +307,6 @@`
		`bodhi_args['descr'], bodhi_args['changelog'] = \`
		`self._format_update_clog(clog)`

		`- if six.PY2:`
		`- # log may contain unicode characters, convert log to unicode string`
		`- # to ensure text can be wrapped correctly in follow step.`
		`- bodhi_args['descr'] = bodhi_args['descr'].decode('utf-8')`
		`- bodhi_args['changelog'] = bodhi_args['changelog'].decode('utf-8')`
		`-`
		`template = textwrap.dedent(template) % bodhi_args`

		`# Calculate the hash of the unaltered template`
		`@@ -317,8 +315,8 @@`
		`orig_hash = orig_hash.hexdigest()`

		`# Write out the template`
		`- with open('bodhi.template', 'w') as f:`
		`- f.write(template.encode('utf-8'))`
		`+ with io.open('bodhi.template', 'w', encoding='utf-8') as f:`
		`+ f.write(template)`

		`# Open the template in a text editor`
		`editor = os.getenv('EDITOR', 'vi')`

pip-pycurl

file added

+19

		`@@ -0,0 +1,19 @@`
		`+ #!/bin/bash`
		`+`
		`+ if python -c "import pycurl" &>/dev/null; then`
		`+ exit 0`
		`+ fi`
		`+`
		`+ # We need to build pycurl with openssl in Fedora 27, otherwise nss should be`
		`+ # used.`
		`+ # See also: https://fedoraproject.org/wiki/Changes/libcurlBackToOpenSSL`
		`+`
		`+ dist=$(rpm --eval "%{dist}")`
		`+ dist=${dist:3}`
		`+`
		`+ if [ $dist -ge 27 ]; then`
		`+ install_option="--with-openssl"`
		`+ else`
		`+ install_option="--with-nss"`
		`+ fi`
		`+ pip install -v -I --install-option="${install_option}" "pycurl>=7.19"`

test/test_cli.py

file modified

+37 -21

		`@@ -10,6 +10,8 @@`
		`# option) any later version. See http://www.gnu.org/copyleft/gpl.html for`
		`# the full text of the license.`

		`+ import io`
		`+ import os`
		`import sys`
		`import json`
		`from datetime import datetime, timedelta`
		`@@ -25,7 +27,7 @@`
		`from utils import CliTestCase`
		`from fedpkg.bugzilla import BugzillaClient`

		`- from mock import call, patch, mock_open, PropertyMock, Mock`
		`+ from mock import call, patch, PropertyMock, Mock`


		`class TestUpdate(CliTestCase):`
		`@@ -36,7 +38,7 @@`

		`self.nvr_patcher = patch('fedpkg.Commands.nvr',`
		`new_callable=PropertyMock,`
		`- return_value='fedpkg-1.29-9')`
		`+ return_value='fedpkg-1.29-9.fc27')`
		`self.mock_nvr = self.nvr_patcher.start()`

		`self.run_command_patcher = patch('fedpkg.Commands._run_command')`
		`@@ -55,13 +57,20 @@`
		`self.os_environ_patcher = patch.dict('os.environ', {'EDITOR': 'vi'})`
		`self.os_environ_patcher.start()`

		`- self.fake_clog = '\n'.join([`
		`+ self.fake_clog = list(six.moves.map(six.u, [`
		`'Add tests for command update',`
churchyard commented 6 years ago I wonder why don't you use unicode literals here instead?
		`'New command update - #1000',`
		`- 'Fix tests - #2000'`
		`- ])`
		`+ 'Fix tests - #2000',`
		`+ '处理一些Unicode字符číář',`
		`+ ]))`
		`+ clog_file = os.path.join(self.cloned_repo_path, 'clog')`
		`+ with io.open(clog_file, 'w', encoding='utf-8') as f:`
		`+ f.write(os.linesep.join(self.fake_clog))`

		`def tearDown(self):`
		`+ if os.path.exists('bodhi.template'):`
		`+ os.unlink('bodhi.template')`
		`+ os.unlink(os.path.join(self.cloned_repo_path, 'clog'))`
		`self.os_environ_patcher.stop()`
		`self.clog_patcher.stop()`
		`self.get_bodhi_version_patcher.stop()`
		`@@ -73,17 +82,24 @@`
		`with patch('sys.argv', new=cli_cmd):`
		`return self.new_cli(name=name, cfg=cfg)`

		`- def create_bodhi_update(self, cli):`
		`- mocked_open = mock_open(read_data=self.fake_clog)`
		`- with patch('six.moves.builtins.open', mocked_open):`
		`- with patch('os.unlink') as unlink:`
		`- cli.update()`
		`-`
		`- # Ensure these files are removed in the end`
		`- unlink.assert_has_calls([`
		`- call('bodhi.template'),`
		`- call('clog')`
		`- ])`
		`+ def assert_bodhi_update(self, cli):`
		`+ with patch('os.unlink') as unlink:`
		`+ cli.update()`
		`+`
		`+ unlink.assert_has_calls([`
		`+ call('bodhi.template'),`
		`+ call('clog')`
		`+ ])`
		`+`
		`+ with io.open('bodhi.template', encoding='utf-8') as f:`
		`+ bodhi_template = f.read()`
		`+ self.assertTrue(self.mock_nvr.return_value in bodhi_template)`
		`+ self.assertTrue('1000,2000' in bodhi_template)`
		`+ self.assertTrue(self.fake_clog[0] in bodhi_template)`
		`+ rest_clog = os.linesep.join([`
		`+ six.u('# {0}').format(line) for line in self.fake_clog[1:]`
		`+ ])`
		`+ self.assertTrue(rest_clog in bodhi_template)`

		`def test_fail_if_missing_config_options(self):`
		`cli_cmd = ['fedpkg', '--path', self.cloned_repo_path, 'update']`
		`@@ -106,7 +122,7 @@`
		`cli = self.get_cli(cli_cmd)`
		`six.assertRaisesRegex(`
		`self, rpkgError, 'No bodhi update details saved',`
		`- self.create_bodhi_update, cli)`
		`+ self.assert_bodhi_update, cli)`

		`self.mock_run_command.assert_called_once_with(`
		`['vi', 'bodhi.template'], shell=True)`
		`@@ -123,7 +139,7 @@`
		`cli_cmd = ['fedpkg', '--path', self.cloned_repo_path, 'update']`

		`cli = self.get_cli(cli_cmd)`
		`- self.create_bodhi_update(cli)`
		`+ self.assert_bodhi_update(cli)`

		`self.mock_run_command.assert_has_calls([`
		`call(['vi', 'bodhi.template'], shell=True),`
		`@@ -146,7 +162,7 @@`
		`cli = self.get_cli(cli_cmd)`
		`six.assertRaisesRegex(`
		`self, rpkgError, 'Could not generate update request',`
		`- self.create_bodhi_update, cli)`
		`+ self.assert_bodhi_update, cli)`

		`@patch('os.path.isfile', return_value=True)`
		`@patch('hashlib.new')`
		`@@ -163,7 +179,7 @@`
		`cli = self.get_cli(cli_cmd)`
		`six.assertRaisesRegex(`
		`self, rpkgError, 'This system has bodhi v4, which is unsupported',`
		`- self.create_bodhi_update, cli)`
		`+ self.assert_bodhi_update, cli)`

		`@patch('os.path.isfile', return_value=True)`
		`@patch('hashlib.new')`
		`@@ -180,7 +196,7 @@`
		`cli = self.get_cli(cli_cmd,`
		`name='fedpkg-stage',`
		`cfg='fedpkg-stage.conf')`
		`- self.create_bodhi_update(cli)`
		`+ self.assert_bodhi_update(cli)`

		`self.mock_run_command.assert_has_calls([`
		`call(['vi', 'bodhi.template'], shell=True),`

tox.ini

file modified

+1 -1

		`@@ -6,7 +6,7 @@`
		`-r{toxinidir}/requirements.txt`
		`-r{toxinidir}/tests-requirements.txt`
		`commands =`
		`- pip install -I --install-option="--with-openssl" "pycurl>=7.19"`
		`+ {toxinidir}/pip-pycurl`
		`nosetests {posargs}`

		`[testenv:py26]`

cqi commented 6 years ago

With this patch, in update command, unicode string is used consistently
through the whole process. String written into file by write() method is
handled carefully with or without encoding to byte string for Python 2
and 3 individually.

Issue reported in #206 was not caught due to write() was mocked. This
patch also fixes this problem. Now, during the test, real bodhi.template
and clog file are written into and read from file system. Tests are
updated accordingly.

Fixes #206

Signed-off-by: Chenxiong Qi cqi@redhat.com

cqi commented 6 years ago

pretty please pagure-ci rebuild

lsedlar commented 6 years ago

Looks good to me. Running nose in my Py2.7 virtualenv works, but tox is reporting some errors. They don't look related to this PR.

Edited 6 years ago by lsedlar

1 new commit added

Copy pip-pycurl to ensure pycurl is installed correctly

6 years ago

cqi commented 6 years ago

It should be

======================================================================
FAIL: Test verify_sls with an SL that is not June 1st or December 1st. An
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cqi/code/fedpkg/test/test_utils.py", line 116, in test_verify_sls_invalid_date
    'December 1st'.format(eol))
AssertionError:

which happens randomly with Python 2.7. Wired. I'll look into it later.

churchyard commented on line 21 of fedpkg/cli.py 6 years ago

for consistency, you can read it binary a decode everywhere. you should also handle situation, when the encoding fails

churchyard commented on line 42 of fedpkg/cli.py 6 years ago

For consistency, you can also open bytes and encode everywhere.

churchyard commented 6 years ago

For cleaner code here I suggest you read about File I/O in The Conservative Python 3 Porting Guide.

cqi commented on line 21 of fedpkg/cli.py 6 years ago

@churchyard Thanks for your comment. I thought consistency of using unicode would be better than the byte string, because less or none fedpkg (even rpkg) handle byte strings. Moving to Python 3 completely is the ultimate goal eventually, then str (unicode string) will be the default.

churchyard commented on line 21 of fedpkg/cli.py 6 years ago

np ;)

rebased onto 1feed9afbef09510530fef18850f7e1d594e9191

6 years ago

cqi commented 6 years ago

@churchyard Updated patch by using io.open.

churchyard commented 6 years ago

Seems that only somewhere...

churchyard commented 6 years ago

Nevertheless the original patch fixed it for me.

cqi commented 6 years ago