Add macOS tp6 runs for non-Stylo

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jryans, Assigned: rwood)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [Stylo][PI:August])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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.
Whiteboard: [Stylo] → [Stylo][PI:August]
Assignee: nobody → rwood
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Component: CSS Parsing and Computation → Talos
Product: Core → Testing
Assignee

Comment 4

2 years ago
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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Depends on: 1390837
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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?
Attachment #8897825 - Flags: review?(jmaher) → review-
Assignee

Updated

2 years ago
Blocks: 1390908
Assignee

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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
Attachment #8897825 - Flags: review?(jmaher) → review+
Comment hidden (mozreview-request)
Assignee

Comment 13

2 years ago
(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
Assignee

Comment 14

2 years ago
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.
Flags: needinfo?(jmaher)
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?
Flags: needinfo?(jmaher)
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
(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 19

2 years ago
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2619edc827ad
Expand talos tp6 to macosx; r=jmaher

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2619edc827ad
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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.
Flags: needinfo?(rwood)
Assignee

Comment 22

2 years ago
(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
Flags: needinfo?(rwood)
Reporter

Updated

2 years ago
Blocks: 1393229
Reporter

Comment 24

2 years ago
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.
Reporter

Updated

2 years ago
Summary: Add macOS tp6 runs for Stylo and non-Stylo → Add macOS tp6 runs for non-Stylo
Assignee

Comment 25

2 years ago
(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!
You need to log in before you can comment on or make changes to this bug.