Closed Bug 1257607 Opened 4 years ago Closed 4 years ago

Add a Version type to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We've got a lot of checks for minimum versions of tools in configure. glandium added one for perl recently:
https://dxr.mozilla.org/mozilla-central/rev/5cfc10c14aaea2a449f74dcbb366179a45442dd6/moz.configure#92

This is doing simple string comparison right now, which is not great for version comparisons since it falls down for common cases like:
>>> '2.10.1' < '2.2'
True

We should have a little Version type to use in moz.configure. I wrote one that's a very simple wrapper around distutils.version.LooseVersion. It just adds two things:
1) Deal with comparing against unicode. LooseVersion can handle being compared against byte strings, but it type checks against `StrType`, so unicode strings from moz.configure fail. This is important so you can write simple comparisons like `perl_version > '5.006'` in moz.configure files.
2) Expose the major,minor,etc version bits easily. I added a `__getitem__` which returns the integer version components in order, returning a default value of zero for any that are unset, so that you can write code like:
```
v = Version('1.2')
major = v[0] # == 1
minor = v[1] # == 2
build = v[2] # == 0
```

I didn't feel right putting a class definition inside a @template, so I put the actual class definition in python/mozbuild/mozbuild/configure/__init__.py and made a template expose it.
Assignee: nobody → ted
This change adds a `Version` type to moz.configure which is a small
wrapper around `distutils.version.Version`. It's suitable for wrapping
version numbers in configure checks and doing equality or greater-than
less-than comparisons in a sensible way.

Review commit: https://reviewboard.mozilla.org/r/40841/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40841/
Attachment #8731798 - Flags: review?(mh+mozilla)
Comment on attachment 8731798 [details]
MozReview Request: bug 1257607 - Add Version type to moz.configure. r?glandium

https://reviewboard.mozilla.org/r/40841/#review37483

::: build/moz.configure/util.configure:90
(Diff revision 1)
>      return result
>  
> +@template
> +@advanced
> +def Version(v):
> +    from mozbuild.configure import Version as _Version

I think this should go in a new mozbuild.configure.util module.

::: python/mozbuild/mozbuild/configure/__init__.py:39
(Diff revision 1)
> +    v = Version('1.2')
> +    v[0] == 1
> +    v[1] == 2
> +    v[3] == 0

mmmm I think something like v.major, v.minor, v.micro would be nicer (and I don't think we'll ever need below micro). And this should actually stop at the first non-integer. Because 1.2a3 and 1.2.3 shouldn't contain the same info...

::: python/mozbuild/mozbuild/configure/__init__.py:56
(Diff revision 1)
> +
> +    def __cmp__(self, other):
> +        # LooseVersion checks isinstance(StringType), so work around it.
> +        if isinstance(other, unicode):
> +            other = other.encode('ascii')
> +        return LooseVersion.__cmp__(self, other)

return super(Version, self).__cmp__(other)
Attachment #8731798 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/40841/#review37483

> mmmm I think something like v.major, v.minor, v.micro would be nicer (and I don't think we'll ever need below micro). And this should actually stop at the first non-integer. Because 1.2a3 and 1.2.3 shouldn't contain the same info...

I used `major, minor, patch` which is the terminology http://semver.org/ uses. I made it stop at the first non-integer and zero the rest.

> return super(Version, self).__cmp__(other)

Sadly this doesn't work because `super` requires a new-style class and the base class of LooseVersion is an old-style class:
http://svn.python.org/view/python/branches/release27-maint/Lib/distutils/version.py?revision=82503&view=markup#l32
Comment on attachment 8731798 [details]
MozReview Request: bug 1257607 - Add Version type to moz.configure. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40841/diff/1-2/
Attachment #8731798 - Flags: review?(mh+mozilla)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Sadly this doesn't work because `super` requires a new-style class and the
> base class of LooseVersion is an old-style class:
> http://svn.python.org/view/python/branches/release27-maint/Lib/distutils/
> version.py?revision=82503&view=markup#l32

Man, they really decided to screw up that class.
Attachment #8731798 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8731798 [details]
MozReview Request: bug 1257607 - Add Version type to moz.configure. r?glandium

https://reviewboard.mozilla.org/r/40841/#review37615

::: python/mozbuild/mozbuild/configure/util.py:27
(Diff revision 2)
> +    def __init__(self, version):
> +        # Can't use super, LooseVersion's base class is not a new-style class.
> +        LooseVersion.__init__(self, version)
> +        # Take the first three integer components, stopping at the first
> +        # non-integer and padding the rest with zeroes.
> +        (self.major,

(self.major, self.minor, self.patch) = (
   v if v else 0
  ...
)

would allow less deep indentation, and probably more things on each line.

maybe shorter (haven't actually counted) and clearer (subjective):
(self.major, self.minor, self.patch) = itertools.islice(itertools.chain(
    itertools.takewhile(lambda x:isinstance(x, int), self.version),
    (0, 0, 0)), 3)

(or (0,)*3 instead of (0,0,0))
https://reviewboard.mozilla.org/r/40841/#review37615

> (self.major, self.minor, self.patch) = (
>    v if v else 0
>   ...
> )
> 
> would allow less deep indentation, and probably more things on each line.
> 
> maybe shorter (haven't actually counted) and clearer (subjective):
> (self.major, self.minor, self.patch) = itertools.islice(itertools.chain(
>     itertools.takewhile(lambda x:isinstance(x, int), self.version),
>     (0, 0, 0)), 3)
> 
> (or (0,)*3 instead of (0,0,0))

That definitely reads nicer, although the islice vs. [:3] is pretty subjective. The chain + (0,0,0) is great though.
Comment on attachment 8731798 [details]
MozReview Request: bug 1257607 - Add Version type to moz.configure. r?glandium

https://reviewboard.mozilla.org/r/40841/#review37669

::: moz.configure:87
(Diff revision 2)
>      @checking('for minimum required perl version >= %s' % min_version)
>      @advanced
>      def get_perl_version(perl):
>          import subprocess
>          try:
> -            return subprocess.check_output([perl, '-e', 'print $]'])
> +            return Version(subprocess.check_output([perl, '-e', 'print $]']))

Gah, I totally overlooked this. You should have left it as it was, because you actually broke the test with this.
Because perl's $] is not a "normal" version.
Attachment #8731798 - Flags: review+
Oh, ugh. I think this actually should be OK if we just change the version string we're passing in to include the full version, right, like:
perl_version_check('5.006000')
https://hg.mozilla.org/mozilla-central/rev/e1e446ee6882
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.