Bug 146056 – [GTK] [Wayland] Allow building and testing the Wayland target with the default JHBuild moduleset.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Alberto Lopez Perez]

Currently any developer that wants to build or test the Wayland target is unable to use the default JHBuild moduleset because we use GTK 3.6 there.

We have an optional extra moduleset that allows building the Wayland target with GTK 3.12, but is not very useful because all the the layout test results are for 3.6.

So I propose here the following:

1) Merge the optional jhbuild-wayland.modules into the default jhbuild.modules. This will mean raising the GTK library on the JHBuild to 3.12.
2) Update all the layout test results to match the new set of libraries.

[Carlos Garcia Campos]

Agree with your proposal.

[Martin Robinson]

(In reply to comment #0)

> 1) Merge the optional jhbuild-wayland.modules into the default

I don't think we should merge all optional modules at this point, but just the ones we need for Wayland. If we want to talk about merging all optional modules, we should do it in another bug, because I think it's unrelated to this issue.

[Michael Catanzaro]

I think we should go straight to the latest stable release, 3.16.4, since it's closer to what our users will actually have. There's less value in testing with 3.12.x since it's already quite old.

(In reply to comment #2)
> I don't think we should merge all optional modules at this point, but just
> the ones we need for Wayland. If we want to talk about merging all optional
> modules, we should do it in another bug, because I think it's unrelated to
> this issue.

Agreed; see also bug #145696

[Martin Robinson]

(In reply to comment #3)
> I think we should go straight to the latest stable release, 3.16.4, since
> it's closer to what our users will actually have. There's less value in
> testing with 3.12.x since it's already quite old.

I also agree with Michael here.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> > I think we should go straight to the latest stable release, 3.16.4, since
> > it's closer to what our users will actually have. There's less value in
> > testing with 3.12.x since it's already quite old.
> 
> I also agree with Michael here.

Everytime I have proposed that, for exactly that same reason, the response has been that the rule is to not bump anything unless required by tests. I guess it applies here. Anyway, I never liked that rule, so I, of course, agree on *always* testing with the version as close as possible to what most users have.

[Martin Robinson]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > I think we should go straight to the latest stable release, 3.16.4, since
> > > it's closer to what our users will actually have. There's less value in
> > > testing with 3.12.x since it's already quite old.
> > 
> > I also agree with Michael here.
> 
> Everytime I have proposed that, for exactly that same reason, the response
> has been that the rule is to not bump anything unless required by tests. I
> guess it applies here. Anyway, I never liked that rule, so I, of course,
> agree on *always* testing with the version as close as possible to what most
> users have.

I think we shouldn't bump unless we absolutely need to, as it introduces a lot of churn in test results and forces every developer to recompile the *entire* JHBuild moduleset. It's a lot of work to update many results and the JHBuild compilation time is nothing to sneeze at. On the other hand, if we are forced to update, I don't think there is any issue updating to a recent version.

Still perhaps it is time to reexamine the rule. One idea is that it is fine to update JHBuild moduleset if the updater personally takes responsibility for carefully updating all pixel and layout test results that change as a result.

[Carlos Alberto Lopez Perez]

(In reply to comment #3)
> I think we should go straight to the latest stable release, 3.16.4, since
> it's closer to what our users will actually have. There's less value in
> testing with 3.12.x since it's already quite old.
> 

I think that we may be risking to break compatibility with older versions without noticing.

If we put 3.16.x on the JHBuild, we won't longer test with older versions. The possibility that a patch inadvertently introduces some change that breaks with < 3.16.x is real.

But, the same will happen with an update to 3.12 (is only a matter of limiting the hypothetical breakage to <3.12 instead of <3.16).

In any case, I'm fine with updating GTK to 3.16 if you all are also ok.


I'm wonder if along with the update to GTK-3.16 we should also (or we want to) update any other library that GTK may depend on like glib, cairo, etc?

[Michael Catanzaro]

(In reply to comment #7)
> I think that we may be risking to break compatibility with older versions
> without noticing.
> 
> If we put 3.16.x on the JHBuild, we won't longer test with older versions.
> The possibility that a patch inadvertently introduces some change that
> breaks with < 3.16.x is real.

Hm... I was thinking that we used GDK_VERSION_MAX_ALLOWED to protect against this, but we don't. And we probably can't, since we often have to use newer things in conditional compilation blocks. So you have a good point.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #3)
> > I think we should go straight to the latest stable release, 3.16.4, since
> > it's closer to what our users will actually have. There's less value in
> > testing with 3.12.x since it's already quite old.
> > 
> 
> I think that we may be risking to break compatibility with older versions
> without noticing.

However, what usually happens is the opposite, things stop working or are broken with recent versions of GTK, and we don't notice it, but our users do. Remember the huge scrollbars and other theming issues, the GMutexLocker build issue, etc.

> If we put 3.16.x on the JHBuild, we won't longer test with older versions.
> The possibility that a patch inadvertently introduces some change that
> breaks with < 3.16.x is real.

We just need to be careful to use conditional compilation when using newer GTK+ API. It's not that common. The thing is that what we are testing is far from real.

> But, the same will happen with an update to 3.12 (is only a matter of
> limiting the hypothetical breakage to <3.12 instead of <3.16).
> 
> In any case, I'm fine with updating GTK to 3.16 if you all are also ok.
> 
> 
> I'm wonder if along with the update to GTK-3.16 we should also (or we want
> to) update any other library that GTK may depend on like glib, cairo, etc?

Yes, indeed. We need to check if newer GTK+ also requires newer versions of other deps, I bet it requires newer glib, for example, but also ATK, pango, etc. The other important dependency that affects the layout tests output is cairo. So, I would take advantage that we will have to rebaseline to also update cairo. With recent enough cairo and GTK+ we have HiDPI support, that we have never tested because we were using an old GTK.

[Alberto Garcia]

(In reply to comment #7)
> In any case, I'm fine with updating GTK to 3.16 if you all are also
> ok.

Debian sid has just upgraded to GTK 3.16 so I don't think it will
cause any problem there.

[Carlos Alberto Lopez Perez]

Created attachment 255754 [details]
Patch

Requesting only review for the patch affecting the jhbuild code. Once this is r+, I will prepare to land it followed by an unreviewed gardening patch rebasing all the affected tests.

[Carlos Alberto Lopez Perez]

Created attachment 255920 [details]
Patch

Upload again to get the GTK EWS green now that it has libepoxy installed.

[Carlos Alberto Lopez Perez]

Reviewers please? Any comment regarding this patch?

[Martin Robinson]

Comment on attachment 255920 [details]
Patch

I think this is a good idea, but I'm surprised this changes no results at all (even non-pixel ones).

[Carlos Alberto Lopez Perez]

(In reply to comment #14)
> Comment on attachment 255920 [details]
> Patch
> 
> I think this is a good idea, but I'm surprised this changes no results at
> all (even non-pixel ones).

(In reply to comment #14)
> Comment on attachment 255920 [details]
> Patch
> 
> I think this is a good idea, but I'm surprised this changes no results at
> all (even non-pixel ones).

Thanks for the review.

Without pixel tests this is a summary that I extracted after running three times (to discard flaky tests) the full Layout tests before and after applying this patch:

New failures after applying this patch:

  fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html [ ImageOnlyFailure ]
  fast/css/input-search-padding.html [ Failure ]
  fast/events/touch/tap-highlight-color.html [ Timeout ]
  fast/events/touch/touch-inside-iframe-scrolled.html [ Timeout ]
  fast/events/touch/touch-scaled-scrolled.html [ Timeout ]
  fast/events/touch/touch-stale-node-crash.html [ Timeout ]
  fast/hidpi/filters-blur.html [ ImageOnlyFailure ]
  fast/hidpi/filters-hue-rotate.html [ ImageOnlyFailure ]
  fast/hidpi/filters-invert.html [ ImageOnlyFailure ]
  fast/hidpi/filters-multiple.html [ ImageOnlyFailure ]
  fast/hidpi/filters-reference.html [ ImageOnlyFailure ]
  fast/selectors/matches-backtracking.html [ Timeout ]
  inspector/css/matched-style-properties.html [ Timeout ]
  inspector/css/stylesheet-with-mutations.html [ Timeout ]


New passes after applying this patch:

  fast/canvas/canvas-fillPath-shadow.html
  fast/dom/HTMLImageElement/sizes/image-sizes-2x.html
  fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html
  fast/events/touch/touch-inside-iframe.html
  fast/hidpi/image-srcset-change-dynamically-from-js-2x.html
  fast/hidpi/image-srcset-fraction-1.5x.html
  fast/hidpi/image-srcset-fraction.html
  fast/hidpi/image-srcset-invalid-inputs-except-one.html
  fast/hidpi/image-srcset-simple-2x.html
  fast/hidpi/image-srcset-src-selection-2x.html



With pixel tests enabled, I'm working on identifying the new failures, but is a bit complicated since there are already like ~2000 image failures even before applying this patch. This are the gross numbers are:

Right now, before applying this patch:

=> Tests to be fixed (7036):
      5 crashes                  ( 0.1%)
     85 timeouts                 ( 1.2%)
    886 missing results          (12.6%)
   1990 image-only failures      (28.3%)



After applying this patch:
=> Tests to be fixed (7061):
      8 crashes                  ( 0.1%)
     94 timeouts                 ( 1.3%)
    887 missing results          (12.6%)
   2012 image-only failures      (28.5%)


So, we have at least 22 new image-only failures that need to be rebased. Probably more.

My idea is that before landing this patch, I will work on preparing a gardening patch that will be landed just after landing this one.
I will do my best to check and rebase the tests that I identify as affected by this upgrade of libraries.

The non-pixel test case is manageable and I have already identified the tests affected.
However, for the pixel test case, I don't think I can check all failures, as there are already ~2000 failures before landing this. But, I will try to do my best and rebase/report the tests that I detect as new failures by comparing the previous test failure list with the new one.

[Martin Robinson]

(In reply to comment #15)

> fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-
> precision.html [ ImageOnlyFailure ]
>   fast/css/input-search-padding.html [ Failure ]
>   fast/events/touch/tap-highlight-color.html [ Timeout ]
>   fast/events/touch/touch-inside-iframe-scrolled.html [ Timeout ]
>   fast/events/touch/touch-scaled-scrolled.html [ Timeout ]
>   fast/events/touch/touch-stale-node-crash.html [ Timeout ]
>   fast/hidpi/filters-blur.html [ ImageOnlyFailure ]
>   fast/hidpi/filters-hue-rotate.html [ ImageOnlyFailure ]
>   fast/hidpi/filters-invert.html [ ImageOnlyFailure ]
>   fast/hidpi/filters-multiple.html [ ImageOnlyFailure ]
>   fast/hidpi/filters-reference.html [ ImageOnlyFailure ]
>   fast/selectors/matches-backtracking.html [ Timeout ]
>   inspector/css/matched-style-properties.html [ Timeout ]
>   inspector/css/stylesheet-with-mutations.html [ Timeout ]


These new timeouts worry me quite a bit. Is it possible to figure out what is causing them? The lone failure might just need to be rebased.

[Carlos Alberto Lopez Perez]

(In reply to comment #16)
> These new timeouts worry me quite a bit. Is it possible to figure out what
> is causing them? The lone failure might just need to be rebased.

Running the layout tests only with the 7 tests that now timeout instead of the full site makes some of them work back, so maybe some of this tests have become slow. 

=> Results: 4/7 tests passed (57.1%)

=> Tests to be fixed (3):
      3 timeouts                 (100.0%)

=> Tests that will only be fixed if they crash (WONTFIX) (0):


Expected to fail, but passed: (1)
  fast/events/touch/touch-inside-iframe-scrolled.html


Regressions: Unexpected timeouts (3)
  fast/events/touch/tap-highlight-color.html [ Timeout ]
  fast/events/touch/touch-scaled-scrolled.html [ Timeout ]
  fast/events/touch/touch-stale-node-crash.html [ Timeout ]

------------------------------------------------------------------------------


This new 3 timeouts, are indeed new timeout as they still timeout if you pass the flag --no-timeout. I have no idea at this moment why they timeout with the new set of libraries.

[Carlos Alberto Lopez Perez]

Comment on attachment 255920 [details]
Patch

Clearing flags on attachment: 255920

Committed r186500: <http://trac.webkit.org/changeset/186500>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(In reply to comment #18)
> Comment on attachment 255920 [details]
> Patch
> 
> Clearing flags on attachment: 255920
> 
> Committed r186500: <http://trac.webkit.org/changeset/186500>

Do the timeouts persist on the bots. I didn't see TestExpectations updates in the patch that landed?

[Carlos Alberto Lopez Perez]

(In reply to comment #20)
> (In reply to comment #18)
> > Comment on attachment 255920 [details]
> > Patch
> > 
> > Clearing flags on attachment: 255920
> > 
> > Committed r186500: <http://trac.webkit.org/changeset/186500>
> 
> Do the timeouts persist on the bots. I didn't see TestExpectations updates
> in the patch that landed?

The bot is still processing. I'm waiting to check the results from the bot. I will update this bug once I land the gardening patch

[Carlos Alberto Lopez Perez]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > Comment on attachment 255920 [details]
> > > Patch
> > > 
> > > Clearing flags on attachment: 255920
> > > 
> > > Committed r186500: <http://trac.webkit.org/changeset/186500>
> > 
> > Do the timeouts persist on the bots. I didn't see TestExpectations updates
> > in the patch that landed?
> 
> The bot is still processing. I'm waiting to check the results from the bot.
> I will update this bug once I land the gardening patch

I have updated the expectations file and rebaselined one test on http://trac.webkit.org/changeset/186513

[Carlos Alberto Lopez Perez]

Also I reported here all the new failures/timeouts that I have identified as part of this upgrade of libraries: https://bugs.webkit.org/show_bug.cgi?id=146731