1184067 - make mozinfo.os_version a special string that can be compared with < / >

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Julien Pagès (:parkouss)]

This would be useful to test manifests so we could check for example:

os_version > '10.2'

See comments in bug 980788 for a detailed description.

Also we should expose the Version class used for that (in mozinfo.py) so others can reuse it.

Comment 1

[(not currently active) Ted Mielczarek]

Specifically, where we're currently setting info['os_version'] = a_string:
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/testing/mozbase/mozinfo/mozinfo/mozinfo.py#l51

we should instead do something like:
info['os_version'] = Version((major, minor))

and have the Version class handle comparisons appropriately. One tricky bit to note is that for your example to work you'll have to support comparing a Version against a string, so you'll probably want to parse the string into a Version and do the comparison that way.

A good test for this would be something that currently doesn't work well: OS X versions. Right now we do string comparisons, so:
>>> '10.10' < '10.2'
True

If you have os_version == '10.10', and a conditional with os_version < '10.9', that should evaluate to False, but right now it'd be True.

Comment 2

[Julien Pagès (:parkouss)]

What about this ?

>>> from distutils.version import LooseVersion
>>> v = LooseVersion('1.5.6')
>>> v < '1.5.7'
True
>>> v <= '1.5.6'
True
>>> v < '1.5.5'
False
>>> print v
1.5.6

This seems to work just fine.

>>> v = LooseVersion('10.10')
>>> v < '10.2'
False

Comment 3

[(not currently active) Ted Mielczarek]

That seems totally reasonable. Let's just use that, then!

Comment 4

[Julien Pagès (:parkouss)]

Attachment: 1184067.patch

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8634074 [details] [diff] [review]
1184067.patch

Review of attachment 8634074 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 6

[Julien Pagès (:parkouss)]

Thanks Ted!

Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc4b268f167e

Comment 7

[Julien Pagès (:parkouss)]

Try is green.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44571696245f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for breaking web-platform-tests.
https://treeherder.mozilla.org/logviewer.html#?job_id=11831024&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab61ece2428c

Comment 10

[Andrew Halberstadt [:ahal]]

So I'm assuming run_info here contains the mozinfo dict:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/harness/wptrunner/wptrunner.py#161

There are two bugs:

1. wptrunner assumes that mozinfo.info will be valid json. This was previously true, but is no longer the case. We may add other class types in the future, so best not to make assumptions.

2. mozlog is trying to parse each line of the stack one at a time for some reason, thus all the 'Test harness output was not a valid structured log message' messages. The stack should be in a single log record, not sure why it isn't.

Comment 11

[James Graham [:jgraham]]

(In reply to Andrew Halberstadt [:ahal] from comment #10)

> 1. wptrunner assumes that mozinfo.info will be valid json. This was
> previously true, but is no longer the case. We may add other class types in
> the future, so best not to make assumptions.

I tend to disagree; it seems entirely reasonable that it is possible to serialize this as JSON. If you want a richer internal representation it makes sense to provide methods to (de)serialize the information as JSON. In this specific case why not just inherit from str and override the comparison methods:

class VersionString(str):
    def __init__(self, *args):
        self.version = version.LooseVersion(self)

    def __cmp__(self, other):
        return self.version.__cmp__(self.version, other)

> 2. mozlog is trying to parse each line of the stack one at a time for some
> reason, thus all the 'Test harness output was not a valid structured log
> message' messages. The stack should be in a single log record, not sure why
> it isn't.

Because when there's an exception in mozlog we can't call reentrantly into the logger without causing a deadlock, at least in the current setup. Therefore the error message just gets printed. The fact that this prints the "not a valid structured log" warning message more than once is a bug that I had a patch for somewhere.

Comment 12

[Julien Pagès (:parkouss)]

(In reply to James Graham [:jgraham] from comment #11)
> class VersionString(str):
>     def __init__(self, *args):
>         self.version = version.LooseVersion(self)
> 
>     def __cmp__(self, other):
>         return self.version.__cmp__(self.version, other)

This unfortunately does not work, as __cmp__ is only called when rich comparison operators are not implemented:

https://docs.python.org/2/reference/datamodel.html#object.__cmp__

The str class seems to implement rich comparison operators (my tests are broken), so we would have to reimplement every method. Not a big deal, just mentioning it.

Comment 13

[Andrew Halberstadt [:ahal]]

I guess in this case inheriting str is fine. If there's ever a need to put something more complicated into mozinfo in the future we'll run into this problem again, but I guess that's probably YAGNI.

Comment 14

[Andrew Halberstadt [:ahal]]

(In reply to Julien Pagès from comment #12)
> The str class seems to implement rich comparison operators (my tests are
> broken), so we would have to reimplement every method. Not a big deal, just
> mentioning it.

I just stumbled across:
https://docs.python.org/2.7/library/functools.html#functools.total_ordering

May come in handy :)

Comment 15

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1184067 - Make mozinfo.os_version a special string that can be compared with < / >. r?ahal

Bug 1184067 - Make mozinfo.os_version a special string that can be compared with < / >. r?ahal

Comment 16

[Julien Pagès (:parkouss)]

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> (In reply to Julien Pagès from comment #12)
> > The str class seems to implement rich comparison operators (my tests are
> > broken), so we would have to reimplement every method. Not a big deal, just
> > mentioning it.
> 
> I just stumbled across:
> https://docs.python.org/2.7/library/functools.html#functools.total_ordering
> 
> May come in handy :)

Oh, yep good to know! Well I already did the patch, so I left it as it was. I am not sure it would make a big difference as total_ordering requires 2 functions anyway (that's still 4 less though).

I let you judge it, I don't mind changing it if you think it would be better. :)

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8638027 [details]
MozReview Request: Bug 1184067 - Make mozinfo.os_version a special string that can be compared with < / >. r?ahal

https://reviewboard.mozilla.org/r/14019/#review12545

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py:23
(Diff revision 1)
> +class StringVersion(str):

I think this class should go into a new file.

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py:31
(Diff revision 1)
> +    component_re = re.compile(r'(\d+ | [a-z]+ | \.)', re.VERBOSE)
> +
> +    def __init__(self, vstring):
> +        str.__init__(self, vstring)
> +        components = filter(lambda x: x and x != '.',
> +                            self.component_re.split(vstring))
> +        for i in range(len(components)):
> +            try:
> +                components[i] = int(components[i])
> +            except ValueError:
> +                pass

We should still use LooseVersion under the hood instead of implementing our own logic.

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py:53
(Diff revision 1)
> +    # rich comparison methods
> +
> +    def __lt__(self, other):
> +        return self.version < self.__to_version(other)
> +
> +    def __le__(self, other):
> +        return self.version <= self.__to_version(other)
> +
> +    def __eq__(self, other):
> +        return self.version == self.__to_version(other)

Personally I'd use functools.total_ordering, but it's ok to leave it too if you want.

Comment 18

[Julien Pagès (:parkouss)]

Attachment: 1184067_2.patch

So I fixed the patch, according to review comments. :)

Unfortunately total_ordering does not work here (it breaks the unit tests), and looking at the implementation it seems to me that this is because it create new methods on the class only if there is None already. So this is a no op here since the base str define everything. Kind of strange since I don't see how it is better than __cmp__ in that case ?

Googling around, __cmp__ support is removed in 3.0. So I suppose this is here to be compatible between versions:

https://mail.python.org/pipermail/python-dev/2009-March/087000.html

Too bad, I liked this __cmp__ stuff.

Anyway, this was just for information, not really relative to this bug. :)

Comment 19

[Julien Pagès (:parkouss)]

I think we are good, so pushed to try (with web platform tests):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efbcfbb4297e

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 8638479 [details] [diff] [review]
1184067_2.patch

Review of attachment 8638479 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, lgtm!

::: testing/mozbase/mozinfo/mozinfo/string_version.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from distutils.version import LooseVersion

If you're worried about import speed, you could import this directly in __init__. Probably not worth it though.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/873bab8f2d7b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/873bab8f2d7b

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/066e84750afd

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Ignore comment 23.  It was a bad commit message that pulsebot dutifully put here.