New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 775826 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

webkit-patch rebaseline-cl fails to create baselines that pass mac_chromium_rel_ng

Project Member Reported by drott@chromium.org, Oct 18 2017

Issue description

What 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.

 

Comment 1 by drott@chromium.org, Oct 18 2017

Status: Assigned (was: Available)

Comment 2 by drott@chromium.org, Oct 18 2017

Description: Show this description

Comment 3 by drott@chromium.org, 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!


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

Comment 5 by drott@chromium.org, 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*?


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).
fallback-paths.png
44.0 KB View Download

Comment 7 by drott@chromium.org, 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?



Summary: webkit-patch rebaseline-cl fails to create baselines that pass mac_chromium_rel_ng (was: webkit-patch rebaseline-cl fails to create baselines that pass mac_chromium_rel)
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?
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.

Comment 10 by drott@chromium.org, 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.

Comment 11 by drott@chromium.org, 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.
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.

Comment 13 by drott@chromium.org, 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.

Comment 14 by drott@chromium.org, 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?

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 

Comment 16 by drott@chromium.org, 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?

Comment 17 by drott@chromium.org, Oct 26 2017

Cc: erikc...@chromium.org

Comment 18 by drott@chromium.org, 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.

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


Comment 20 by drott@chromium.org, Oct 27 2017

Cc: tansell@chromium.org
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?

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.

Comment 22 by drott@chromium.org, Oct 30 2017

Owner: drott@chromium.org
Status: Started (was: Assigned)
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.



It should help, at least.
Project Member

Comment 24 by bugdroid1@chromium.org, 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

Comment 25 by drott@chromium.org, 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?


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.

Comment 27 by drott@chromium.org, 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.

Yeah, there's something weird going on, someone (likely me) will need to dig into it more, which I'll try to do shortly.

Comment 29 by drott@chromium.org, Oct 31 2017

Thanks, Dirk, I appreciate your help.

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.
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?

> 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).
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!
Owner: dpranke@chromium.org
Status: Fixed (was: Started)
Dirk fixed this in https://chromium-review.googlesource.com/c/chromium/tools/build/+/749623

Sign in to add a comment