#90 python3: improve Python 3.x compatibility
Merged 7 years ago by cqi. Opened 7 years ago by pavlix.
pavlix/rpkg python3  into  master

python3: fix container usage
Pavel Šimerda • 7 years ago  
python3: fix string types
Pavel Šimerda • 7 years ago  
python3: fix configparser usage
Pavel Šimerda • 7 years ago  
file modified
+7 -8
@@ -25,8 +25,6 @@ 

  import sys

  import tempfile

  

- from ConfigParser import ConfigParser

- 

  import gssapi

  

  from osbs.api import OSBS
@@ -409,9 +407,9 @@ 

                      raise rpkgError('Unable to find remote push url: %s' % e)

          if isinstance(url, six.text_type):

              # GitPython >= 1.0 return unicode. It must be encoded to string.

-             self._push_url = url.encode('utf-8')

-         else:

              self._push_url = url

+         else:

+             self._push_url = url.decode('utf-8')

  

      @property

      def commithash(self):
@@ -492,7 +490,8 @@ 

          """Get the local arch as defined by rpm"""

  

          proc = subprocess.Popen(['rpm --eval %{_arch}'], shell=True,

-                                 stdout=subprocess.PIPE)

+                                 stdout=subprocess.PIPE,

+                                 universal_newlines=True)

          self._localarch = proc.communicate()[0].strip('\n')

  

      @property
@@ -536,8 +535,8 @@ 

                  #     self._module_name = "/".join(parts.path.split("/")[-2:])

                  module_name = posixpath.basename(parts.path)

  

-                 if module_name.endswith(b'.git'):

-                     module_name = module_name[:-len(b'.git')]

+                 if module_name.endswith('.git'):

+                     module_name = module_name[:-len('.git')]

                  self._module_name = module_name

                  return

          except rpkgError:
@@ -2548,7 +2547,7 @@ 

  

      def container_build_setup(self, get_autorebuild=None,

                                set_autorebuild=None):

-         cfp = ConfigParser.SafeConfigParser()

+         cfp = configparser.SafeConfigParser()

          if os.path.exists(self.osbs_config_filename):

              cfp.read(self.osbs_config_filename)

  

file modified
+1 -1
@@ -1403,7 +1403,7 @@ 

                                               quiet=self.args.q)

              while True:

                  all_done = True

-                 for task_id, task in tasks.items():

+                 for task_id, task in list(tasks.items()):

                      changed = task.update()

                      if not task.is_done():

                          all_done = False

@@ -20,7 +20,5 @@ 

          moduledir = os.path.join(self.path, self.module)

          cmd.path = moduledir

  

-         # pycurl can't handle unicode variable

-         # module_name needs to be a byte string

-         self.assertNotEqual(type(cmd.module_name), six.text_type)

-         self.assertEqual(type(cmd.module_name), six.binary_type)

+         self.assertNotEqual(type(cmd.module_name), six.binary_type)

+         self.assertEqual(type(cmd.module_name), six.text_type)

Fix a few issues with Python 3.x compatibility. With this patch
and the respective patches in other packages I can successfully
run at least some 'fedpkg' commands.

See also:

https://pagure.io/fedpkg/pull-request/43

rebased

7 years ago

rebased

7 years ago

Why need to make this change?

b is used for compatibility, that is both Python 2 and 3 treats '.git' as bytes instead of bytes and str respectively.

No need to convert string. Meanwhile, this causes an error in Python 3.

>>> str.decode
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'str' has no attribute 'decode'

It is a relative import that ensures utlis.py is loaded properly with Python 3.x.

Warning: Could not install krbV module. Kerberos support will be disabled.
Traceback (most recent call last):
  File "/usr/lib/python-exec/python3.4/fedpkg", line 13, in <module>
    from fedpkg.__main__ import main
  File "/usr/lib64/python3.4/site-packages/fedpkg/__init__.py", line 14, in <module>
    from . import cli
  File "/usr/lib64/python3.4/site-packages/fedpkg/cli.py", line 12, in <module>
    from pyrpkg.cli import cliClient
  File "/usr/lib64/python3.4/site-packages/pyrpkg/cli.py", line 26, in <module>
    import utils
ImportError: No module named 'utils'

The changes are related. It doesn't make sense to evaluate this change separately.

I'm not getting any such error in Python 3. Without this change fedpkg verrel returns b'strongswan'-5.5.0-2.fc26 instead of strongswan-5.5.0-2.fc26.

rebased

7 years ago

rebased

7 years ago

Next line imports configparser correctly.

We need to return a unicode text string from Popen.communicate().

We need a unicode text string in self._push_url in Python 3.x.

You cannot pass bytes to endswith() method on a unicode text string in Python 3.x.

Relative import is needed with Python 3.x

Warning: Could not install krbV module. Kerberos support will be disabled.
Traceback (most recent call last):
  File "/usr/bin/fedpkg", line 54, in <module>
    exec(data)
  File "<string>", line 13, in <module>
  File "/usr/lib64/python3.4/site-packages/fedpkg/__init__.py", line 14, in <module>
    from . import cli
  File "/usr/lib64/python3.4/site-packages/fedpkg/cli.py", line 12, in <module>
    from pyrpkg.cli import cliClient
  File "/usr/lib64/python3.4/site-packages/pyrpkg/cli.py", line 26, in <module>
    import utils
ImportError: No module named 'utils'

I believe I addressed all your concerns with a new rebase and code comments.

I'm not so sure about changing type of push_url. There is a reason for the way it is: pycurl on EPEL 7 was not able to handle unicode values (bug #1241059). We can't drop Python 2 support completely, especially given that rpkg depends on e.g. koji which isn't available on Python 3.x.

I have already provided two different fixes for the same issue and successfully tested both of them with Python 3.4 and Python 2.7. I have also provided Python 3.x fixes for fedpkg and koji client.

Since now on I am an active user of fedpkg and related tools using Python 3.x and my patches, while still keeping Python 2.x compatibility unless of course there's a bug that I overlooked. Is there anything else I can do for you?

For anyone who would like to use Fedora tools before respective upstreams accept Python 3.x patches, I'm now tracking this on GitHub with the gentoo-fedora overlay.

https://github.com/pavlix/gentoo-fedora/issues/2

Ok, since you say it works and my testing did not show any issues either, I'm +1 on the patch. Let's fix possible issues when (if) they show up.

I agree to use unicode all the time even with Python 2.

I think this piece of original code is necessary, but the the return value from GitPython should be decoded into Unicode instead of encoding to a byte string, since we have to support RHEL6 and only GitPython-0.3.2-0.6.RC1.el6 is available in EPEL, that returns string in str rather than Unicode.

Tests need to be updated. There is an error.

======================================================================
FAIL: test_name_is_not_unicode (commands.test_package_name.CommandPackageNameTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/jenkins/workspace/pyrpkg/Builders/F24/test/commands/test_package_name.py", line 25, in test_name_is_not_unicode
    self.assertNotEqual(type(cmd.module_name), six.text_type)
AssertionError: <type 'unicode'> == <type 'unicode'>
>>  raise self.failureException("<type 'unicode'> == <type 'unicode'>")

-------------------- >> begin captured logging << --------------------
pyrpkg: DEBUG: Cloning file:///tmp/rpkg-tests.lKfDIO/gitroot/module1
pyrpkg: DEBUG: Running: git clone file:///tmp/rpkg-tests.lKfDIO/gitroot/module1 --origin origin
pyrpkg: DEBUG: Creating repo object from /tmp/rpkg-tests.lKfDIO/module1
pyrpkg: DEBUG: Could not determine the remote name: 'git config --get branch.TODO.remote' returned with exit code 1
pyrpkg: DEBUG: Falling back to default remote name 'origin'
--------------------- >> end captured logging << ---------------------

----------------------------------------------------------------------
Ran 108 tests in 4.864s

FAILED (failures=1)

Full log is available here, http://jenkins.fedorainfracloud.org/job/pyrpkg/Builders=F24/144/console

I'm not exactly sure what change you're requesting. Could you please make the change upstream (or in a branch) including the test change and I would then rebase my patch on top of it and do my own testing?

@cqi Could you please make the changes according to our exchange of inline comments so that I can rebase my work on top of that?

@cqi Could you please make the changes according to our exchange of inline comments so that I can rebase my work on top of that?

@pavlix if you agree with what I said, I think you could restore the lines deleted by you and make the related changes

-         if isinstance(url, six.text_type):
-             # GitPython >= 1.0 return unicode. It must be encoded to string.
-             self._push_url = url.encode('utf-8')
-         else:
-             self._push_url = url

@cqi As I said, I would like you to clarify what you are requesting, or make the changes so that I can rebase on top of it.

@pavlix okay. What I meant is to give unicode string to self._push_url in order to ensure all strings in rpkg are in Unicode, even in Python 2. So, I think above lines code should be changed to

if isinstance(url, six.text_type):
    self._push_url = url
else:
    self._push_url = url.decode('utf-8')

rebased

7 years ago

rebased

7 years ago

I updated the patch accordingly, split it into multiple patches and accompanied by detailed information. I see that the master got already improved, so we now have just a bunch of small and well documented changes that I think we can push now. :)

rebased

7 years ago

1 new commit added

  • python3: fix container usage
7 years ago

I'm still +1, but a unittest needs to be updated since the type of push_url was changed:

http://jenkins.fedorainfracloud.org/job/pyrpkg/178/Builders=F24/console

======================================================================
FAIL: test_name_is_not_unicode (commands.test_package_name.CommandPackageNameTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/jenkins/workspace/pyrpkg/Builders/F24/tests/commands/test_package_name.py", line 25, in test_name_is_not_unicode
    self.assertNotEqual(type(cmd.module_name), six.text_type)
AssertionError: <type 'unicode'> == <type 'unicode'>
>>  raise self.failureException("<type 'unicode'> == <type 'unicode'>")

4 new commits added

  • python3: fix container usage
  • python3: fix string types
  • python3: use relative import
  • python3: fix configparser usage
7 years ago

I'm still +1, but a unittest needs to be updated since the type of push_url was changed:

I made a trivial change in the test in the "fix string types" commit. Tests now work OK with Python 2.x on Fedora.

@pavlix Can you rebase this patch? Then, it can be merged.

rebased

7 years ago

@pavlix Still The pull-request cannot be merged due to conflicts. It seems you need to rebase on the latest master branch.

@pavlix Still The pull-request cannot be merged due to conflicts. It seems you need to rebase on the latest master branch.

@cqi Sorry for that. It should be settled by now.

rebased

7 years ago

Pull-Request has been merged by cqi

7 years ago