Closed Bug 1186748 Opened 9 years ago Closed 9 years ago

Switch all builds to Gtk+3

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Depends on 1 open bug)

Details

Attachments

(8 files, 4 obsolete files)

18.97 KB, patch
glandium
: review+
Details | Diff | Splinter Review
13.80 KB, patch
mshal
: review+
Details | Diff | Splinter Review
3.03 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.39 KB, patch
glandium
: review+
Details | Diff | Splinter Review
3.36 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.16 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.46 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.99 KB, patch
mccr8
: review+
bholley
: review+
Details | Diff | Splinter Review
Bug 1186003 didn't enable Gtk+3 on *all* builds, and landing bug 1186229 broke several builds as a consequence. The fixup was to manually switch to Gtk+2 in the relevant mozconfigs. This bug is about actually switching those builds to Gtk+3.
Depends on: 1177951
Some mozconfigs don't include mozconfig.linux*, and don't get gtk-related
definitions, so move them in a separate mozconfig. To avoid having two
files, one for 32-bit builds and one for 64-bit builds, rely on the
includer to set PKG_CONFIG_LIBDIR appropriately.

At the same time, move all the --enable-default-toolkit=cairo-gtk2 in that
new file in the case the gtk3 package wasn't pulled from tooltool.
Attachment #8637862 - Flags: review?(mshal)
valgrind builds will be dealt with later, they require more work still.
Attachment #8637863 - Flags: review?(mshal)
Depends on: 1186875
Attachment #8637862 - Flags: review?(mshal) → review+
Comment on attachment 8637863 [details] [diff] [review]
Enable Gtk+3 on all build jobs except valgrind

>--- a/browser/config/mozconfigs/linux64/hazards
>+++ b/browser/config/mozconfigs/linux64/hazards
>+TOOLTOOL_DIR="$(dirname $topsrcdir)"

What is TOOLTOOL_DIR for? I don't see it used anywhere - was it left in accidentally?
Attachment #8637863 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8637863 [details] [diff] [review]
> Enable Gtk+3 on all build jobs except valgrind
> 
> >--- a/browser/config/mozconfigs/linux64/hazards
> >+++ b/browser/config/mozconfigs/linux64/hazards
> >+TOOLTOOL_DIR="$(dirname $topsrcdir)"
> 
> What is TOOLTOOL_DIR for? I don't see it used anywhere - was it left in
> accidentally?

No, I forgot a part of the change when I moved a part to the patch for valgrind builds that are not covered here.
Attachment #8637863 - Attachment is obsolete: true
Attachment #8638134 - Flags: review?(mshal)
Comment on attachment 8638134 [details] [diff] [review]
Enable Gtk+3 on all build jobs except valgrind

Can we get rid of TOOLTOOL_DIR if hazard_build.py ran tooltool in the source directory instead of abs_work_dir? No need to block on that, but if that's reasonable we can file a followup.
Attachment #8638134 - Flags: review?(mshal) → review+
I'm not the right person to ask. That would be a question for sfink, I guess.
Flags: needinfo?(sphink)
Blocks: gtk3
Depends on: 1187101
Depends on: 1187104
Will gtk2 still be "supported" or will that code be removed?
Depends on: 1187245
Depends on: 1187533
Carrying over r+. This rebases the previous patch on top of bug 1187245
Attachment #8637862 - Attachment is obsolete: true
Attachment #8638970 - Flags: review+
Like the previous, but covers more platforms, including valgrind. Note this may be landed in smaller parts, so that all platforms that are currently completely green can be turned to Gtk+3 without waiting for the more problematic ones.
Attachment #8638134 - Attachment is obsolete: true
Attachment #8638971 - Flags: review?(mshal)
This is for when the "Enable Gtk+3 on all build jobs" part is fully landed, at which point we don't need to fall back to --enable-default-toolkit=cairo-gtk2.
Attachment #8638972 - Flags: review?(mshal)
No longer depends on: 1186875
Depends on: 1187649
Depends on: 1187664
Attachment #8638971 - Flags: review?(mshal) → review+
Comment on attachment 8638972 [details] [diff] [review]
Now that all builds are pulling the Gtk+3 tooltool package, remove the Gtk+2 fallback in mozconfig.gtk

> # $TOOLTOOL_DIR/gtk3 comes from tooltool, when the tooltool manifest contains it.

Now that all tooltool manifests contain it, maybe "$TOOLTOOL_DIR/gtk3 comes from tooltool, which must be included in the tooltool manifest" or some such?
Attachment #8638972 - Flags: review?(mshal) → review+
Keywords: leave-open
Attachment #8638970 - Flags: checkin+
Comment on attachment 8638971 [details] [diff] [review]
Enable Gtk+3 on all build jobs

This patch was landed partially. I'll attach the remaining parts as separate patches.
Attachment #8638971 - Flags: checkin+
This is rebased on top of bug 1188780
Attachment #8640901 - Flags: review+
This is rebased on top of bug 1188780
Attachment #8640902 - Flags: review+
Can you give more details wrt the decision to backout because all I see is oranges to possibly file here? Also note that the current state of non-ASan is to use Gtk+3, and that ASan builds not covering that is kind of a problem.
Depends on: 1189592
Attachment #8640901 - Flags: checkin+
Didn't reland ASan waiting for an answer to comment 23.

This is the part switching for ASan, rebased on top of bug 1188780.
Flags: needinfo?(ryanvm)
Attachment #8641540 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #23)
> Can you give more details wrt the decision to backout because all I see is
> oranges to possibly file here? Also note that the current state of non-ASan
> is to use Gtk+3, and that ASan builds not covering that is kind of a problem.

They were frequent enough that they warranted backout, which is a right we've always reserved. Sorry that it's inconvenient to you.
Flags: needinfo?(ryanvm)
Note that we've been seeing similar screentopng errors on regular Linux builds since the GTK3 switch was made, so there appears to be a real regression there. That breaks screenshotting after mochitest failures, i.e.

https://treeherder.mozilla.org/logviewer.html#?job_id=1926821&repo=mozilla-central - which yields "04:42:03 INFO - Failed to start /builds/slave/test/build/tests/bin/screentopng for screenshot: No such file or directory"

Which then yields http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-central/sha512/cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e
The screentopng thing is bug 1189526
Depends on: 1190180
had to back this out again for causing Bug 1190180 / Bug 1190317 or depend on the backout of https://hg.mozilla.org/mozilla-central/rev/d093c57c6835
Flags: needinfo?(mh+mozilla)
I think the backout didn't actually fix the issue we saw.
So, after being backed out with barely an explanation, now, it's backed out without even being able to find those jobs that are failing, because neither mentioned bug gives treeherder links, and those jobs don't seem to appear on treeherder...

Also, thank you for backing out the whole thing, instead of just the relevant part...

</frustration>
Flags: needinfo?(mh+mozilla) → needinfo?(cbook)
And the not landed B2G desktop part, rebased on bug 1188780
Attachment #8642705 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #34)
> So, after being backed out with barely an explanation, now, it's backed out
> without even being able to find those jobs that are failing, because neither
> mentioned bug gives treeherder links, and those jobs don't seem to appear on
> treeherder...


yeah, it's not _really_ easy to move back from a log to a treeherder job.
With the task name (for example h1dtMu9PRNacSNkGu2PzfA) you can find the task with taskcluster's task inspector; from there you have some information in https://tools.taskcluster.net/task-inspector/#h1dtMu9PRNacSNkGu2PzfA/.

But I agree it would have been easier with all the information in the first place, sorry for this.

> 
> Also, thank you for backing out the whole thing, instead of just the
> relevant part...
> 

Just FYI I know Carsten tried to backout only the relevant part, but there was some conflicts and he thought it would be less error-prone to backout more things without conflicts.
Attachment #8642705 - Flags: checkin+
Depends on: 1191209
Depends on: 1191212
Flags: needinfo?(cbook)
On my CentOS 6.6 64-bit machine, I am no longer able to run lastest Firefox Dev Edition (42.0a2), since it does not have GTK3. Are you guys dropping support for CentOS 6?

XPCOMGlueLoad error for file /data/apps/firefox-dev-ed/libmozgtk.so:
libgtk-3.so.0: cannot open shared object file: No such file or directory
Couldn't load XPCOM.

ldd libmozgtk.so
	linux-vdso.so.1 =>  (0x00007fff20509000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fefd7622000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007fefd741d000)
	libgtk-3.so.0 => not found
	libgdk-3.so.0 => not found
	libstdc++.so.6 => /usr/lib64/libstdc++.so.6 (0x00007fefd7116000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fefd6d82000)
	/lib64/ld-linux-x86-64.so.2 (0x0000003985200000)
	libm.so.6 => /lib64/libm.so.6 (0x00007fefd6afe000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007fefd68e7000)
(In reply to Neon from comment #43)
> On my CentOS 6.6 64-bit machine, I am no longer able to run lastest Firefox
> Dev Edition (42.0a2), since it does not have GTK3. Are you guys dropping
> support for CentOS 6?

In practice, yes. But I'd expect CentOS 6 to have its own firefox packages with GTK2 support.
Depends on: 1204358
Attachment #8640902 - Flags: checkin+
Flagging Andrew for the LSAN suppression (which is something that would have been added when LSAN was enabled had Gtk+3 been enabled for ASAN back then), and Bobby for the e10s mochitest skip (because running ASAN on the same thing we're planning to ship is more important than a single test that intermittently fails only on ASAN (I thought it failed on normal builds too, but it looks like it doesn't, or at a _very_ low rate))
Attachment #8682327 - Flags: review?(continuation)
Attachment #8682327 - Flags: review?(bobbyholley)
Comment on attachment 8682327 [details] [diff] [review]
Make ASAN builds happy with Gtk+3

Review of attachment 8682327 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the suppression changes.

::: build/sanitizers/lsan_suppressions.txt
@@ +115,5 @@
>  leak:libstdc++.so
>  leak:libXrandr.so
>  leak:pthread_setspecific_internal
>  leak:swrast_dri.so
> +leak:libpixman-1.so

Please insert this earlier so that this list at the bottom remains in alphabetical order.
Attachment #8682327 - Flags: review?(continuation) → review+
Comment on attachment 8682327 [details] [diff] [review]
Make ASAN builds happy with Gtk+3

Review of attachment 8682327 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8682327 - Flags: review?(bobbyholley) → review+
Keywords: leave-open
Depends on: 1223198
Blocks: 1221502
There are various blocks like this in the code:

> #if (MOZ_WIDGET_GTK == 2)
> ...
> #else
> ...
> #endif

And also ones with |MOZ_WIDGET_GTK == 3|. Are these removable now, or soon? I.e. when can we drop support for GTK 2?
Flags: needinfo?(mh+mozilla)
(In reply to Nicholas Nethercote [:njn] from comment #53)
> There are various blocks like this in the code:
> 
> > #if (MOZ_WIDGET_GTK == 2)
> > ...
> > #else
> > ...
> > #endif
> 
> And also ones with |MOZ_WIDGET_GTK == 3|. Are these removable now, or soon?
> I.e. when can we drop support for GTK 2?

Please don't remove support for Gtk2. We (Red Hat) need to ship Firefox on non-Gtk3 systems like RHEL5, 6, CentOS 6 and so. I volunteer to maintain this gtk2 target.
cf. Martin's answer.
Flags: needinfo?(mh+mozilla)
(In reply to Michael Shal [:mshal] from comment #6)
> Comment on attachment 8638134 [details] [diff] [review]
> Enable Gtk+3 on all build jobs except valgrind
> 
> Can we get rid of TOOLTOOL_DIR if hazard_build.py ran tooltool in the source
> directory instead of abs_work_dir? No need to block on that, but if that's
> reasonable we can file a followup.

I've always hated where tooltool runs from, but changing it broke a bunch of stuff. I am working on retiring all mozharness hazard builds in bug 1250709, in favor of shell script driven taskcluster builds, so this will shortly become irrelevant.
Flags: needinfo?(sphink)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: