| Message ID | 20181015141453.32632-9-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Make them work for both Python 2 and 3 | expand |
On 10/15/18 10:14 AM, Max Reitz wrote: > There are two imports that need to be modified when running the iotests > under Python 3: One is StringIO, which no longer exists; instead, the > StringIO class comes from the io module, so import it from there. The > other is the ConfigParser, which has just been renamed to configparser. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/iotests.py | 8 ++++++-- > tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 7ca94e9278..a64ea90fb4 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], > > # We need to filter out the time taken from the output so that qemu-iotest > # can reliably diff the results against master output. > - import StringIO > + if sys.version_info.major >= 3: > + from io import StringIO > + else: > + from StringIO import StringIO > + > if debug: > output = sys.stdout > verbosity = 2 > sys.argv.remove('-d') > else: > - output = StringIO.StringIO() > + output = StringIO() > > logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) > > diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py > index d45e2e0a6a..6b2d659dee 100755 > --- a/tests/qemu-iotests/nbd-fault-injector.py > +++ b/tests/qemu-iotests/nbd-fault-injector.py > @@ -48,7 +48,10 @@ import sys > import socket > import struct > import collections > -import ConfigParser > +if sys.version_info.major >= 3: > + import configparser > +else: > + import ConfigParser as configparser > > FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB > > @@ -225,7 +228,7 @@ def parse_config(config): > return rules > > def load_rules(filename): > - config = ConfigParser.RawConfigParser() > + config = configparser.RawConfigParser() > with open(filename, 'rt') as f: > config.readfp(f, filename) > return parse_config(config) > This may be a type of culture clash (on my side, due to not enough QEMU culture), but shouldn't this be applied before anything else on this series? I mean, PATCH 1/9 is supposed to fix the reliability aspects of nbd-fault-injector under Python 3, but without this patch, it won't actually run on Python 3. Regards, - Cleber.
On Mon, Oct 15, 2018 at 02:59:28PM -0400, Cleber Rosa wrote: > > > On 10/15/18 10:14 AM, Max Reitz wrote: > > There are two imports that need to be modified when running the iotests > > under Python 3: One is StringIO, which no longer exists; instead, the > > StringIO class comes from the io module, so import it from there. The > > other is the ConfigParser, which has just been renamed to configparser. > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > --- > > tests/qemu-iotests/iotests.py | 8 ++++++-- > > tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > > index 7ca94e9278..a64ea90fb4 100644 > > --- a/tests/qemu-iotests/iotests.py > > +++ b/tests/qemu-iotests/iotests.py > > @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], > > > > # We need to filter out the time taken from the output so that qemu-iotest > > # can reliably diff the results against master output. > > - import StringIO > > + if sys.version_info.major >= 3: > > + from io import StringIO > > + else: > > + from StringIO import StringIO > > + > > if debug: > > output = sys.stdout > > verbosity = 2 > > sys.argv.remove('-d') > > else: > > - output = StringIO.StringIO() > > + output = StringIO() > > > > logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) > > > > diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py > > index d45e2e0a6a..6b2d659dee 100755 > > --- a/tests/qemu-iotests/nbd-fault-injector.py > > +++ b/tests/qemu-iotests/nbd-fault-injector.py > > @@ -48,7 +48,10 @@ import sys > > import socket > > import struct > > import collections > > -import ConfigParser > > +if sys.version_info.major >= 3: > > + import configparser > > +else: > > + import ConfigParser as configparser > > > > FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB > > > > @@ -225,7 +228,7 @@ def parse_config(config): > > return rules > > > > def load_rules(filename): > > - config = ConfigParser.RawConfigParser() > > + config = configparser.RawConfigParser() > > with open(filename, 'rt') as f: > > config.readfp(f, filename) > > return parse_config(config) > > > > This may be a type of culture clash (on my side, due to not enough QEMU > culture), but shouldn't this be applied before anything else on this series? > > I mean, PATCH 1/9 is supposed to fix the reliability aspects of > nbd-fault-injector under Python 3, but without this patch, it won't > actually run on Python 3. Both patches are required to make the code run on Python 3, and they don't depend on each other. So I think the order doesn't matter. But I think the order chosen by Max is slightly more intuitive: having explicit mentions of Python 3 in the code would be confusing if we didn't fix the compatibility issues on patches 1-7 first.
On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote: > There are two imports that need to be modified when running the iotests > under Python 3: One is StringIO, which no longer exists; instead, the > StringIO class comes from the io module, so import it from there. The > other is the ConfigParser, which has just been renamed to configparser. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/iotests.py | 8 ++++++-- > tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 7ca94e9278..a64ea90fb4 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], > > # We need to filter out the time taken from the output so that qemu-iotest > # can reliably diff the results against master output. > - import StringIO > + if sys.version_info.major >= 3: > + from io import StringIO > + else: > + from StringIO import StringIO Considering that io.StringIO exists on Python 2.7, a comment explaining why exactly it doesn't work would be nice. But this shouldn't block this workaround, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + > if debug: > output = sys.stdout > verbosity = 2 > sys.argv.remove('-d') > else: > - output = StringIO.StringIO() > + output = StringIO() > > logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) > > diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py > index d45e2e0a6a..6b2d659dee 100755 > --- a/tests/qemu-iotests/nbd-fault-injector.py > +++ b/tests/qemu-iotests/nbd-fault-injector.py > @@ -48,7 +48,10 @@ import sys > import socket > import struct > import collections > -import ConfigParser > +if sys.version_info.major >= 3: > + import configparser > +else: > + import ConfigParser as configparser > > FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB > > @@ -225,7 +228,7 @@ def parse_config(config): > return rules > > def load_rules(filename): > - config = ConfigParser.RawConfigParser() > + config = configparser.RawConfigParser() > with open(filename, 'rt') as f: > config.readfp(f, filename) > return parse_config(config) > -- > 2.17.1 >
On 10/15/18 5:17 PM, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote: >> There are two imports that need to be modified when running the iotests >> under Python 3: One is StringIO, which no longer exists; instead, the >> StringIO class comes from the io module, so import it from there. The >> other is the ConfigParser, which has just been renamed to configparser. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/iotests.py | 8 ++++++-- >> tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- >> 2 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >> index 7ca94e9278..a64ea90fb4 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], >> >> # We need to filter out the time taken from the output so that qemu-iotest >> # can reliably diff the results against master output. >> - import StringIO >> + if sys.version_info.major >= 3: >> + from io import StringIO >> + else: >> + from StringIO import StringIO > > Considering that io.StringIO exists on Python 2.7, a comment > explaining why exactly it doesn't work would be nice. > Another possibility, that I find self explanatory: import io if sys.version_info.major >= 3: output = io.StringIO() else: output = io.BytesIO() - Cleber. > But this shouldn't block this workaround, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > >> + >> if debug: >> output = sys.stdout >> verbosity = 2 >> sys.argv.remove('-d') >> else: >> - output = StringIO.StringIO() >> + output = StringIO() >> >> logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) >> >> diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py >> index d45e2e0a6a..6b2d659dee 100755 >> --- a/tests/qemu-iotests/nbd-fault-injector.py >> +++ b/tests/qemu-iotests/nbd-fault-injector.py >> @@ -48,7 +48,10 @@ import sys >> import socket >> import struct >> import collections >> -import ConfigParser >> +if sys.version_info.major >= 3: >> + import configparser >> +else: >> + import ConfigParser as configparser >> >> FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB >> >> @@ -225,7 +228,7 @@ def parse_config(config): >> return rules >> >> def load_rules(filename): >> - config = ConfigParser.RawConfigParser() >> + config = configparser.RawConfigParser() >> with open(filename, 'rt') as f: >> config.readfp(f, filename) >> return parse_config(config) >> -- >> 2.17.1 >> >
On Mon, Oct 15, 2018 at 08:05:02PM -0400, Cleber Rosa wrote: > > > On 10/15/18 5:17 PM, Eduardo Habkost wrote: > > On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote: > >> There are two imports that need to be modified when running the iotests > >> under Python 3: One is StringIO, which no longer exists; instead, the > >> StringIO class comes from the io module, so import it from there. The > >> other is the ConfigParser, which has just been renamed to configparser. > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > >> --- > >> tests/qemu-iotests/iotests.py | 8 ++++++-- > >> tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- > >> 2 files changed, 11 insertions(+), 4 deletions(-) > >> > >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > >> index 7ca94e9278..a64ea90fb4 100644 > >> --- a/tests/qemu-iotests/iotests.py > >> +++ b/tests/qemu-iotests/iotests.py > >> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], > >> > >> # We need to filter out the time taken from the output so that qemu-iotest > >> # can reliably diff the results against master output. > >> - import StringIO > >> + if sys.version_info.major >= 3: > >> + from io import StringIO > >> + else: > >> + from StringIO import StringIO > > > > Considering that io.StringIO exists on Python 2.7, a comment > > explaining why exactly it doesn't work would be nice. > > > > Another possibility, that I find self explanatory: > > import io > > if sys.version_info.major >= 3: > output = io.StringIO() > else: > output = io.BytesIO() Looks nice and clean. But I'm not sure all output sent to `output` when running with Python 2 will be byte strings. What if `unittest.TextTestRunner` tries to write unicode strings to `output`?
On 15.10.18 20:59, Cleber Rosa wrote: > > > On 10/15/18 10:14 AM, Max Reitz wrote: >> There are two imports that need to be modified when running the iotests >> under Python 3: One is StringIO, which no longer exists; instead, the >> StringIO class comes from the io module, so import it from there. The >> other is the ConfigParser, which has just been renamed to configparser. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/iotests.py | 8 ++++++-- >> tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- >> 2 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >> index 7ca94e9278..a64ea90fb4 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], >> >> # We need to filter out the time taken from the output so that qemu-iotest >> # can reliably diff the results against master output. >> - import StringIO >> + if sys.version_info.major >= 3: >> + from io import StringIO >> + else: >> + from StringIO import StringIO >> + >> if debug: >> output = sys.stdout >> verbosity = 2 >> sys.argv.remove('-d') >> else: >> - output = StringIO.StringIO() >> + output = StringIO() >> >> logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) >> >> diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py >> index d45e2e0a6a..6b2d659dee 100755 >> --- a/tests/qemu-iotests/nbd-fault-injector.py >> +++ b/tests/qemu-iotests/nbd-fault-injector.py >> @@ -48,7 +48,10 @@ import sys >> import socket >> import struct >> import collections >> -import ConfigParser >> +if sys.version_info.major >= 3: >> + import configparser >> +else: >> + import ConfigParser as configparser >> >> FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB >> >> @@ -225,7 +228,7 @@ def parse_config(config): >> return rules >> >> def load_rules(filename): >> - config = ConfigParser.RawConfigParser() >> + config = configparser.RawConfigParser() >> with open(filename, 'rt') as f: >> config.readfp(f, filename) >> return parse_config(config) >> > > This may be a type of culture clash (on my side, due to not enough QEMU > culture), but shouldn't this be applied before anything else on this series? > > I mean, PATCH 1/9 is supposed to fix the reliability aspects of > nbd-fault-injector under Python 3, but without this patch, it won't > actually run on Python 3. I don't mind, I followed no specific order. Well, patch 9 is patch 9 because it is the largest one, so I didn't want to discourage people before the end of the series. :-) Max
On 16.10.18 02:12, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 08:05:02PM -0400, Cleber Rosa wrote: >> >> >> On 10/15/18 5:17 PM, Eduardo Habkost wrote: >>> On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote: >>>> There are two imports that need to be modified when running the iotests >>>> under Python 3: One is StringIO, which no longer exists; instead, the >>>> StringIO class comes from the io module, so import it from there. The >>>> other is the ConfigParser, which has just been renamed to configparser. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> tests/qemu-iotests/iotests.py | 8 ++++++-- >>>> tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- >>>> 2 files changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>>> index 7ca94e9278..a64ea90fb4 100644 >>>> --- a/tests/qemu-iotests/iotests.py >>>> +++ b/tests/qemu-iotests/iotests.py >>>> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], >>>> >>>> # We need to filter out the time taken from the output so that qemu-iotest >>>> # can reliably diff the results against master output. >>>> - import StringIO >>>> + if sys.version_info.major >= 3: >>>> + from io import StringIO >>>> + else: >>>> + from StringIO import StringIO >>> >>> Considering that io.StringIO exists on Python 2.7, a comment >>> explaining why exactly it doesn't work would be nice. Oh, it does exist? I didn't know. O:-) So I suppose it's because the test runner emits just normal strings, which in 2.x are byte strings; but io's StringIO always expects Unicode strings. StringIO, OTOH, accepts both (and returns Unicode strings once you put a Unicode string into it). >> Another possibility, that I find self explanatory: >> >> import io >> >> if sys.version_info.major >= 3: >> output = io.StringIO() >> else: >> output = io.BytesIO() > > Looks nice and clean. > > But I'm not sure all output sent to `output` when running with > Python 2 will be byte strings. What if `unittest.TextTestRunner` > tries to write unicode strings to `output`? Hm. It doesn't look this way to me in practice. And considering that it only really prints a test summary, I don't think there are edge cases where it behaves differently. So I think using BytesIO() in 2.x is better. Max
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 7ca94e9278..a64ea90fb4 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[], # We need to filter out the time taken from the output so that qemu-iotest # can reliably diff the results against master output. - import StringIO + if sys.version_info.major >= 3: + from io import StringIO + else: + from StringIO import StringIO + if debug: output = sys.stdout verbosity = 2 sys.argv.remove('-d') else: - output = StringIO.StringIO() + output = StringIO() logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN)) diff --git a/tests/qemu-iotests/nbd-fault-injector.py b/tests/qemu-iotests/nbd-fault-injector.py index d45e2e0a6a..6b2d659dee 100755 --- a/tests/qemu-iotests/nbd-fault-injector.py +++ b/tests/qemu-iotests/nbd-fault-injector.py @@ -48,7 +48,10 @@ import sys import socket import struct import collections -import ConfigParser +if sys.version_info.major >= 3: + import configparser +else: + import ConfigParser as configparser FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB @@ -225,7 +228,7 @@ def parse_config(config): return rules def load_rules(filename): - config = ConfigParser.RawConfigParser() + config = configparser.RawConfigParser() with open(filename, 'rt') as f: config.readfp(f, filename) return parse_config(config)
There are two imports that need to be modified when running the iotests under Python 3: One is StringIO, which no longer exists; instead, the StringIO class comes from the io module, so import it from there. The other is the ConfigParser, which has just been renamed to configparser. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 8 ++++++-- tests/qemu-iotests/nbd-fault-injector.py | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-)