Closed Bug 1390968 Opened 7 years ago Closed 7 years ago

Test and fix python3 -mcompileall for in-tree Python

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Version:
52 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: dustin, Assigned: dustin)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Bug 1390968: add a py(3) job;
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1390968: python-3 compatibility for python/**/*.py
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
gps
: review+
Details
Bug 1390968: python-3 compatibility for taskcluster/**/*.py
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
gps
: review+
Details 
Per Alex Gaynor:

> A great first milestone is likely getting python3 -mcompileall python/ clean. It looks like there's a couple dozen syntax errors that I bet could be knocked out in an hour.
Comment on attachment 8901994 [details]
Bug 1390968: python-3 compatibility for python/**/*.py

https://reviewboard.mozilla.org/r/173410/#review178808

These changes should get build peer review. I'm happy to approve them. And I'm somewhat surprised the changes are this minimal!

::: python/mach/mach/config.py:22
(Diff revision 1)
>  from __future__ import absolute_import, unicode_literals
>  
>  import collections
>  import os
>  import sys
> +import six

Please add six to python/mach/setup.py.
Attachment #8901994 - Flags: review+
Comment on attachment 8901998 [details]
Bug 1390968: python-3 compatibility for taskcluster/**/*.py

https://reviewboard.mozilla.org/r/173412/#review178810

::: taskcluster/ci/source-test/python-tests.yml:167
(Diff revision 1)
>          max-run-time: 3600
>      run:
>          using: run-task
>          # this will evolve as we get closer and closer to python 3 compatibility. It may
>          # eventually be a mach command or shell script.
> -        command: python3 -mcompileall python/
> +        command: python3 -mcompileall python/ taskcluster/

This really wants to be a `mach lint` sub-command. I suppose the day will come.
Attachment #8901998 - Flags: review+
Comment on attachment 8901998 [details]
Bug 1390968: python-3 compatibility for taskcluster/**/*.py

https://reviewboard.mozilla.org/r/173412/#review178812

::: taskcluster/ci/source-test/python-tests.yml:170
(Diff revision 1)
>          # this will evolve as we get closer and closer to python 3 compatibility. It may
>          # eventually be a mach command or shell script.
> -        command: python3 -mcompileall python/
> +        command: python3 -mcompileall python/ taskcluster/
>      when:
>          files-changed:
>              - 'python/**/*.py'

Forgot to update files-changed. I'd just make it `**/*.py` and be done with it. That's the direction this check will go eventually anyway.
Comment on attachment 8901994 [details]
Bug 1390968: python-3 compatibility for python/**/*.py

https://reviewboard.mozilla.org/r/173410/#review178816

::: python/mozbuild/mozbuild/action/exe_7z_archive.py:27
(Diff revision 1)
>          subprocess.check_call(['7z', 'a', '-r', '-t7z', mozpath.join(tmpdir, 'app.7z'), '-mx', '-m0=BCJ2', '-m1=LZMA:d25', '-m2=LZMA:d19', '-m3=LZMA:d19', '-mb0:1', '-mb0s1:2', '-mb0s2:3'])
>  
>          with open(package, 'wb') as o:
>              for i in [mozpath.join(tmpdir, '7zSD.sfx'), tagfile, mozpath.join(tmpdir, 'app.7z')]:
>                  shutil.copyfileobj(open(i, 'rb'), o)
> -        os.chmod(package, 0755)
> +        os.chmod(package, stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH | stat.S_IXOTH)

please don't do this. stat.S_I* mixes are unreadable compared to the simplicity of octal literals.

Use 0o755 instead.
Comment on attachment 8901993 [details]
Bug 1390968: add a py(3) job;

https://reviewboard.mozilla.org/r/173408/#review178814

Seems reasonable as a first go.

I've been contemplating the idea of compiling Python from source as part of image building. Then, we could throw something into e.g. `/mozilla/python35` and have a consistent and well-defined Python 3 environment without the "bloat" of the system install (any random `python-*` system package will install global Python packages, which is not desirable). This could be done as a follow-up though. But given all the pain that managing Python has been, it is really tempting to say that all Python 3 in CI from day 0 will use a well-defined and isolated Python.
Attachment #8901993 - Flags: review?(gps) → review+
More generally (especially for mozjar), please keep octal literals, just use the form that works in both python 2 and 3.
Huh, what I read suggested 2.7 didn't recognize 0o123 syntax.  I should have verified that!  I agree about unreadability :)

Regarding mach lint, yes, I think we'll get there.  I just didn't want to mess with python2 invoking python3..
Comment on attachment 8901994 [details]
Bug 1390968: python-3 compatibility for python/**/*.py

https://reviewboard.mozilla.org/r/173410/#review179048

Please fix the octal vs. hex bug -- I also agree with :gps that the octal constants are mostly more readable than the `stat` constants.

Feel free to take-or-leave the "use more six" issue.

::: python/mach/mach/config.py:25
(Diff revision 1)
>  if sys.version_info[0] == 3:
>      from configparser import RawConfigParser, NoSectionError
>      str_type = str
>  else:
>      from ConfigParser import RawConfigParser, NoSectionError
>      str_type = basestring

If you're already adding `six`, does it make sense to change these to use it?

::: python/mozbuild/mozpack/mozjar.py:388
(Diff revision 1)
>              # higher bits are the stat.st_mode value. On MSDOS, the lower bits
>              # are the FAT attributes.
>              xattr = entry['external_attr']
>              # Skip directories
>              if (host == 0 and xattr & 0x10) or (host == 3 and
> -                                                xattr & (040000 << 16)):
> +                                                xattr & (0x4000 << 16)):

This is wrong, it should be `0o40000`.
Attachment #8901994 - Flags: review?(agaynor) → review-
Comment on attachment 8901998 [details]
Bug 1390968: python-3 compatibility for taskcluster/**/*.py

https://reviewboard.mozilla.org/r/173412/#review179050
Attachment #8901998 - Flags: review?(agaynor) → review+
Comment on attachment 8901994 [details]
Bug 1390968: python-3 compatibility for python/**/*.py

https://reviewboard.mozilla.org/r/173410/#review179060
Attachment #8901994 - Flags: review?(agaynor) → review+
Just waiting for that last try push and I'll land this.
For reasons I can't explain, that ran some Windows builds in Buildbot (not via BBB).  Maybe "win64" was the wrong try syntax?
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6d1757da618
add a py(3) job; r=gps
https://hg.mozilla.org/integration/autoland/rev/dfb510e198b8
python-3 compatibility for python/**/*.py; r=Alex_Gaynor,gps
https://hg.mozilla.org/integration/autoland/rev/70194da47f25
python-3 compatibility for taskcluster/**/*.py; r=Alex_Gaynor,gps
See Also: → 1391019
(In reply to Gregory Szorc [:gps] from comment #5)
> This really wants to be a `mach lint` sub-command. I suppose the day will
> come.

This is bug 1391019.
Depends on: 1401687
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: