Closed
Bug 812179
Opened 12 years ago
Closed 12 years ago
Remove hacks for Python < 2.6 in config/
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: yati.sagade)
References
Details
(Whiteboard: [mentor=ted][lang=python])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
yati.sagade
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
We have a lot of code scattered about the tree that works around Python 2.6 features not being available. I don't have an exhaustive list handy, but for example:
http://mxr.mozilla.org/mozilla-central/source/config/writemozinfo.py#70
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#21
There's also some code somewhere that works around features in the zipfile module that weren't available in 2.5, and I'm sure we have a fair amount of other things like this in the code as well.
Comment 1•12 years ago
|
||
We can also remove from __future__ import with_statement
Comment 2•12 years ago
|
||
Did we also have hacks for ignoring directories of a certain name in shutil.copytree ? This was added only in Python 2.6.
Comment 3•12 years ago
|
||
Python 2.6 introduces the modern exception handling syntax:
except Exception as e:
(as opposed to "exception Exception, e")
Other new features:
* multiprocessing (although it has issues on BSD, so watch out)
* str.format()
* abc package
* class decorators
* Many functions in os suck much less
Comment 4•12 years ago
|
||
> (as opposed to "exception Exception, e")
Drive-by nit-pick - this should be "except Exception, e"
Comment 5•12 years ago
|
||
os.path.relpath, too.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → yati.sagade
Status: NEW → ASSIGNED
Comment 6•12 years ago
|
||
The CPython source tree contains a script for automatically stripping outdated "from __future__" parameters.
It assumes whatever version of Python you run it with is the minimum required version. So, just run this script with Python 2.6 on the tree and we should be set!
http://hg.python.org/cpython/raw-file/4a17784f2fee/Tools/scripts/cleanfuture.py is the revision in the Python 2.6 branch.
Assignee | ||
Comment 7•12 years ago
|
||
Yes, this is a good idea, as most of the fixes are going to be removing unneeded __future__ imports. I just could neither find a package for, nor could I build python 2.6 on my Fedora box for some reason. Switched to Debian now. On it :)
Assignee | ||
Comment 8•12 years ago
|
||
This is a patch that removes (almost all) < 2.6 hacks from the config/ toplevel dir. Now changing old style string formatting ('%s' % 'blah') to str.format() is some task. I have converted a few files in config/, but those changes are not in this patch.
Attachment #693055 -
Flags: review+
Attachment #693055 -
Flags: feedback+
Comment 9•12 years ago
|
||
Comment on attachment 693055 [details] [diff] [review]
Patch for the config/ toplevel directory
Please request for review and/or feedback by setting to ? and selecting an appropriate reviewer. (I'm guessing Gregory might be a good start.)
Attachment #693055 -
Flags: review+
Attachment #693055 -
Flags: feedback+
Comment 10•12 years ago
|
||
> Please request for review and/or feedback by setting to ? and selecting an
> appropriate reviewer. (I'm guessing Gregory might be a good start.)
(or ted, since he's listed as a mentor on the whiteboard)
Assignee | ||
Updated•12 years ago
|
Attachment #693055 -
Flags: review?(ted)
Assignee | ||
Comment 11•12 years ago
|
||
Gary, thanks. Done :)
Reporter | ||
Comment 12•12 years ago
|
||
Comment on attachment 693055 [details] [diff] [review]
Patch for the config/ toplevel directory
Review of attachment 693055 [details] [diff] [review]:
-----------------------------------------------------------------
Overall this looks great, there's just one file I'd like you to exclude from the patch.
::: config/configobj.py
@@ -13,5 @@
> # Scripts maintained at http://www.voidspace.org.uk/python/index.shtml
> # For information about bugfixes, updates and support, please join the
> # ConfigObj mailing list:
> # http://lists.sourceforge.net/lists/listinfo/configobj-develop
> # Comments, suggestions and bug reports welcome.
It looks like we've imported this script directly from an upstream location, so we should probably just leave it alone. Sorry about that, I forgot we had this.
::: config/writemozinfo.py
@@ -73,5 @@
> # per-window private browsing
> d["perwindowprivatebrowsing"] = 'MOZ_PER_WINDOW_PRIVATE_BROWSING' in env and env['MOZ_PER_WINDOW_PRIVATE_BROWSING'] == '1'
> return d
>
> -#TODO: replace this with the json module when Python >= 2.6 is a requirement.
Hooray!
Attachment #693055 -
Flags: review?(ted) → review+
Assignee | ||
Comment 13•12 years ago
|
||
No problem, I am having a tough time with mercurial by the way :P I'll post a patch once I figure out how to rollback :)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #693055 -
Attachment is obsolete: true
Attachment #695329 -
Flags: review+
Assignee | ||
Comment 15•12 years ago
|
||
This patch also contains oldstyle formatting to str.format() changes on a couple files. I'll save doing that for all other files to a dedicated patch in itself.
Reporter | ||
Comment 17•12 years ago
|
||
Yati, was this ready to land? It looks like we dropped this on the floor.
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #695329 -
Attachment is obsolete: true
Attachment #719056 -
Flags: review+
Assignee | ||
Comment 19•12 years ago
|
||
Yupp, I'd fixed that in the patch. Anyway, there was an import problem which I've now fixed and attached the patch. Ted, is there a way I can selectively test this code on my end?
Reporter | ||
Comment 20•12 years ago
|
||
Someone should be able to push this patch to the try server for yout o make sure it doesn't break anything. (I don't have time right at the moment, but I could do it tomorrow if nobody beats me to it.)
Assignee | ||
Comment 21•12 years ago
|
||
No problem :)
Comment 22•12 years ago
|
||
Try run for c80c9487b6f3 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=c80c9487b6f3
Results (out of 18 total builds):
failure: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Ms2ger@gmail.com-c80c9487b6f3
Comment 23•12 years ago
|
||
Comment on attachment 719056 [details] [diff] [review]
This is the fix for the config/ directory
Review of attachment 719056 [details] [diff] [review]:
-----------------------------------------------------------------
::: config/Preprocessor.py
@@ +132,5 @@
> ln = self.context['LINE']
> if self.writtenLines != ln:
> + self.out.write('//@line {line} "{file}"{le}'.format(line=ln,
> + file=self.context['FILE'],
> + le=self.LE}))
The builds failed on Try because of the leftover '}' on the last line here.
I pushed to Try again with that line fixed:
https://tbpl.mozilla.org/?tree=Try&rev=ce04ad514347
Comment 24•12 years ago
|
||
Try run for ce04ad514347 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=ce04ad514347
Results (out of 18 total builds):
failure: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-ce04ad514347
Comment 25•12 years ago
|
||
Push to Try yet again after copying changed files to js/src/config to make check-sync-dirs.py happy. The latest push failed with this error which looks like it's caused by "import print_function" in pythonpath.py:
Traceback (most recent call last):
File "/builds/slave/try-lx-00000000000000000000000/build/config/pythonpath.py", line 58, in <module>
main(sys.argv[1:])
File "/builds/slave/try-lx-00000000000000000000000/build/config/pythonpath.py", line 50, in main
execfile(script, frozenglobals)
File "/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/dist/sdk/bin/header.py", line 540, in <module>
print >>depfd, "%s: %s" % (options.outfile, " ".join(deps))
TypeError: unsupported operand type(s) for >>: 'builtin_function_or_method' and 'file'
https://tbpl.mozilla.org/php/getParsedLog.php?id=20157606&tree=Try
Comment 26•12 years ago
|
||
Try run for 0f09e272f670 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=0f09e272f670
Results (out of 19 total builds):
exception: 8
failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-0f09e272f670
Comment 27•12 years ago
|
||
For the record, here's the patch plus the changes I made based on the Try server results. Yati, are you interested in fixing up the remaining issue(s)? Or if you'd like me to, I'm happy to continue working on them.
Assignee | ||
Comment 28•12 years ago
|
||
Hi Matt, by remaining issues do you mean the rest of bug 812179? If you want, we can work on it collaboratively, or even split it up into chunks, as I can only work on the patches after work.
Assignee | ||
Comment 29•12 years ago
|
||
BTW, thanks for the patch :)
Comment 30•12 years ago
|
||
For now I mean the remaining issues in the /config patch -- for example the pythonpath.py exception above. Are you able to build and test this code locally, or would you like me to do the remaining work of testing it and landing it?
Assignee | ||
Comment 31•12 years ago
|
||
Oh, I'd like you to please try and land it, if possible. Thanks.
Comment 32•12 years ago
|
||
With a few more minor syntax fixes and some test changes copied from bug 845620, this is green on Try: https://tbpl.mozilla.org/?tree=Try&rev=eb2e61a8a318
So I pushed it to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f83edc05fa4
It will be merged to mozilla-central in the next day or so, and then this bug will be marked fixed. We should open follow-up bugs for fixing Python <2.6 hacks in other directories. Thank you very much for the patch!
Assignee | ||
Comment 33•12 years ago
|
||
Thanks a ton! Yes, once the bug is marked fixed, we should break this up. As of now, I have the patches for build/ and browser/ dirs as well. Thanks again :)
Updated•12 years ago
|
Summary: Remove hacks for Python < 2.6 → Remove hacks for Python < 2.6 in config/
Comment 34•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 35•12 years ago
|
||
Looks like we broke SeaMonkey builds:
http://tinderbox.mozilla.org/showlog.cgi?tree=SeaMonkey&errorparser=unittest&logfile=1362210290.1362210808.23382.gz&buildtime=1362210290&buildname=Linux%20comm-central-trunk%20build&fulltext=1
starting from "set props: buildid".
Assignee | ||
Comment 36•12 years ago
|
||
I'll fix this and push a path in a while.
Comment 37•12 years ago
|
||
(In reply to Yati Sagade from comment #36)
> I'll fix this and push a path in a while.
Thanks.. I've got a patch in the wings that will hopefully fix the python issue, which I'm thinking it is. If your patch doesn't break FF then it's most likely our builders needs a path adjustment.
Assignee | ||
Comment 38•12 years ago
|
||
Oh, I'm sorry, I meant I'll push a patch, not a path. Anyway, it is just a matter of adding `from __future__ import print_function` to the top of the file(s) affected by this very problem.
Assignee | ||
Comment 39•12 years ago
|
||
Well, It looks like config/printconfigsetting.py is in good shape(I made sure it runs in a crude way on my machine). Any idea what might have gone wrong?
Comment 40•12 years ago
|
||
It looks like maybe one of the automation scripts is calling printconfigsettings.py (using execfile or something) from another script that does not import print_function. Those scripts live outside mozilla-central, so until we can get a fix for them deployed, we might want to just revert the change to printconfigsettings.py.
Comment 41•12 years ago
|
||
Looking closer, it looks like the SeaMonkey build is just running "python printcontfigsetting.py ..." as a shell command, so I have no idea why this SyntaxError could be happening.
Comment 42•12 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #41)
> Looking closer, it looks like the SeaMonkey build is just running "python
> printcontfigsetting.py ..." as a shell command, so I have no idea why this
> SyntaxError could be happening.
Sooo:
The calls in the build work just fine:
http://mxr.mozilla.org/comm-central/source/mozilla/configure.in#3907
Because it uses the python as defined by configure[.in]
However the calls from buildbot fail because |python| is not py2.7 on our machines, (we're not using the mock-based builders yet)
Since the change for this file is py-3.0 compat I request that we backout the change to this *1* file. Since the changes needed to SeaMonkey buildbot would break us again when we finally switch to mock (which is in our wings as well, for shortly after pymake)
Assignee | ||
Comment 43•12 years ago
|
||
(In reply to Justin Wood (:Callek) from comment #42)
> Because it uses the python as defined by configure[.in]
>
> However the calls from buildbot fail because |python| is not py2.7 on our
> machines, (we're not using the mock-based builders yet)
Aah, so that's the reason - just wanted to point out that Python 2.6 will work fine, but I guess even that is not available there.
> Since the change for this file is py-3.0 compat I request that we backout
> the change to this *1* file. Since the changes needed to SeaMonkey buildbot
> would break us again when we finally switch to mock (which is in our wings
> as well, for shortly after pymake)
Matt, I'm new to hg and have no idea how to revert the changes to this one file(The way I would think of is to set tip to some point before your merge, copy the file out, reset back the tip, and copy the previous version over). Do let me know if there's a cleaner way out.
Comment 44•12 years ago
|
||
(In reply to Yati Sagade from comment #43)
> (In reply to Justin Wood (:Callek) from comment #42)
> > Since the change for this file is py-3.0 compat I request that we backout
> > the change to this *1* file. Since the changes needed to SeaMonkey buildbot
> > would break us again when we finally switch to mock (which is in our wings
> > as well, for shortly after pymake)
>
> Matt, I'm new to hg and have no idea how to revert the changes to this one
> file(The way I would think of is to set tip to some point before your merge,
> copy the file out, reset back the tip, and copy the previous version over).
> Do let me know if there's a cleaner way out.
I can do this change as well, I'm just awaiting a build-peer to come online and give me an rs+ to land said fix. SeaMonkey bustage alone does not allow a "broken" backout. So rules make me need a review/acceptance.
Comment 45•12 years ago
|
||
Callek: rs+ with the condition you address bug 842445. Just kidding. It's an unqualified rs+ :)
Comment 46•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #45)
> Callek: rs+ with the condition you address bug 842445. Just kidding. It's an
> unqualified rs+ :)
Thanks!
Pushed directly to central:
https://hg.mozilla.org/mozilla-central/rev/edf9c64f821e
And merged central to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c3ffc28f4a
Comment 47•12 years ago
|
||
Backed out the expandlibs part because it broke bug 603370.
https://hg.mozilla.org/integration/mozilla-inbound/rev/92824d900e25
Please file a new bug if you want to redo it for expandlibs. (thus not reopening)
Target Milestone: mozilla22 → ---
Comment 48•12 years ago
|
||
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/92824d900e25
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•