Closed
Bug 1257607
Opened 8 years ago
Closed 8 years ago
Add a Version type to moz.configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ted, Assigned: ted)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Details |
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 | ||
Updated•8 years ago
|
Assignee: nobody → ted
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8731798 -
Flags: review?(mh+mozilla) → review+
Comment 6•8 years ago
|
||
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))
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e446ee68823667291e2d6136398f1c948b925d bug 1257607 - Add Version type to moz.configure. r=glandium
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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')
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e1e446ee6882
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•