1264325 - Talos on windows seems broken with python 2.7.11

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Avi Halachmi (:avih)]

Using Mozilla-build 2.2 (has python 2.7.11):

- cd mozilla-central/testing/talos
- python INSTALL.py
- source Scripts/activate
- talos --help

Gives the following error:

Traceback (most recent call last):
  File "t:\dev\moz\src\m-c\testing\talos\Scripts\talos-script.py", line 9, in <module>
    load_entry_point('talos', 'console_scripts', 'talos')()
  File "d:\mozilla-build\python\lib\site-packages\pkg_resources\__init__.py", line 549, in load_entry_point
    return get_distribution(dist).load_entry_point(group, name)
  File "d:\mozilla-build\python\lib\site-packages\pkg_resources\__init__.py", line 2547, in load_entry_point
    return ep.load()
  File "d:\mozilla-build\python\lib\site-packages\pkg_resources\__init__.py", line 2207, in load
    return self.resolve()
  File "d:\mozilla-build\python\lib\site-packages\pkg_resources\__init__.py", line 2213, in resolve
    module = __import__(self.module_name, fromlist=['__name__'], level=0)
  File "t:\dev\moz\src\m-c\testing\talos\talos\run_tests.py", line 13, in <module>
    import utils
  File "t:\dev\moz\src\m-c\testing\talos\talos\utils.py", line 42, in <module>
    PLATFORM_TYPE = _get_platform()
  File "t:\dev\moz\src\m-c\testing\talos\talos\utils.py", line 37, in _get_platform
    raise TalosError('unsupported windows version')
NameError: global name 'TalosError' is not defined


If I do the same from Mozilla-Build 2.1 (has 2.7.10), then it does run correctly, and 'talos --help' now also works from MB2.2 (since the python binaries at the virtualenv are from MB2.1).

Ryan created a test package based on MB2.2 - but with python 2.17.10, and it does seems to work correctly.

I also tried Ubuntu 16.04 beta2 (which has python "2.7.11+") and I _think_ there's no issue there.

So it seems the problem is between talos and python 2.7.11 (as provided by Mozilla-Build) on Windows.

Comment 1

[Justin Wood (:Callek)]

Joel, we'll be upgrading "soon" to py2.7.11 on test slaves (we're doing builders first)...

Any insight into if this is an actual problem or how difficult/long it would be to fix (with backports of the fix to all trees if necessary)

Comment 2

[Avi Halachmi (:avih)]

I think platform.system() returns 6.3.9600 (Windows 8.1 64), but talos/talos/utils.py only searches 5.1, 6.1, 6.2, but doesn't find any of those so throws an "unsupported windows version" exception.

Not sure if win8.1 should be treated differently than win8, but locally, the following patch seems to make it work (both talos --help and also actually running a test):

diff --git a/testing/talos/talos/utils.py b/testing/talos/talos/utils.py
index ac5cad7..af76c01 100755
--- a/testing/talos/talos/utils.py
+++ b/testing/talos/talos/utils.py
@@ -33,6 +33,8 @@ def _get_platform():
             return 'w7_'
         elif '6.2' in platform.version():  # w8
             return 'w8_'
+        elif '6.3' in platform.version():  # w8
+            return 'w8_'
         else:
             raise TalosError('unsupported windows version')
     elif platform.system() == "Darwin":

Comment 3

[Avi Halachmi (:avih)]

(also, no windows 10 support?)

Comment 4

[Avi Halachmi (:avih)]

(sorry for the spam).

With python 2.7.10, platform.version() returns:
- Windows 8.1 (64): 6.2.9200
- Windows 10  (32): 6.2.9200 (yes, same as win8.1)

So it's obvious why the current code works.


With Python 2.7.11, platform.version() returns:
- Windows 8.1 (64): 6.3.9600
- Windows 10  (32): 10.0.10586

And so it fails.

Comment 5

[Justin Wood (:Callek)]

(In reply to Avi Halachmi (:avih) from comment #4)
> (sorry for the spam).
> 
> With python 2.7.10, platform.version() returns:
> - Windows 8.1 (64): 6.2.9200
> - Windows 10  (32): 6.2.9200 (yes, same as win8.1)
> 
> So it's obvious why the current code works.
> 
> 
> With Python 2.7.11, platform.version() returns:
> - Windows 8.1 (64): 6.3.9600
> - Windows 10  (32): 10.0.10586
> 
> And so it fails.

Apparantly thats from the py2.7.11 rel notes:

- Issue #19143: platform module now reads Windows version from kernel32.dll to
  avoid compatibility shims.

Which I found in some similar bugs when I was doing win10 work, like https://hg.mozilla.org/mozilla-central/rev/1989889145ce

Comment 6

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #5)
> Apparantly thats from the py2.7.11 rel notes:
> 
> - Issue #19143: platform module now reads Windows version from kernel32.dll
> to
>   avoid compatibility shims.

To be clear, upstream this is:

https://bugs.python.org/issue19143

with the 2.7 branch upstream cset of https://hg.python.org/cpython/rev/d8453733cc0c/ which used a different approach than my rev for w10 work.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

thanks avih for chiming in here.  To fix this we would need to patch talos here:
* https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/utils.py#27
** and verify other testing code: https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting+platform.system+Win&redirect=false&case=true

we only run talos on beta/aurora/trunk, so once this is landed successfully on trunk, then uplift to aurora/beta should be very simple.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Callek, I have a few other higher priority items this month to work on, I won't be much help here- feel free to submit a patch and do try pushes/landings.  I can review the patch if needed.

Comment 9

[Avi Halachmi (:avih)]

Any plans here?

As long as it's not fixed, it's impossible to run talos locally on windows 8.1 or windows 10 with latest mozilla-build - That's almost for two months now.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

we will use this small hack to fix this for now- the next patch that I review for :avih, will just include this- so we will duplicate this bug when appropriate.

Comment 12

[Avi Halachmi (:avih)]

This is unrelated to nor a duplicate of bug 1274992.

I'll post a hack patch for review which allows it to run locally and which generally is an incorrect solution. Joel does not know what the correct solution is and neither do I - so at least let people run it locally meanwhile.

Comment 13

[Avi Halachmi (:avih)]

Attachment: bug1264325.patch - don't break when running on windows 8.1/10

Keep in mind that this is probably an incorrect solution, but Joel requested me to land it anyway.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8755484 [details] [diff] [review]
bug1264325.patch - don't break when running on windows 8.1/10

Review of attachment 8755484 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, this is good to see!

Comment 15

[Avi Halachmi (:avih)]

Attachment: bug1264325.python-2.7.11.mq.patch

Carrying r+  just changing from git patch to mercurial patch.

Comment 16

[Avi Halachmi (:avih)]

Attachment: bug1264325.v3.python-2.7.11.mq.patch

Carrying r+, forgot the commit message at the earlier patch.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6afb408165ad

Comment 18

[Carsten Book [:Tomcat]]

backed this out in  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ae4b9e58a98a seems this or the other talos change caused https://treeherder.mozilla.org/logviewer.html#?job_id=28541751&repo=mozilla-inbound

Comment 19

[Avi Halachmi (:avih)]

Meh, really? we can't land lines longer than 79 chars? Is this only with talos? only with python? or with any new code which lands at m-c?

I applaud the effort, though on this case it seems a bit excessive considering the current code base, but that's off topic. 

How can I check this flake8 thingy next time locally - to see the same issues as if I tried to land it?

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

run flake8 locally on files changed:
cd testing/talos/talos
pip install flake8
flake8 test.py

that should sort it out- try will catch this as well.

Comment 21

[Avi Halachmi (:avih)]

Will it show warnings only on the lines which got modified? or on the entire file?

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

on all lines- but warnings should be non-existent on lines which you did not modify.

Comment 23

[Avi Halachmi (:avih)]

So there no way to check locally "is this patch going to trigger a flake8 error" without running it manually on all the files which the patch touches and then verify manually if the warnings relate to lines which the patch touches?

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

cd testing/talos
flake8 talos

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

According to https://groups.google.com/d/msg/mozilla.tools/LOibynYnkwU/DVCQJyYsCgAJ you can use "mach lint" and "mach lint <path(s)>" to run flake8.

Comment 26

[Avi Halachmi (:avih)]

Attachment: bug1264325.v4.python-2.7.11.mq.patch

Carrying r+, updated the comment to not exceed 79 chars, Joel said on IRC that the patch is OK.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6cc8c57dff9

Comment 27

[Avi Halachmi (:avih)]

Try push looks good.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fce92905c8

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6fce92905c8