Closed Bug 1264325 Opened 4 years ago Closed 3 years ago

Talos on windows seems broken with python 2.7.11

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: avih, Unassigned)

Details

(Whiteboard: [talos_wishlist])

Attachments

(1 file, 3 obsolete files)

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.
Whiteboard: [talos_wishlist]
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)
Blocks: 1266240
Flags: needinfo?(jmaher)
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":
(also, no windows 10 support?)
(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.
(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
(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.
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.
Flags: needinfo?(jmaher)
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.
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.
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1274992
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.
No longer blocks: 1266240
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Keep in mind that this is probably an incorrect solution, but Joel requested me to land it anyway.
Attachment #8755484 - Flags: review?(jmaher)
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!
Attachment #8755484 - Flags: review?(jmaher) → review+
Carrying r+  just changing from git patch to mercurial patch.
Attachment #8755484 - Attachment is obsolete: true
Attachment #8755545 - Flags: review+
Keywords: checkin-needed
Carrying r+, forgot the commit message at the earlier patch.
Attachment #8755545 - Attachment is obsolete: true
Attachment #8755548 - Flags: review+
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?
Flags: needinfo?(avihpit) → needinfo?(cbook)
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.
Flags: needinfo?(cbook)
Will it show warnings only on the lines which got modified? or on the entire file?
on all lines- but warnings should be non-existent on lines which you did not modify.
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?
cd testing/talos
flake8 talos
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.
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
Attachment #8755548 - Attachment is obsolete: true
Attachment #8755959 - Flags: review+
Try push looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6fce92905c8
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.