Switch all builds to Gtk+3

RESOLVED FIXED in Firefox 45

Status

RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Depends on: 1 bug)

unspecified
mozilla45
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(8 attachments, 4 obsolete attachments)

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
(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 1177951
(Assignee)

Comment 1

4 years ago
Created attachment 8637862 [details] [diff] [review]
Move gtk-related things in a separate mozconfig

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)
(Assignee)

Comment 2

4 years ago
Created attachment 8637863 [details] [diff] [review]
Enable Gtk+3 on all build jobs except valgrind

valgrind builds will be dealt with later, they require more work still.
Attachment #8637863 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8638134 [details] [diff] [review]
Enable Gtk+3 on all build jobs except valgrind
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+
(Assignee)

Comment 7

4 years ago
I'm not the right person to ask. That would be a question for sfink, I guess.
Flags: needinfo?(sphink)
(Assignee)

Updated

4 years ago
Blocks: 627699
(Assignee)

Updated

4 years ago
Depends on: 1187101
(Assignee)

Updated

4 years ago
Depends on: 1187104

Comment 8

4 years ago
Will gtk2 still be "supported" or will that code be removed?
(Assignee)

Updated

4 years ago
Depends on: 1187245
(Assignee)

Updated

4 years ago
Depends on: 1187533
(Assignee)

Comment 9

4 years ago
Created attachment 8638970 [details] [diff] [review]
Move gtk-related things in a separate mozconfig

Carrying over r+. This rebases the previous patch on top of bug 1187245
Attachment #8637862 - Attachment is obsolete: true
Attachment #8638970 - Flags: review+
(Assignee)

Comment 10

4 years ago
Created attachment 8638971 [details] [diff] [review]
Enable Gtk+3 on all build jobs

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)
(Assignee)

Comment 11

4 years ago
Created attachment 8638972 [details] [diff] [review]
Now that all builds are pulling the Gtk+3 tooltool package, remove the Gtk+2 fallback in mozconfig.gtk

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)
(Assignee)

Updated

4 years ago
No longer depends on: 1186875
(Assignee)

Updated

4 years ago
Depends on: 1187649
(Assignee)

Updated

4 years ago
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+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Updated

4 years ago
Attachment #8638970 - Flags: checkin+
(Assignee)

Comment 17

4 years ago
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+
(Assignee)

Comment 18

4 years ago
Created attachment 8640901 [details] [diff] [review]
Switch valgrind builds to Gtk+3. r=mshal

This is rebased on top of bug 1188780
Attachment #8640901 - Flags: review+
(Assignee)

Comment 19

4 years ago
Created attachment 8640902 [details] [diff] [review]
Switch mulet builds to Gtk+3

This is rebased on top of bug 1188780
Attachment #8640902 - Flags: review+
(Assignee)

Comment 20

4 years ago
Created attachment 8640904 [details] [diff] [review]
Now that all builds are pulling the Gtk+3 tooltool package, remove the Gtk+2 fallback in mozconfig.gtk

Rebased as well.
Attachment #8638972 - Attachment is obsolete: true
Attachment #8640904 - Flags: review+
(Assignee)

Comment 23

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 1189592
(Assignee)

Updated

4 years ago
Attachment #8640901 - Flags: checkin+
(Assignee)

Comment 25

4 years ago
Created attachment 8641540 [details] [diff] [review]
Switch ASan builds to Gtk+3

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
(Assignee)

Comment 29

4 years ago
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.
(Assignee)

Comment 34

4 years ago
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)
(Assignee)

Comment 37

4 years ago
Created attachment 8642705 [details] [diff] [review]
Switch B2G desktop builds to Gtk+3

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.
(Assignee)

Updated

4 years ago
Attachment #8642705 - Flags: checkin+
(Assignee)

Updated

4 years ago
Depends on: 1191209
(Assignee)

Updated

4 years ago
Depends on: 1191212
Flags: needinfo?(cbook)

Comment 43

4 years ago
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)
(Assignee)

Comment 44

4 years ago
(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.

Updated

3 years ago
Depends on: 1204358
(Assignee)

Updated

3 years ago
Attachment #8640902 - Flags: checkin+
(Assignee)

Comment 47

3 years ago
Created attachment 8682327 [details] [diff] [review]
Make ASAN builds happy with Gtk+3

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+
(Assignee)

Updated

3 years ago
Keywords: leave-open

Comment 51

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3a831ebbcfa
https://hg.mozilla.org/mozilla-central/rev/38fe6c419f08
https://hg.mozilla.org/mozilla-central/rev/d7844fe50165
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1223198

Updated

3 years ago
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.
(Assignee)

Comment 55

3 years ago
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)

Updated

11 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.