search

icatproject / icat.utils

Setup scripts and a few useful bits of Java code.
Other
0 stars 2 forks source link

Python 3 compatibility #10

Closed stuartpullinger closed 4 years ago

stuartpullinger commented 4 years ago

This PR aims to allow the ICAT scripts to be run under Python 2 and Python 3.

The StringIO.StringIO import has been changed to io.StringIO. StringIO is only used to store the stdout and stderr on Windows. I don't yet know if this is the correct approach in this case so it will requre some understanding and testing before this is merged.

RKrahl commented 4 years ago

Tested on Windows 10 Python 2.7 and 3.8 and Manjaro Linux Python 2.7 and 3.8 using

python -m py_compile src/main/resources/setup_utils.py src/main/scripts/icatdoi src/main/scripts/setup-glassfish.py

That only checks for compiler errors, e.g. if the scripts are syntactically correct, but not if they are working correctly.

stuartpullinger commented 4 years ago

Yep - we still need to test to be sure. We will do that before merging. As a first test, this was done to check that the syntax changes were accepted in both Python 2 and 3.

RKrahl commented 4 years ago

I get an error from setup_utils.py on Linux for both, Python 2 and Python 3.

Python 2:

$ ./setup install
Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib64/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 669, in run
    out.write(line)
TypeError: unicode argument expected, got 'str'

Exception in thread Thread-3:
Traceback (most recent call last):
  File "/usr/lib64/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 669, in run
    out.write(line)
TypeError: unicode argument expected, got 'str'

Traceback (most recent call last):
  File "./setup", line 7, in <module>
    actions, arg, props = getActions("setup.properties", [])
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 110, in getActions
    if container == "Glassfish": actions = GlassfishActions(props, options)
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 425, in __init__
    self.domain = self.getAsadminProperty("property.administrative.domain.name")
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 649, in getAsadminProperty
    return out.splitlines()[0].split("=")[1]
IndexError: list index out of range

Python 3:

$ python3 ./setup install    
Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 669, in run
    out.write(line)
TypeError: string argument expected, got 'bytes'

Exception in thread Thread-3:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 669, in run
    out.write(line)
TypeError: string argument expected, got 'bytes'

Traceback (most recent call last):
  File "./setup", line 7, in <module>
    actions, arg, props = getActions("setup.properties", [])
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 110, in getActions
    if container == "Glassfish": actions = GlassfishActions(props, options)
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 425, in __init__
    self.domain = self.getAsadminProperty("property.administrative.domain.name")
  File "/hmi/payara41/apps/ids.storage_test/setup_utils.py", line 649, in getAsadminProperty
    return out.splitlines()[0].split("=")[1]
IndexError: list index out of range

The error message already points to the problem: the script launches an external command with subprocess.Popen(). In order to capture the output, ir reads stdout from that process and writes it into an io.StringIO object. But reading from a process yields bytes, while io.StringIO expects str as input (respectively str versus unicode in the case of Python 2).

What puzzled me for quite a while was: why does I also get the error with Python 2, since the old version did work with Python 2 and this program logic didn't change. The answer is: the old version was writing into a StringIO.StringIO object and not into an io.StringIO object. These two classes are not the same in Python 2 and behave differently with respect to the expected input.

RKrahl commented 4 years ago

After I fixed the bytes versus str issue mentioned above locally, I ran into another issue with Python 3:

$ python3 ./setup install
Traceback (most recent call last):
  File "./setup", line 25, in <module>
    if arg in ["CONFIGURE", "INSTALL"]: actions.configure(prop_name, prop_list) 
  File "/hmi/payara41/apps/icat.server/setup_utils.py", line 228, in configure
    if self.verbosity > 1:
TypeError: '>' not supported between instances of 'NoneType' and 'int'

The cause was found to the following: the setup_utils.py script adds a -v command line switch to increase verbosity of the diagnostic output:

    parser.add_option("--verbose", "-v", help="produce more output - this may appear twice to get even more", action="count")

The corresponding option, stored in the attribute Actions.verbosity, is taken as an integer, see the line of code cited in the error message above. But if the -v switch has not been used at all, the value defaults to None. With Python 2 this even works (more by chance then by intention, I guess):

>>> # Python 2:
>>> None > 1
False

Python 3 is stricter and does not allow that weird kind of comparison:

>>> # Python 3
>>> None > 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '>' not supported between instances of 'NoneType' and 'int'
stuartpullinger commented 4 years ago

Good find, Rolf! I think we should add a default=0 keyword argument which should work in both 2 and 3 (Docs).

We should also consider moving from optparse to argparse as the former is deprecated - but maybe that should be addressed in a separate PR after this one.

RKrahl commented 4 years ago

Good find, Rolf! I think we should add a default=0 keyword argument which should work in both 2 and 3 (Docs).

Yes, would be an option. Since I didn't wanted to bother with the old optparse, I just added another fix to this in #11.

And yes, that whole setup_utils.py script could do with a major revision.