Closed Bug 812179 Opened 12 years ago Closed 11 years ago

Remove hacks for Python < 2.6 in config/

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: yati.sagade)

References

Details

(Whiteboard: [mentor=ted][lang=python])

Attachments

(2 files, 2 obsolete files)

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.
We can also remove from __future__ import with_statement
Did we also have hacks for ignoring directories of a certain name in shutil.copytree ? This was added only in Python 2.6.
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
> (as opposed to "exception Exception, e")

Drive-by nit-pick - this should be "except Exception, e"
os.path.relpath, too.
Assignee: nobody → yati.sagade
Status: NEW → ASSIGNED
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.
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 :)
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 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+
> 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)
Attachment #693055 - Flags: review?(ted)
Gary, thanks. Done :)
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+
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 :)
Attachment #693055 - Attachment is obsolete: true
Attachment #695329 - Flags: review+
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.
Yati, was this ready to land? It looks like we dropped this on the floor.
Attachment #695329 - Attachment is obsolete: true
Attachment #719056 - Flags: review+
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?
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.)
No problem :)
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 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
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
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
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
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.
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.
BTW, thanks for the patch :)
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?
Oh, I'd like you to please try and land it, if possible. Thanks.
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!
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 :)
Summary: Remove hacks for Python < 2.6 → Remove hacks for Python < 2.6 in config/
https://hg.mozilla.org/mozilla-central/rev/8f83edc05fa4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
I'll fix this and push a path in a while.
(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.
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.
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?
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.
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.
(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)
(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.
(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.
Callek: rs+ with the condition you address bug 842445. Just kidding. It's an unqualified rs+ :)
(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
Blocks: 846999
Blocks: 847149
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 → ---
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.