webkit-patch rebaseline-cl fails to create baselines that pass mac_chromium_rel_ng |
|||||||
Issue descriptionWhat steps will reproduce the problem? (1) Run webkit-patch rebaseline-cl on https://chromium-review.googlesource.com/c/chromium/src/+/718752 for patchset 7 and 8 (2) Observe trybot and rebaseline bots results What is the expected result? Rebaseline CL manages to create rebaselines to greenify mac bots What happens instead? mac10.12_blink_rel and mac10.10_blink_rel, as well as mac_chromium_rel_ng stay red despite having run webkit-patch rebaseline-cl.
,
Oct 18 2017
,
Oct 18 2017
I now tried running $ run-webkit-tests -t gnrelease --test-list=hb_failures.txt --reset-results --copy-baselines to generate baselines locally, then upload those to the bots and I still get failures on mac10.12 and mac10.10 - but only one failure on mac_chromium_rel. Now, if I try to use the webkit-patch rebaseline-cl tool again, it goes ahead and attempts to overwrite my manual rebaselines in LayoutTests/platform/mac. This is blocking me from landing an important HarfBuzz roll in https://chromium-review.googlesource.com/c/chromium/src/+/718752 so any help or workaround suggestions are appreciated, thanks!
,
Oct 18 2017
One possible option to try next: Stefan just added a "--builders" to rebaseline-cl, so after syncing to HEAD you can now try to download baselines only for mac10.10: webkit-patch rebaseline-cl --builders mac10.10_blink_rel In theory, that should only reset the baselines in LayoutTests/platform/mac/ if they're the same, but you could try to make it not do that by adding the --no-optimize flag: webkit-patch rebaseline-cl --no-optimize --builders mac10.10_blink_rel It's surprising that there are so many more failures on mac10.12_blink_rel than mac_chromium_rel_ng on the latest patchset -- supposedly they're both running 10.12 and using the same baselines. Looking at the latest mac10.12 failures[1] , do you think the "expected" are correct? Could the difference be because of some aspect of font setup on the different machines? [1] https://storage.googleapis.com/chromium-layout-test-archives/mac10_12_blink_rel/2225/layout-test-results/test-expectations.html
,
Oct 18 2017
Thanks, Quinten, for having a look. > It's surprising that there are so many more failures on mac10.12_blink_rel than mac_chromium_rel_ng on the latest patchset -- supposedly they're both running 10.12 and using the same baselines. Looking at the latest mac10.12 failures[1] , do you think the "expected" are correct? Could the difference be because of some aspect of font setup on the different machines? Agree, would you be able to check further on what mac OS versions / builds these machines are? The expected is correct, yes: Narrower and "more natural"™ looking spacing is the improvement of the CL. Can I pull baselines from the mac_chromium_rel builder with Stefan's command extension, too? Can I use local test results, rebaselines? How to best fuse those with webkit-patch rebaseline*?
,
Oct 18 2017
I *believe* the difference is that mac10.12_blink_rel is configured to use Mac 10.12.6 specifically, whereas mac_chromium_rel_ng uses 10.12, but not necessarily 10.12.6. From: https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.blink%2Fmac10.12_blink_rel%2F2225%2F%2B%2Frecipes%2Fsteps%2Fwebkit_layout_tests_on_Intel_GPU_on_Mac__with_patch__on_Mac-10.12.6%2F0%2Flogs%2Fstep_metadata%2F0 https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2Fmac_chromium_rel_ng%2F568419%2F%2B%2Frecipes%2Fsteps%2Fwebkit_layout_tests_on_Intel_GPU_on_Mac__with_patch__on_Mac-10.12%2F0%2Flogs%2Fstep_metadata%2F0 Unfortunately rebaseline-cl doesn't support pulling from CQ try bots currently (bug 739522). You can definitely use local test results as baselines, and mostly it should be OK but webkit-patch rebaseline* commands might clobber some of these. The --no-optimize and --no-fill-missing flags for rebaseline-cl can disable particular behaviors of that tool to prevent clobbering things. Also of course keep in mind that the baselin fallback path is a tree (attached diagram).
,
Oct 23 2017
> I *believe* the difference is that mac10.12_blink_rel is configured to use Mac 10.12.6 specifically, whereas mac_chromium_rel_ng uses 10.12, but not necessarily 10.12.6. It doesn't look like it. I checked the versions on mac10.12_blink_rel and both builders for it vm191-m4 and vm1445-m4 are on 10.12.2 Buildnumber 16C68. I spot checked some builders for mac_chromium_rel: They are vm213-m4 10.12.2 16C68 vm697-m4 10.12.2 16C68 So they are on the same versions. But the baselines I made on my local machine were from 10.12.6, so that may be the reason they did not work :( The builders should be on latest 10.12, too. > You can definitely use local test results as baselines, and mostly it should be OK but webkit-patch rebaseline* commands might clobber some of these. The --no-optimize and --no-fill-missing flags for rebaseline-cl can disable particular behaviors of that tool to prevent clobbering things. How? Can I make rebaseline-cl ingest local test results? Or should I use --reset-results --copy-baselines on run-webkit-tests or what procedure do you recommend?
,
Oct 23 2017
What I had in mind was something like: 1. Use webkit-patch rebaseline-cl to get results from builders 2. Use run-webkit-tests --reset-results --copy-baselines to overwrite some baselines for the specific platform. But, if both mac10.12_blink_rel and mac_chromium_rel_ng are using the same version, then we would expect the baselines generated by mac10.12_blink_rel to pass mac_chromium_rel_ng, unless there's some other configuration difference between the bots. Is there anything else that might be important besides the version?
,
Oct 24 2017
mac_chromium_rel_ng will be configured to run as a release config w/ DCHECKs enabled, so there might be some differences there. However, if they do fail to pass, you'll probably just need to skip the tests since as you know there's no good support for dchecks directly.
,
Oct 24 2017
Turns out there might be differences in the swarming dimension that the layout tests are requesting. Exploring that further in issue: 777773 The actual buildbot buildslave OS versions do not seem to matter for layout tests anymore, I was probably on the wrong track there. For Layout tests we would need to look at the swarming fleet OS configuration.
,
Oct 24 2017
Re #9: the failures are only layout test failures related to how system fonts are spaced, as expected from the HarfBuzz roll, but nevertheless, I am expecting consistent swarming layout tests results between mac_chromium_rel and mac10.12_blink_rel. Now issue 777773 is fixed, and I am running out of ideas what those differences could be. Do you have any other ideas, Dirk or Quinten, or could you help me look into it? Thanks in advance.
,
Oct 24 2017
I haven't really looked at the failures yet to see what's failing. I will try to take a look / reproduce later today if I have time and I should have some more ideas after that.
,
Oct 24 2017
Thank you, Dirk! As a bit of background: The main change we do in HarfBuzz is calling CTFontCreateUIFontForLanguage for creating an instance of the system font for shaping when the request font is ".NS SF Display" or ".NS SF Text". This CoreText call activates tracking/improved letter spacing in CoreText. That's why the rebaselined results are usually narrower, which is correct. I am not sure what kind of differences we may see across minor versions or build numbers of mac OS, or whether anything else plays into the behavior of this CoreText call.
,
Oct 25 2017
It seems to have something to do with mac SDK version. mac_chromium_rel runs with FORCE_MAC_TOOLCHAIN in the environment variables, which, as far as I understand downloads a copy of "hermetic" XCode and builds with that, SDK target 10.12. blink10.12_mac_rel runs with FORCE_MAC_SDK_MIN=10.10 which tries to find a local 10.10. SDK and build against that as SDK target. If I patch the CL to override FORCE_MAC_SDK_MIN with "10.12", compare https://chromium-review.googlesource.com/c/chromium/src/+/718752/21/build/config/mac/mac_sdk_overrides.gni Then I seem to get consistent results on both of those bots. Can we move the rebaseline-cl trybots to the FORCE_MAC_TOOLCHAIN build configuration as well?
,
Oct 25 2017
Sounds like a good idea, I think we want to use the 10.12 SDK where possible. I'm not sure, I think this might involve changing //tools/mb/mb_config.pyl.pyl -- there's a "mixin" that can be applied to builder gn configs there called 'mac_new_sdk', which sets 'gn_args': 'mac_sdk_min="10.12"' -- but mac_chromium_rel_ng doesn't use that now (it uses 'gpu_tests_release_trybot', which doesn't include this mixin). Dirk, is that the right place to look? Or is there somewhere else? Related: bug 624049
,
Oct 26 2017
There seems to be some environment vars magic in the chromium/tools/build infrastructure that sets these FORCE_MAC_TOOLCHAIN and FORCE_MAC_SDK_MIN variables. Can we remove this and move to //tools/mb/mb_config.pyl to configure the mac SDK dependency?
,
Oct 26 2017
,
Oct 26 2017
If you Erik or Dirk have suggestions on how to change the tools/build slave scripts, that'd be appreciated. I'll try to dig in and understand those, but that might take me a while.
,
Oct 26 2017
The SDK variables are used for two things: 1) Downloading the appropriate toolchain during "gclient sync" 2) Configuring the build steps to use pass around the appropriate parameters. Given that (1) happens before [and is unrelated to] //tools/mb/mb_config.pyl, we currently cannot move the variable to the latter location. In an ideal world, all of our builds would be using the 10.12 SDK. https://bugs.chromium.org/p/chromium/issues/detail?id=669240 This is blocked on getting Blink macOS bots to use swarming: https://bugs.chromium.org/p/chromium/issues/detail?id=524758
,
Oct 27 2017
Thanks Erik for the explanations. It seems to me from [1] that the Blink bots at least use swarming for layout tests already. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=524758#c180 Is there something missing or blocking us from changing the environment variable at least for mac10.12_blink_rel?
,
Oct 27 2017
Nothing should block for mac10.12, although ideally we could move everything over. I sent tansell and dirk an email to figure out what's going on.
,
Oct 30 2017
Would this CL work for switching 10.12. over? https://chromium-review.googlesource.com/c/chromium/tools/build/+/743942 I hope this would resolve the immediate problem of layout test differences between mac10.12_blink_rel and mac_chromium_rel, which both execute on 10.12 but link against different SDKs at them moment. Thanks for your review, or advice on what approach to use.
,
Oct 30 2017
It should help, at least.
,
Oct 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/build/+/3ae96a1c8c8c220b5bbc389ffac5ae455110645f commit 3ae96a1c8c8c220b5bbc389ffac5ae455110645f Author: Dominik Röttsches <drott@chromium.org> Date: Tue Oct 31 08:13:05 2017 Do not override SDK version on 10.12 bots While there might be still blockers to switch to the hermetic Mac toolchain universally, move WebKit 10.12 bots to it already to avoid layout test result differences between mac10.12_blink_rel and mac_chromium_rel. Bug: 775826 Change-Id: I3f8d56d191a3842426e3b038a664a03529604987 Reviewed-on: https://chromium-review.googlesource.com/743942 Commit-Queue: Dominik Röttsches <drott@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> [modify] https://crrev.com/3ae96a1c8c8c220b5bbc389ffac5ae455110645f/scripts/slave/recipe_modules/chromium_tests/chromium_webkit.py
,
Oct 31 2017
So the CL landed, but I filed issue 779980 since I am not sure whether the mac10.12_blink_rel bots pick up the change without a master restart? How does this work?
,
Oct 31 2017
AFAIK, changes in recipe_modules/ take effect at the start of the next build, so by now they should have taken effect -- I think this means that rebaselining from mac10.12_blink_rel should work now.
,
Oct 31 2017
Thanks for the clarification, but unfortunately, the mac10.12_blink_rel bots still have 'FORCE_MAC_SDK_MIN': '10.10' in the environment instead of FORCE_MAC_TOOLCHAIN as I was expecting. Compare: https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.blink%2Fmac10.12_blink_rel%2F2386%2F%2B%2Frecipes%2Fsteps%2Fgclient_runhooks__with_patch_%2F0%2Fstdout from https://build.chromium.org/p/tryserver.blink/builders/mac10.12_blink_rel/builds/2386 So the rebaselining in https://chromium-review.googlesource.com/c/chromium/src/+/718752 failed. I am trying to land the HarfBuzz roll for more than a week now. 😌 I do need more help here, how can we move the mac10.12_blink_rel bots to using the 10.12 SDK? Thanks in advance.
,
Oct 31 2017
Yeah, there's something weird going on, someone (likely me) will need to dig into it more, which I'll try to do shortly.
,
Oct 31 2017
Thanks, Dirk, I appreciate your help.
,
Oct 31 2017
I think the problem is the split builder/tester config on chromium.webkit, and I'm not 100% sure that I can easily fix this by just removing the config from the builder. It might be easier (or better) to just make sure all of the mac platforms are running on swarming, and that's more important anyway, so I'm looking at fix that this afternoon.
,
Nov 1 2017
Thanks for investigating, Dirk. > It might be easier (or better) to just make sure all of the mac platforms are running on swarming, and that's more important anyway, so I'm looking at fix that this afternoon. Where do the swarming layout test runs get their binaries from? It's important that the content_shell executing the layout test is built against the 10.12 SDK, isn't it? So if they get it from the same builder as before, wouldn't the problem still occur?
,
Nov 1 2017
> Where do the swarming layout test runs get their binaries from? > It's important that the content_shell executing the layout test is built > against the 10.12 SDK, isn't it? So if they get it from the same builder > as before, wouldn't the problem still occur? Good questions. You're correct that simply moving to swarming by itself doesn't change anything, as the binaries would still be built incorrectly. However, this whole "force mac 10.10 toolchain" thing is the result of not always building on 10.12 (which we can't do, because we're building and running the binaries on the same machine). So, first we move to swarming, then we can get rid of the 10.10 and 10.11 builders, and then the problem goes away, and a bunch of code can be deleted. With luck, that'll all happen today (except for possibly the code deleted part).
,
Nov 1 2017
I'll keep my fingers crossed! :) It would be amazing if I can land the HarfBuzz roll with working rebaselines before the end of the week. Thanks for the clarification and for working on this!
,
Nov 8 2017
Dirk fixed this in https://chromium-review.googlesource.com/c/chromium/tools/build/+/749623 |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by drott@chromium.org
, Oct 18 2017