Closed Bug 1393234 Opened 9 years ago Closed 9 years ago

Add --disable-stylo to mozharness, test harnesses, etc.

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Whiteboard: [Stylo])

Attachments

(5 files)

Bug 1393234 - Check STYLO_FORCE_DISABLED in reftests.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1393234 - Add --disable-stylo for desktop tests.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1393234 - Add --disable-stylo for WPT.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1393234 - Add --disable-stylo for Talos.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1393234 - Add --disable-stylo for AWSY.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Once Stylo is enabled by default, we need an easy way to run side-by-side tests with the Gecko style system. We should add --disable-stylo options to all mozharness scripts that currently have --enable-stylo options.
Comment on attachment 8900941 [details] Bug 1393234 - Check STYLO_FORCE_DISABLED in reftests. https://reviewboard.mozilla.org/r/172392/#review177910 ::: layout/tools/reftest/reftest.jsm:728 (Diff revision 1) > #else > sandbox.webrtc = false; > #endif > > #ifdef MOZ_STYLO > - sandbox.stylo = > + let styloEnabled = false; this is assuming stylo is not enabled by default, when we make the switch next week will we set styloEnabled=true by default?
Attachment #8900941 - Flags: review?(jmaher) → review+
Comment on attachment 8900942 [details] Bug 1393234 - Add --disable-stylo for desktop tests. https://reviewboard.mozilla.org/r/172394/#review177918 ::: testing/mochitest/runtests.py:1405 (Diff revision 1) > if options.extra_mozinfo_json: > mozinfo.update(options.extra_mozinfo_json) > if 'STYLO_FORCE_ENABLED' in os.environ: > mozinfo.update({'stylo': True}) > + if 'STYLO_FORCE_DISABLED' in os.environ: > + mozinfo.update({'stylo': False}) is there a default value in mozinfo? I would like to know it is false now and we will flip a bit at some point in the near future.
Attachment #8900942 - Flags: review?(jmaher) → review+
Comment on attachment 8900943 [details] Bug 1393234 - Add --disable-stylo for WPT. https://reviewboard.mozilla.org/r/172396/#review177922 ::: testing/mozharness/scripts/web_platform_tests.py:100 (Diff revision 1) > + [["--disable-stylo"], { > + "action": "store_true", > + "dest": "disable_stylo", > + "default": False, > + "help": "Run tests with Stylo disabled"} > + ], can we ensure that both --enable-stylo and --disable-stylo are not specified- maybe throw an error? I am concerned as we could accidentallyl do something and be testing the wrong thing.
Attachment #8900943 - Flags: review?(jmaher) → review-
Comment on attachment 8900944 [details] Bug 1393234 - Add --disable-stylo for Talos. https://reviewboard.mozilla.org/r/172398/#review177926 ::: testing/talos/talos/cmdline.py:180 (Diff revision 1) > + add_arg('--disable-stylo', action="store_true", > + dest='disable_stylo', > + help='If given, disable Stylo via Environment variables.') > add_arg('--stylo-threads', type=int, > dest='stylothreads', > - help='If given, run stylo with a certain number of threads') > + help='If given, run Stylo with a certain number of threads') I would like a check to ensure that if --disable-stylo is specified both --enable-stylo and --stylo-threads are not specified. ::: testing/talos/talos/run_tests.py:198 (Diff revision 1) > # if e10s add as extra results option > if config['e10s']: > talos_results.add_extra_option('e10s') > > # stylo is another option for testing > - if config['stylo']: > + # TODO: What should happen here when Stylo becomes default? we should keep the 'stylo' tag on the data until we are not testing it- otherwise we need to add another tag for 'gecko' to be the non-stylo cases; possibly that is the route to go and when stylo is default we drop the stylo tag and accept the single shift in numbers. If you like the 'gecko' idea, maybe put some plumbing in here for that?
Attachment #8900944 - Flags: review?(jmaher) → review-
Comment on attachment 8900945 [details] Bug 1393234 - Add --disable-stylo for AWSY. https://reviewboard.mozilla.org/r/172400/#review177928 same issue, in general, lets make sure all harnesses guard against adding both enable and disable flags- otherwise the logic looks good. ::: testing/awsy/awsy/process_perf_data.py:70 (Diff revision 1) > 'name': name, > 'subtests': [], > 'lowerIsBetter': True, > 'units': 'bytes' > } > + # TODO: What should happen here when Stylo becomes default? I think the same comment goes with the talos patch- I am leaning towards taking the hit sooner than later. My only hesitancy is if we revert back at the last minute, it could get real confusion- then again, probably not as confusing as some big patches landing/backing out.
Attachment #8900945 - Flags: review?(jmaher) → review-
Comment on attachment 8900941 [details] Bug 1393234 - Check STYLO_FORCE_DISABLED in reftests. https://reviewboard.mozilla.org/r/172392/#review177910 > this is assuming stylo is not enabled by default, when we make the switch next week will we set styloEnabled=true by default? This code is prepared for the flip to default: 1. We will add a line in configure[1] to set `enable_stylo` to true on Nightly, like this patch[2] in my queue 2. This propagates[3] to the define `MOZ_STYLO_ENABLE` 3. This sets[4] the default value of the pref to `true`. So, we were already prepared for the default case without this change. It only expands the check to also handle force disabling for Gecko mode test jobs. [1]: http://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/toolkit/moz.configure#589 [2]: https://github.com/jryans/gecko-dev/commit/290202d5509e4219db3e53299d4d996f0f474313 [3]: http://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/toolkit/moz.configure#811 [4]: http://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/modules/libpref/init/all.js#5748
Comment on attachment 8900942 [details] Bug 1393234 - Add --disable-stylo for desktop tests. https://reviewboard.mozilla.org/r/172394/#review177918 > is there a default value in mozinfo? I would like to know it is false now and we will flip a bit at some point in the near future. Yes, there is[1], it's controlled by the `MOZ_STYLO_ENABLE` define which will change as part of the flip. [1]: http://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/python/mozbuild/mozbuild/mozinfo.py#85
Comment on attachment 8900943 [details] Bug 1393234 - Add --disable-stylo for WPT. https://reviewboard.mozilla.org/r/172396/#review177922 > can we ensure that both --enable-stylo and --disable-stylo are not specified- maybe throw an error? I am concerned as we could accidentallyl do something and be testing the wrong thing. Good idea, I have added these checks.
Comment on attachment 8900944 [details] Bug 1393234 - Add --disable-stylo for Talos. https://reviewboard.mozilla.org/r/172398/#review177926 > I would like a check to ensure that if --disable-stylo is specified both --enable-stylo and --stylo-threads are not specified. Added. > we should keep the 'stylo' tag on the data until we are not testing it- otherwise we need to add another tag for 'gecko' to be the non-stylo cases; possibly that is the route to go and when stylo is default we drop the stylo tag and accept the single shift in numbers. > > If you like the 'gecko' idea, maybe put some plumbing in here for that? I think I agree it makes sense to stop tagging once something becomes the default. So for the Stylo disabled case, I updated this to add a new "stylo_disabled" tag. ("gecko" alone has too many meanings, and "gecko_style_system" is a bit verbose, so I think "stylo_disabled" is clear enough.)
Comment on attachment 8900945 [details] Bug 1393234 - Add --disable-stylo for AWSY. https://reviewboard.mozilla.org/r/172400/#review177928 > I think the same comment goes with the talos patch- I am leaning towards taking the hit sooner than later. My only hesitancy is if we revert back at the last minute, it could get real confusion- then again, probably not as confusing as some big patches landing/backing out. Added a `stylo-disabled` tag (AWSY seems to use a dash convention instead of underscore...)
Comment on attachment 8900943 [details] Bug 1393234 - Add --disable-stylo for WPT. https://reviewboard.mozilla.org/r/172396/#review178154 thanks!
Attachment #8900943 - Flags: review?(jmaher) → review+
Comment on attachment 8900944 [details] Bug 1393234 - Add --disable-stylo for Talos. https://reviewboard.mozilla.org/r/172398/#review178156 ::: testing/talos/talos/run_tests.py:201 (Diff revisions 1 - 2) > # stylo is another option for testing > - # TODO: What should happen here when Stylo becomes default? > if config['enable_stylo']: > talos_results.add_extra_option('stylo') > + if config['disable_stylo']: > + talos_results.add_extra_option('stylo_disabled') I like this :)
Attachment #8900944 - Flags: review?(jmaher) → review+
Comment on attachment 8900945 [details] Bug 1393234 - Add --disable-stylo for AWSY. https://reviewboard.mozilla.org/r/172400/#review178160
Attachment #8900945 - Flags: review?(jmaher) → review+
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c68579fe828f Check STYLO_FORCE_DISABLED in reftests. r=jmaher https://hg.mozilla.org/integration/autoland/rev/6753af360e83 Add --disable-stylo for desktop tests. r=jmaher https://hg.mozilla.org/integration/autoland/rev/bfc5b68f4a97 Add --disable-stylo for WPT. r=jmaher https://hg.mozilla.org/integration/autoland/rev/05a1b7e0ddec Add --disable-stylo for Talos. r=jmaher https://hg.mozilla.org/integration/autoland/rev/436aaf777aa4 Add --disable-stylo for AWSY. r=jmaher
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: