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

RESOLVED FIXED in Firefox 42

Status

Testing
Mozbase
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

unspecified
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
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.
(Assignee)

Comment 2

2 years ago
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
That seems totally reasonable. Let's just use that, then!
(Assignee)

Comment 4

2 years ago
Created attachment 8634074 [details] [diff] [review]
1184067.patch
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8634074 - Flags: review?(ted)
Comment on attachment 8634074 [details] [diff] [review]
1184067.patch

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

Nice!
Attachment #8634074 - Flags: review?(ted) → review+
(Assignee)

Comment 6

2 years ago
Thanks Ted!

Pushed to try:

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

Comment 7

2 years ago
Try is green.
Keywords: checkin-needed

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/44571696245f
Keywords: checkin-needed
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
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.
(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.
(Assignee)

Comment 12

2 years ago
(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.
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.
(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 :)
(Assignee)

Comment 15

2 years ago
Created attachment 8638027 [details]
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
Attachment #8638027 - Flags: review?(ahalberstadt)
(Assignee)

Comment 16

2 years ago
(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 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.
Attachment #8638027 - Flags: review?(ahalberstadt)
(Assignee)

Comment 18

2 years ago
Created attachment 8638479 [details] [diff] [review]
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. :)
Attachment #8634074 - Attachment is obsolete: true
Attachment #8638027 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8638479 - Flags: review?(ahalberstadt)
(Assignee)

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efbcfbb4297e
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.
Attachment #8638479 - Flags: review?(ahalberstadt) → review+

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/873bab8f2d7b
https://hg.mozilla.org/mozilla-central/rev/873bab8f2d7b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Comment 23

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

Comment 24

2 years ago
Ignore comment 23.  It was a bad commit message that pulsebot dutifully put here.
You need to log in before you can comment on or make changes to this bug.