1387265 - Add macOS tp6 runs for non-Stylo

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Since many Stylo developers are on macOS, it would help with correlating performance changes to tp6 results if we also had tp6 on macOS.

Let's add parallel Stylo (STYLO_FORCE_ENABLED=1) and regular tp6 runs on macOS.

Comment 1

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ada62334720f6c836953b56175496b103d5d4a

Comment 2

[Robert Wood [:rwood]]

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

Comment 3

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c14553632b5dc3dc8a27312527e7abed856bc0f

Comment 4

[Robert Wood [:rwood]]

So turns out mitmproxy has special pre-built binaries for macosx, that don't require python 3.x. Download these binaries when on osx, set appropriate paths, and install the mitmproxy CA certificate into the osx Firefox location.

Try run is green, tp6 on osx. Hacked talos.json to run tp6 in place of chromez (since the taskcluster/bb configs aren't in place for tp6 on osx):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b401d7a81f4fb0fb57d9cc74f20496ad2359d323&selectedJob=123349401

Comment 5

[Robert Wood [:rwood]]

Attachment: Bug 1387265 - Expand talos tp6 to macosx;

Review commit: https://reviewboard.mozilla.org/r/169118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169118/

Comment 6

[Robert Wood [:rwood]]

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169118/diff/1-2/

Comment 7

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

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

https://reviewboard.mozilla.org/r/169118/#review174466

pretty close!

As a note, osx is 100% on taskcluster, so there is no need for buildbot changes (and I ensured that tp6* is available on all platforms in buildbot when I made my last set of changes)

::: testing/mozharness/mozharness/mozilla/testing/talos.py:443
(Diff revision 1)
> -        self.info("Installing mitmproxy")
> +            self.info("Installing mitmproxy")
> -        self.py3_install_modules(modules=['mitmproxy'])
> +            self.py3_install_modules(modules=['mitmproxy'])
> -        self.mitmdump = os.path.join(self.py3_path_to_executables(), 'mitmdump')
> +            self.mitmdump = os.path.join(self.py3_path_to_executables(), 'mitmdump')
> +        else:
> +            # on macosx we use a prebuilt mitmproxy release binary
> +            mitmproxy_bin_url = 'https://github.com/mitmproxy/mitmproxy/releases/download/v2.0.2/mitmproxy-2.0.2-osx.tar.gz'

while I am not a fan of downloading directly from github and from the talos script directly, I think the alternative is to upload this to tooltool, then instrument mozharness to download the bits.

Can you file a followup bug to address that?

::: testing/mozharness/mozharness/mozilla/testing/talos.py:444
(Diff revision 1)
> -        self.py3_install_modules(modules=['mitmproxy'])
> +            self.py3_install_modules(modules=['mitmproxy'])
> -        self.mitmdump = os.path.join(self.py3_path_to_executables(), 'mitmdump')
> +            self.mitmdump = os.path.join(self.py3_path_to_executables(), 'mitmdump')
> +        else:
> +            # on macosx we use a prebuilt mitmproxy release binary
> +            mitmproxy_bin_url = 'https://github.com/mitmproxy/mitmproxy/releases/download/v2.0.2/mitmproxy-2.0.2-osx.tar.gz'
> +            mitmproxy_bin_loc = os.path.join(self.talos_path, 'talos', 'mitmproxy')

I would prefer mitmproxy_path

::: testing/mozharness/mozharness/mozilla/testing/talos.py:445
(Diff revision 1)
> -        self.mitmdump = os.path.join(self.py3_path_to_executables(), 'mitmdump')
> +            self.mitmdump = os.path.join(self.py3_path_to_executables(), 'mitmdump')
> +        else:
> +            # on macosx we use a prebuilt mitmproxy release binary
> +            mitmproxy_bin_url = 'https://github.com/mitmproxy/mitmproxy/releases/download/v2.0.2/mitmproxy-2.0.2-osx.tar.gz'
> +            mitmproxy_bin_loc = os.path.join(self.talos_path, 'talos', 'mitmproxy')
> +            #mitmproxy_bin_zip = os.path.join(mitmproxy_bin_loc, 'mitmproxy-2.0.2-osx.tar.gz')

can you remove the commented out line of code here?

::: testing/talos/talos/mitmproxy/mitmproxy.py:88
(Diff revision 1)
>      # browser_path is exe, we want install dir
>      browser_install = os.path.dirname(browser_path)
> +    # on macosx we need to remove the last folders 'Content/MacOS'
> +    if mozinfo.os == 'mac':
> +        browser_install, x = os.path.split(browser_install)
> +        browser_install, x = os.path.split(browser_install)

why is this line duplicated?

Comment 8

[Robert Wood [:rwood]]

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

https://reviewboard.mozilla.org/r/169118/#review174466

> while I am not a fan of downloading directly from github and from the talos script directly, I think the alternative is to upload this to tooltool, then instrument mozharness to download the bits.
> 
> Can you file a followup bug to address that?

Yeah good point, filed Bug 1390908 - Make mitmproxy binary available on tooltool and update talos tp6 accordingly

> why is this line duplicated?

Each line removes the trailing part of the path, i.e. '/Content' and then 'MacOS', I'll replace it with one line intead

Comment 9

[Robert Wood [:rwood]]

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169118/diff/2-3/

Comment 10

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

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

https://reviewboard.mozilla.org/r/169118/#review174548

can you add code to schedule this in tests.yml and test-sets.yml

Comment 11

[Robert Wood [:rwood]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8582c752979082ffda74acf31ac4125f6dff3911

Comment 12

[Robert Wood [:rwood]]

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169118/diff/3-4/

Comment 13

[Robert Wood [:rwood]]

(In reply to Robert Wood [:rwood] from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8582c752979082ffda74acf31ac4125f6dff3911

Update: On macosx tp6 is green but for some reason tp6s and tp6st fail, will look into this tomorrow

Comment 14

[Robert Wood [:rwood]]

On macosx starting up the browser via talos with STYLO_FORCE_ENABLED=1 fails with:

11:26:58     INFO -    File "/Users/rwood/mozilla-central/testing/talos/talos/run_tests.py", line 280, in run_tests
11:26:58     INFO -      talos_results.add(mytest.runTest(browser_config, test))
11:26:58     INFO -    File "/Users/rwood/mozilla-central/testing/talos/talos/ttest.py", line 61, in runTest
11:26:58     INFO -      return self._runTest(browser_config, test_config, setup)
11:26:58     INFO -    File "/Users/rwood/mozilla-central/testing/talos/talos/ttest.py", line 203, in _runTest
11:26:58     INFO -      if counter_management else None),
11:26:58     INFO -    File "/Users/rwood/mozilla-central/testing/talos/talos/talos_process.py", line 115, in run_browser
11:26:58     INFO -      proc.run()
11:26:58     INFO -    File "/Users/rwood/mozilla-central/obj-x86_64-apple-darwin16.7.0/testing/talos-venv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 751, in run
11:26:58     INFO -      self.proc = self.Process([self.cmd] + self.args, **args)
11:26:58     INFO -    File "/Users/rwood/mozilla-central/obj-x86_64-apple-darwin16.7.0/testing/talos-venv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 114, in __init__
11:26:58     INFO -      universal_newlines, startupinfo, creationflags)
11:26:58     INFO -    File "/usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__
11:26:58     INFO -      errread, errwrite)
11:26:58     INFO -    File "/usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child
11:26:58     INFO -      raise child_exception
11:26:58    ERROR -  TypeError: execve() arg 3 contains a non-string value

:jmaher, :jrans is on PTO - do you have any idea why this would fail? Starting up via talos without STYLO_FORCE_ENABLED works fine hence why 'tp6' is green but 'tp6-stylo' and 'tp6-stylo-threads' fails.

Comment 15

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

I am not sure- if we could get the actual command line (process args) that might help to narrow down the problem.  could we enable this tp6 for osx and ignore tp6s/tp6st for the short term?

Comment 16

[Robert Wood [:rwood]]

Comment on attachment 8897825 [details]
Bug 1387265 - Expand talos tp6 to macosx;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169118/diff/4-5/

Comment 17

[Robert Wood [:rwood]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #15)
> I am not sure- if we could get the actual command line (process args) that
> might help to narrow down the problem.  could we enable this tp6 for osx and
> ignore tp6s/tp6st for the short term?

Ok, I'll land tp6 on osx but without stylo/stylo-threads for now.

Comment 18

[Robert Wood [:rwood]]

Try for updated patch, macosx tp6 only:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=877eb66974aa39e6185039341a0af2e9809077a3

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2619edc827ad
Expand talos tp6 to macosx; r=jmaher

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2619edc827ad

Comment 21

[Chris Peterson [:cpeterson]]

(In reply to Robert Wood [:rwood] from comment #14)
> On macosx starting up the browser via talos with STYLO_FORCE_ENABLED=1 fails
> with:
...
> 11:26:58    ERROR -  TypeError: execve() arg 3 contains a non-string value
> 
> :jmaher, :jrans is on PTO - do you have any idea why this would fail?
> Starting up via talos without STYLO_FORCE_ENABLED works fine hence why 'tp6'
> is green but 'tp6-stylo' and 'tp6-stylo-threads' fails.

How are you setting STYLO_FORCE_ENABLED=1? It needs to be an environment variable defined before the test launches Firefox. It's not a command line argument to Firefox. I don't know how Python process spawning APIs support environment variables to be defined for the new process' environment or if the new process inherits the test runner's environment variables.

Comment 22

[Robert Wood [:rwood]]

(In reply to Chris Peterson [:cpeterson] from comment #21)

> 
> How are you setting STYLO_FORCE_ENABLED=1? It needs to be an environment
> variable defined before the test launches Firefox. It's not a command line
> argument to Firefox. I don't know how Python process spawning APIs support
> environment variables to be defined for the new process' environment or if
> the new process inherits the test runner's environment variables.

Yes, as an environment variable, the same way/code that talos is currently using to enable it on Windows:

http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/testing/mozharness/mozharness/mozilla/testing/talos.py#593

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I'm exploring the macOS env var issue in a try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59035bf6b40e92617332545de67112e91de5a333

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I believe the issue is that, at least on macOS, we have to use string values for the env var, not ints.  Looks like Talos is the only harness that was using ints for this.

I filed bug 1393229 to fix that and turn on the Stylo version.

Comment 25

[Robert Wood [:rwood]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> I believe the issue is that, at least on macOS, we have to use string values
> for the env var, not ints.  Looks like Talos is the only harness that was
> using ints for this.
> 
> I filed bug 1393229 to fix that and turn on the Stylo version.

Thanks for your work on this :jryans!