Closed Bug 1428713 Opened 2 years ago Closed 9 months ago
[mozprocess] Add support for Python 3
59 bytes, text/x-review-board-request
47 bytes, text/x-phabricator-request
|Details | Review|
47 bytes, text/x-phabricator-request
|Details | Review|
47 bytes, text/x-phabricator-request
|Details | Review|
Without dropping support for legacy Python, we need to add support for Python 3 to mozprocess.
Hi Henrik, should I take this up?
To work on this bug you will need to install and configure Mercurial, which will enable you to download the Firefox source code. It will also be used to commit your changes locally and prepare a patch for review. See the installation guide at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/installing.html for help getting Mercurial on your system. Once installed, there are some extensions we recommend installing, details of these can be found here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/extensions.html To clone the Firefox source code we recommend using the unified repository. Details of this and how to create a bookmark for your work can be found here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/unifiedrepo.html As this bug relates to Python 3, you will need to have this installed on your system. Our continuous integration is currently using Python 3.5, so for best results we recommend using the same version locally. There are a number of ways to install Python, and they vary depending on your environment. We suggest reading over the guides at http://docs.python-guide.org/en/latest/starting/installation/#python-3-installation-guides to find the best method for you. Note that we also need to maintain support for Python 2, so you'll also need to have Python 2.7 installed. Whilst we're moving towards adding support for Python 3, we've disabled any tests that fail against this version. This means that in order to work on this bug you will need to enable the tests. This can be done by removing "skip-if = python == 3" from the manifest file in `testing/mozbase/mozprocess/tests/manifest.ini` To run the tests against Python 3, execute the following command: ``` mach python-test --python=3.5 testing/mozbase/mozprocess ``` Work through the test failures, and update the tests and source code for the package to simultaneously support Python 2.7 and Python 3.5. You can always check that your changes have not inadvertently broken support for Python 2 by using `--python=2.7` on the command line. If you're new to Python 3, the guide at https://docs.python.org/3/howto/pyporting.html may help you to get started with understanding the differences. To assist with supporting both Python 2 and 3, we have vendored the "six" package. You can read more about this and how to use it at https://pythonhosted.org/six/ Note that this package depends on others that may not yet support Python 3. If these dependencies are preventing tests from passing, then we can block this bug on getting support in those packages. If there are circular dependencies then we may need to coordinate landing a sequence of patches between bugs, Once the package supports Python 3 and the tests pass, we will also need to prepare for a new release. Bump the version number in `testing/mozbase/mozprocess/setup.py` to "1.0.0", and update the classifiers so they reflect the versions of Python that we now support. Finally, to prepare a universal distribution, create a `testing/mozbase/mozprocess/setup.cfg` file with the following contents: ``` [bdist_wheel] universal=1 ``` When your patch(es) are ready, you will need to push them for review. We are use MozReview for this, and instructions for how to prepare your machine and how to structure your commit messages can be found in our guide: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html There may be some review issues to address, but once your contribution has been approved and subsequently landed, we will release the new version of the package!
Hello. Can I work on this? (I'm not sure of the correct processes for assigning bugs. I've installed the environment, reproduced issue and working on fix. But it can be slow, because I'm new to contributing to opensource and to mercurial) I don't have any other questions for now - just planning to spend some time on reading the docs.
Hi Pavel! I have assigned this bug to you. I'm not very familiar with mozprocess but I will help where I can, and bring in others when I can't help you myself. Let me know how you get on.
Assignee: nobody → slepushkin
Status: NEW → ASSIGNED
Hi, Dave I've pushed my commit, here's the log: ----------------- $ hg push review pushing to https://reviewboard-hg.mozilla.org/autoreview searching for appropriate review repository redirecting push to https://reviewboard-hg.mozilla.org/gecko searching for changes no changes found submitting 1 changesets for review changeset: 478942:d69fee28a2a9 summary: Bug 1428713 - [mozprocess] Add support for Python 3 review: https://reviewboard.mozilla.org/r/261348 (draft) review id: bz://1428713/PSlepushkin review url: https://reviewboard.mozilla.org/r/261346 (draft) (review requests lack reviewers; visit review url to assign reviewers) publish these review requests now (Yn)? Y (published review request 261346) ----------------- I've tried to follow docs, but it's my 1st time, so there's a possibility of errors from my side. Please let me know if I need to correct anything.
Comment on attachment 8997662 [details] Bug 1428713 - [mozprocess] Add support for Python 3 https://reviewboard.mozilla.org/r/261346/#review269336 Thanks Pavel, this looks great, and the tests are all passing for me. I've triggered a try run to see if these changes cause any regressions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa0139c9bc5f48998e29df0c812923017f7a8a41. ::: testing/mozbase/mozprocess/setup.py:21 (Diff revision 1) > 'Intended Audience :: Developers', > 'License :: OSI Approved :: Mozilla Public License 2.0 (MPL 2.0)', > 'Natural Language :: English', > 'Operating System :: OS Independent', > - 'Programming Language :: Python', > + 'Programming Language :: Python :: 2.7', > + 'Programming Language :: Python :: 3' Could you change this to "3.5" to make it clear we're not officially claiming support for other versions.
Comment on attachment 8997662 [details] Bug 1428713 - [mozprocess] Add support for Python 3 https://reviewboard.mozilla.org/r/261348/#review269340 ::: testing/mozbase/mozprocess/tests/test_output.py:5 (Diff revision 1) > #!/usr/bin/env python > > from __future__ import absolute_import > > import io Please remove this import as it's no longer used.
Hi,Dave Pushed another commit.
(In reply to Pavel Slepushkin from comment #10) > Hi,Dave > Pushed another commit. The new commit looks great, but for some reason it's been submitted as a new review. You should be able to amend your original commit and push again to update the review. With separate reviews it's not possible to land everything in one go, and the unused import would cause a failure and a backout.
Hi, Dave. I'm kinda lost here. On my host: I've deleted my last commit and amended my first (reviewed) commit: >hg strip --keep --rev -1 >hg commit --amend I've checked diff and OK with it (it's correct) But now I'm unable to push: >hg push >pushing to https://hg.mozilla.org/mozilla-unified >searching for changes >remote has heads on branch 'default' that are not known locally: 05626aa5df00 0d04a3f89940 2177be7a65c0 723b6cad1824 and 1 others >abort: push creates new remote head 6c01b2accda1 with bookmark 'myfeature'! >(pull and merge or see 'hg help push' for details about pushing new heads) Not sure how to proceed. I can pull my latest commit, but that won't solve the problem with submitting to new review. Don't want to push force without asking first. Can you please advice something? (My understanding is - my commit https://bugzilla.mozilla.org/attachment.cgi?id=8999707 should be discarded, https://bugzilla.mozilla.org/attachment.cgi?id=8997662 will become a head, after that and I'll be able to push amended head , but I can be wrong)
You appear to be attempting to push to mozilla-unified rather than review. Can you try the following: > hg push review
Thanks Now I've got this: >hg push review >pushing to https://reviewboard-hg.mozilla.org/autoreview >searching for appropriate review repository >redirecting push to https://reviewboard-hg.mozilla.org/gecko >searching for changes >remote: adding changesets >remote: adding manifests >remote: adding file changes >remote: added 1 changesets with 2 changes to 6 files (+1 heads) >remote: recorded push in pushlog >submitting 1 changesets for review > >changeset: 478943:6c01b2accda1 >summary: Bug 1428713 - [mozprocess] Add support for Python 3 >review: https://reviewboard.mozilla.org/r/261348 (draft) > >review id: bz://1428713/PSlepushkin >review url: https://reviewboard.mozilla.org/r/261346 (draft) > >publish these review requests now (Yn)? Y >error publishing review request 261346: Error publishing: Bugzilla error: Dave Hunt [:davehunt] ⌚️UTC+1 <email@example.com> >is not currently accepting 'review' requests. (HTTP 500, API Error 225) >(review requests not published; visit review url to attempt publishing there) And I can see, that I've published only a draft: https://reviewboard.mozilla.org/r/261346/diff/1-2/
Oops, that was my fault for not resetting my Bugzilla review flag after returning from vacation. You should be able to publish your review now. Sorry about that.
Looks like I've published it. https://reviewboard.mozilla.org/r/261346/
Comment on attachment 8997662 [details] Bug 1428713 - [mozprocess] Add support for Python 3 https://reviewboard.mozilla.org/r/261348/#review269518 Looks great, thanks!
Attachment #8997662 - Flags: review?(dave.hunt) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/d08617053829 [mozprocess] Add support for Python 3 r=davehunt
Backed out changeset d08617053829 (Bug 1428713) for awsy-base failures. Backout: https://hg.mozilla.org/integration/autoland/rev/c340d01c2552260a7d96ec5699594e6167f7bb43 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d08617053829ae9ea37b55c410768a1b07cde51d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=success&selectedJob=194091899 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194091899&repo=autoland&lineNumber=1018
It looks like mozrunner has a dependency on mozprocess that specifies the version must be greater that or equal to 0.23 but lower than 1.0. Could you modify this to mozprocess>=0.23,<2 as we haven't introduced any changes other than adding support for Python 3. The line that needs to be updated is https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/testing/mozbase/mozrunner/setup.py#19 We also need to update the following to 1.0.0: https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/testing/mozbase/mozrunner/setup.py#19 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_chrome.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_chrome_android.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_edge.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_firefox.txt#3 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_ie.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_opera.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_safari.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_sauce.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_servo.txt#1 https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/requirements_webkit.txt#1
I've been informed that you shouldn't need to worry about the files within testing/web-platform-tests as these will be taken care of when we release mozprocess v1.0.0. So it's just testing/mozbase/mozrunner/setup.py and testing/tps/setup.py that need updating.
Updated both files and pushed.
Comment on attachment 8997662 [details] Bug 1428713 - [mozprocess] Add support for Python 3 https://reviewboard.mozilla.org/r/261348/#review269534
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/aaabe6d53de2 [mozprocess] Add support for Python 3 r=davehunt
When attempting to release I noticed two issues. First, there's a missing comma in the classifiers in setup.py causing two to be concatenated. Second, we missed the following change from comment 0: To prepare a universal distribution, create a `testing/mozbase/mozprocess/setup.cfg` file with the following contents: ``` [bdist_wheel] universal=1 ``` I'll raise a new bug for the follow-up fixes.
Bug 1483850 is a regression from the landing of this patch and causes Mn tier-1 test jobs to fail: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=483ff2d2548af15a8fe5f813791c4817d2c4d091&selectedJob=194228118 Can we please get it backed-out? Thanks.
Backed out changeset aaabe6d53de2 (bug 1428713) for causing Bug 1483850 Backout: https://hg.mozilla.org/mozilla-central/rev/5879ce048c4abe85a5f5c5737c8de9301b4b0e49 Push with the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=73efbce701a9880e35a5139dc67757fa68ad892a Failure log file: https://treeherder.mozilla.org/logviewer.html#?job_id=194282756&repo=mozilla-central&lineNumber=8183
Target Milestone: mozilla63 → ---
Pavel: Let's take care of the regression and comment 28 here before landing this again.
Henrik has reopened bug 991866. I wonder if we need to bite the bullet and fix that before we can claim Python 3 compatibility. As that bug stopped being an issue for us 4 years ago, let's see if gps has any fresh thoughts on how we might proceed.
That's an old bug. But I think mozprocess should let the caller control how process output is decoded. There should be a way to get a handle on the raw byte stream. It should also be possible to interpret output as an encoding and to define the error handling strategy when that encoding is violated. Essentially, the `subprocess.run()` API from Python 3.6+ should be provided - namely the control that "encoding" and "errors" arguments provide.
Hi 1) I cannot reproduce ">UnicodeEncodeError: 'ascii' codec can't encode characters in position" and without test scenario - don't know how to possibly fix it. The idea >the `subprocess.run()` API from Python 3.6+ should be provided - namely the control that "encoding" and "errors" arguments provide will probably fix it, the problem is - subprocess API was changed in Python 3 and in this bug - we are trying to keep backward compatibility. I'm stuck here and open to ideas - how to reproduce it (maybe it is really simple - I just don't comfortable with environment ), what can possible fix it or what docs should I read. BTW - I've tryied to replace StringsIO to BytesIO, but that means changes for unit tests and potentially will break more things than previous commit. 2) Found another non-adressed problem on Windows: >1:18.93 > errread, errwrite) = args_tuple >1:18.93 E ValueError: too many values to unpack (expected 17) >1:18.93 >1:18.93 testing\mozbase\mozprocess\mozprocess\processhandler.py:248: ValueError the problem code is redefinition of execute_child: > if isWin: > # Redefine the execute child so that we can track process groups > def _execute_child(self, *args_tuple): > # workaround for bug 950894 > if sys.hexversion < 0x02070600: # prior to 2.7.6 > (args, executable, preexec_fn, close_fds, > cwd, env, universal_newlines, startupinfo, > creationflags, shell, > p2cread, p2cwrite, > c2pread, c2pwrite, > errread, errwrite) = args_tuple > to_close = set() > else: # 2.7.6 and later > (args, executable, preexec_fn, close_fds, > cwd, env, universal_newlines, startupinfo, > creationflags, shell, to_close, > p2cread, p2cwrite, > c2pread, c2pwrite, > errread, errwrite) = args_tuple And I don't know what to do here. subprocess._execute_child definition changed in python 2.7.6 (bug 950894 for mozprocess), then changed in 3.1. Easy fix will be - adding one more elif with additional args, but after next 'subprocess' change somebody will have to do that again. IMO, that is increase of technical debt and I don't like it. six package doesn't cover it Again, open to suggestions 3) As for comment 28 - sorry, missed it. will submit it with next commit. But for 1 and 2 - I need some advice.
(In reply to Dave Hunt [:davehunt] ⌚️UTC+1 from comment #33) > Henrik has reopened bug 991866. Oh, that might have be wrong. Maybe we should just close it again, and if necessary a new bug should be filed. (In reply to Pavel Slepushkin from comment #35) > 1) I cannot reproduce ">UnicodeEncodeError: 'ascii' codec can't encode > characters in position" > and without test scenario - don't know how to possibly fix it. Given that the failure was a permafailure for ccov test jobs, it might be worth running those Mn tests with a build like https://queue.taskcluster.net/v1/task/IDusNU1lTOibj4tjgniJDQ/artifacts/public/build/target.zip. I assume ccov Firefox builds might be special. Otherwise I can remember that in the past we also had some sporadic Unicode failures because the transmitted text was not Unicode. But that didn't happen for a long time now. So my proposal is, pick this build and check older Treeherder logs for which Marionette tests it mostly / always failed. That way you might find your reproducible testcase. > 2) Found another non-adressed problem on Windows: > And I don't know what to do here. > subprocess._execute_child definition changed in python 2.7.6 (bug 950894 for > mozprocess), then changed in 3.1. > Easy fix will be - adding one more elif with additional args, but after next > 'subprocess' change somebody will have to do that again. Simply add it for now, and make it the top-most block. Everything from 2.7 should be in the elif block afterward. At some point we might have to re-evaluate if it makes sense to override the subprocess module. It was mostly necessary for Python 2.7 and earlier due to missing support for the wide API-endpoints on Windows. Given that Python 3 has support for that, we can drop it once 2.7 reaches EOL. > IMO, that is increase of technical debt and I don't like it. Trust me, no-one of us. But as long as we have to override this method I don't think that we have another option.
I hit another issue, again need some help. To sum things up: Issue with converting string to ctypes.c_wchar_p,required for calling winapi ->CreateProcessW According to docs (https://docs.microsoft.com/ru-ru/windows/desktop/api/processthreadsapi/nf-processthreadsapi-createprocessw): >lpEnvironment >A pointer to the environment block for the new process. If this parameter is NULL, the new process uses the environment of the calling process. >An environment block consists of a null-terminated block of null-terminated strings. Each string is in the following form: >name=value\0 So, we need to have block of null terminated strings And the issue is (in very simplified form) that python2 works while python3 throws an error for the same code for a string with null-delimiters: >pavel@pavel-NUC6CAYH ~ >$ python2 -c "from ctypes import c_wchar_p ; print (c_wchar_p(u'1=1\x002=2'))" >c_wchar_p(u'1=1') >pavel@pavel-NUC6CAYH ~ >$ python3 -c "from ctypes import c_wchar_p ; print (c_wchar_p(u'1=1\x002=2'))" >Traceback (most recent call last): > File "<string>", line 1, in <module> >ValueError: embedded null character please let me know any ideas.
Which version of Python are you using? It looks like this might be related to https://bugs.python.org/issue32745.
Hi, thanks. Looks like it. Latest 3.5 is also affected: Python 3.5.4 (v3.5.4:3f56838, Aug 8 2017, 02:17:05) [MSC v.1900 64 bit (AMD64)] on win32 Next one - is not affected: Python 3.5.3rc1 (v3.5.3rc1:de530d7f21c0, Jan 2 2017, 06:41:25) [MSC v.1900 64 bit (AMD64)] on win32 IMO, we have to wait for python fix Meanwhile I'll check another bug assigned to me.
(In reply to Pavel Slepushkin from comment #39) > Hi, thanks. > Looks like it. > Latest 3.5 is also affected: > Python 3.5.4 (v3.5.4:3f56838, Aug 8 2017, 02:17:05) [MSC v.1900 64 bit > (AMD64)] on win32 > > Next one - is not affected: > Python 3.5.3rc1 (v3.5.3rc1:de530d7f21c0, Jan 2 2017, 06:41:25) [MSC v.1900 > 64 bit (AMD64)] on win32 This release pre-dates 3.5.4 > IMO, we have to wait for python fix Is there a workaround? If not, perhaps we could consider requiring Python 3.6 for mozprocess. This will mean we unblock packages that depend on mozprocess from supporting Python 3, but that we won't be able to use in in the Firefox build system. I don't think that will be switching to Python 3 for some time yet, so perhaps this isn't much concern for now. We'd just need to ensure that we don't break when using Python 2.7 on any platform that is currently supported.
Hello. Don't see any easy workaround. And considering this: >At some point we might have to re-evaluate if it makes sense to override the subprocess module. It was mostly necessary for Python 2.7 and earlier due to missing support for the wide API-endpoints on Windows. Given that Python 3 has support for that, we can drop it once 2.7 reaches EOL. don't want to do a complete rewrite of it.(Also not sure, that I am able do it - win (and winapi) issues are unknown area for me)
The cause of the backout (ordinal not in range) wasn't only happening on Windows (see https://treeherder.mozilla.org/logviewer.html#?job_id=194228118&repo=autoland&lineNumber=7060), so perhaps it's not necessary for us to tackle the Windows specific issues right now. Could you see if you can replicate the "ordinal not in range" issue, as fixing that may allow us to land your patch and at least move us closer to Python 3 support?
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/9c17fddb650f [mozprocess] Add support for Python 3 r=ahal
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/46ccb1988712 [mozprocess] Add support for Python 3 r=ahal
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.