Closed Bug 1256988 Opened 8 years ago Closed 8 years ago

Move MOZ_WIDGET_TOOLKIT to Python configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: chmanchester, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(19 files)

58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
This unlocks a lot of other options. Moving --target in bug 1255305 helps.
Assignee: nobody → mh+mozilla
The default for MOZ_WIDGET_TOOLKIT depends on --target, which is available, but also on $gonkdir and MOZ_IOS, which aren't yet.
Depends on: 1250301
We can get away with just moving the --with-gonk and --with-ios-sdk options.
No longer depends on: 1250301
Depends on: 1257051
(In reply to Mike Hommey [:glandium] from comment #2)
> We can get away with just moving the --with-gonk and --with-ios-sdk options.

To make things more explicit than the added dependency: in the end I'll be moving --with-gonk and changing the required --target for ios.
It hasn't done anything since bug 191447, 13 years ago

Review commit: https://reviewboard.mozilla.org/r/40387/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40387/
Attachment #8731129 - Flags: review?(cmanchester)
Now that the MOZ_WIDGET_TOOLKIT test is in moz.configure, the value for
MOZ_WIDGET_TOOLKIT is now set in old-configure.in very early, which
now allows to check for its value before doing to Xt test instead of
resetting XT_LIBS later.

Review commit: https://reviewboard.mozilla.org/r/40389/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40389/
Attachment #8731130 - Flags: review?(cmanchester)
Remove the AC_DEFINE because it is unused.

Review commit: https://reviewboard.mozilla.org/r/40395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40395/
Attachment #8731133 - Flags: review?(cmanchester)
Note all these patches are not directly about MOZ_WIDGET_TOOLKIT, but they are related to the first block of MOZ_WIDGET_TOOLKIT, and I didn't feel like multipying bugs.
https://reviewboard.mozilla.org/r/40367/#review36895

::: toolkit/moz.configure:20
(Diff revision 1)
> +# `choices` depending on the target, but that doesn't pan out for the same
> +# reason.
> +option('--enable-default-toolkit', nargs=1,
> +       choices=('cairo-windows', 'cairo-gtk2', 'cairo-gtk2-x11', 'cairo-gtk3',
> +                'cairo-qt', 'cairo-cocoa', 'cairo-uikit', 'cairo-android',
> +                'cairo-gonk'), help='Select default toolkit')

Move `help=` to the next line? This formatting is a little confusing.

::: toolkit/moz.configure:26
(Diff revision 1)
> +
> +@depends('--enable-default-toolkit', target, gonkdir)
> +def toolkit(value, target, gonkdir):
> +    # Define possible choices for each platform. The default is the first one
> +    # listed when there are several.
> +    os = target.os

Remove or use in the if-else chain.
https://reviewboard.mozilla.org/r/40397/#review36897

::: toolkit/moz.configure:128
(Diff revision 1)
> +option(env='MOZ_INSTRUMENT_EVENT_LOOP',
> +       help='Force-enable event loop instrumentation')
> +
> +@depends('MOZ_INSTRUMENT_EVENT_LOOP', toolkit)
> +def instrument_event_loop(value, toolkit):
> +    if value or toolkit in ('windows', 'gtk2', 'gtk3', 'cocoa', 'android',

Parens would help me understand the operator precedence here. Also, is `value.origin` safe eve n if `not value`?
https://reviewboard.mozilla.org/r/40399/#review36899

::: toolkit/moz.configure:141
(Diff revision 1)
> +option(env='USE_FC_FREETYPE',
> +       help='Force-enable the use of fontconfig freetype')
> +
> +@depends('USE_FC_FREETYPE', toolkit)
> +def fc_freetype(value, toolkit):
> +    if (value or toolkit in ('gtk2', 'gtk3', 'qt') and

Same question here.
https://reviewboard.mozilla.org/r/40367/#review36895

> Remove or use in the if-else chain.

It *is* used.
https://reviewboard.mozilla.org/r/40397/#review36897

> Parens would help me understand the operator precedence here. Also, is `value.origin` safe eve n if `not value`?

value is not a bool.
Comment on attachment 8731118 [details]
MozReview Request: Bug 1256988 - Move --with-gonk to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40365/diff/1-2/
Comment on attachment 8731119 [details]
MozReview Request: Bug 1256988 - Move MOZ_WIDGET_TOOLKIT to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40367/diff/1-2/
Comment on attachment 8731120 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_ENABLE_GTK2 with tests on MOZ_WIDGET_TOOLKIT==gtk2

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40369/diff/1-2/
Comment on attachment 8731121 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_ENABLE_GTK3 with tests on MOZ_WIDGET_TOOLKIT==gtk3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40371/diff/1-2/
Comment on attachment 8731122 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_ENABLE_GTK with tests on MOZ_WIDGET_TOOLKIT containing gtk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40373/diff/1-2/
Comment on attachment 8731123 [details]
MozReview Request: Bug 1256988 - Remove duplicate gfx.content.azure.backends settings in all.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40375/diff/1-2/
Comment on attachment 8731124 [details]
MozReview Request: Bug 1256988 - Fix #endif comments for MOZ_WIDGET_GTK

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40377/diff/1-2/
Comment on attachment 8731125 [details]
MozReview Request: Bug 1256988 - Remove MOZ_WIDGET_GTK2 define

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40379/diff/1-2/
Comment on attachment 8731126 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_WIDGET_GTK with tests on MOZ_WIDGET_TOOLKIT containing gtk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40381/diff/1-2/
Comment on attachment 8731127 [details]
MozReview Request: Bug 1256988 - Move MOZ_WIDGET_* defines to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40383/diff/1-2/
Comment on attachment 8731128 [details]
MozReview Request: Bug 1256988 - Remove useless Gtk/accessibility check in js/src/old-configure.in

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40385/diff/1-2/
Comment on attachment 8731129 [details]
MozReview Request: Bug 1256988 - Remove NO_X11

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40387/diff/1-2/
Comment on attachment 8731130 [details]
MozReview Request: Bug 1256988 - Skip the Xt library test for MOZ_WIDGET_TOOLKIT==qt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40389/diff/1-2/
Comment on attachment 8731131 [details]
MozReview Request: Bug 1256988 - Move X11 and MOZ_WIDGET_TOOLKIT-related things to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40391/diff/1-2/
Comment on attachment 8731132 [details]
MozReview Request: Bug 1256988 - Move --with-gl-provider to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40393/diff/1-2/
Comment on attachment 8731133 [details]
MozReview Request: Bug 1256988 - Move MOZ_PDF_PRINTING to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40395/diff/1-2/
Comment on attachment 8731134 [details]
MozReview Request: Bug 1256988 - Move MOZ_INSTRUMENT_EVENT_LOOP to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40397/diff/1-2/
Comment on attachment 8731135 [details]
MozReview Request: Bug 1256988 - Move USE_FC_FREETYPE to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40399/diff/1-2/
Comment on attachment 8731136 [details]
MozReview Request: Bug 1256988 - Remove MOZ_TOUCH, nothing is using it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40401/diff/1-2/
Comment on attachment 8731133 [details]
MozReview Request: Bug 1256988 - Move MOZ_PDF_PRINTING to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40395/diff/2-3/
Comment on attachment 8731134 [details]
MozReview Request: Bug 1256988 - Move MOZ_INSTRUMENT_EVENT_LOOP to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40397/diff/2-3/
Comment on attachment 8731135 [details]
MozReview Request: Bug 1256988 - Move USE_FC_FREETYPE to moz.configure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40399/diff/2-3/
Comment on attachment 8731136 [details]
MozReview Request: Bug 1256988 - Remove MOZ_TOUCH, nothing is using it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40401/diff/2-3/
Attachment #8731118 - Flags: review?(cmanchester) → review+
Comment on attachment 8731119 [details]
MozReview Request: Bug 1256988 - Move MOZ_WIDGET_TOOLKIT to moz.configure

https://reviewboard.mozilla.org/r/40367/#review37003
Attachment #8731119 - Flags: review?(cmanchester) → review+
Attachment #8731120 - Flags: review?(cmanchester) → review+
Comment on attachment 8731120 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_ENABLE_GTK2 with tests on MOZ_WIDGET_TOOLKIT==gtk2

https://reviewboard.mozilla.org/r/40369/#review37005
Attachment #8731121 - Flags: review?(cmanchester) → review+
Comment on attachment 8731121 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_ENABLE_GTK3 with tests on MOZ_WIDGET_TOOLKIT==gtk3

https://reviewboard.mozilla.org/r/40371/#review37007
Comment on attachment 8731122 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_ENABLE_GTK with tests on MOZ_WIDGET_TOOLKIT containing gtk

https://reviewboard.mozilla.org/r/40373/#review37009

::: accessible/atk/moz.build:46
(Diff revision 2)
>      '/other-licenses/atk-1.0',
>  ]
>  
>  FINAL_LIBRARY = 'xul'
>  
> -if CONFIG['MOZ_ENABLE_GTK']:
> +if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']:

Should this be "startswith" throughout? I guess it probably doesn't matter.
Attachment #8731122 - Flags: review?(cmanchester) → review+
Comment on attachment 8731123 [details]
MozReview Request: Bug 1256988 - Remove duplicate gfx.content.azure.backends settings in all.js

https://reviewboard.mozilla.org/r/40375/#review37017

It might be helpful to add a note in the commit message that these checks are unecessary because the #else above them.
Attachment #8731123 - Flags: review?(cmanchester) → review+
Comment on attachment 8731124 [details]
MozReview Request: Bug 1256988 - Fix #endif comments for MOZ_WIDGET_GTK

https://reviewboard.mozilla.org/r/40377/#review37019
Attachment #8731124 - Flags: review?(cmanchester) → review+
Comment on attachment 8731125 [details]
MozReview Request: Bug 1256988 - Remove MOZ_WIDGET_GTK2 define

https://reviewboard.mozilla.org/r/40379/#review37021
Attachment #8731125 - Flags: review?(cmanchester) → review+
Comment on attachment 8731126 [details]
MozReview Request: Bug 1256988 - Replace tests on MOZ_WIDGET_GTK with tests on MOZ_WIDGET_TOOLKIT containing gtk

https://reviewboard.mozilla.org/r/40381/#review37055
Attachment #8731126 - Flags: review?(cmanchester) → review+
Comment on attachment 8731127 [details]
MozReview Request: Bug 1256988 - Move MOZ_WIDGET_* defines to moz.configure

https://reviewboard.mozilla.org/r/40383/#review37061
Attachment #8731127 - Flags: review?(cmanchester) → review+
Comment on attachment 8731128 [details]
MozReview Request: Bug 1256988 - Remove useless Gtk/accessibility check in js/src/old-configure.in

https://reviewboard.mozilla.org/r/40385/#review37063
Attachment #8731128 - Flags: review?(cmanchester) → review+
Attachment #8731129 - Flags: review?(cmanchester) → review+
Comment on attachment 8731130 [details]
MozReview Request: Bug 1256988 - Skip the Xt library test for MOZ_WIDGET_TOOLKIT==qt

https://reviewboard.mozilla.org/r/40389/#review37077

::: old-configure.in:3454
(Diff revision 2)
>      MOZ_ENABLE_QT=1
>      if test -z "$WITHOUT_X11"; then
>        MOZ_ENABLE_XREMOTE=1
>        MOZ_GL_DEFAULT_PROVIDER=GLX
>        MOZ_X11=1
>        AC_DEFINE(MOZ_X11)
> -      XT_LIBS=
>      fi

So if we have MOZ_WIDGET_TOOLKIT == qt, and WITHOUT_X11, XT_LIBS would possibly be non-empty before, but it's never set now when MOZ_WIDGET_TOOLKIT == qt.

I'm trusting this is either impossible, or unimportant.
Attachment #8731130 - Flags: review?(cmanchester) → review+
Comment on attachment 8731131 [details]
MozReview Request: Bug 1256988 - Move X11 and MOZ_WIDGET_TOOLKIT-related things to moz.configure

https://reviewboard.mozilla.org/r/40391/#review37079

::: old-configure.in:2112
(Diff revision 2)
>  dnl ========================================================
>  dnl Checks for X libraries.
>  dnl Ordering is important.
>  dnl Xt is dependent upon SM as of X11R6
>  dnl ========================================================
> -if test "$no_x" != "yes"; then
> +if test -n "$MOZ_X11"; then

Glad to get rid of this double negative!
Attachment #8731131 - Flags: review?(cmanchester) → review+
Attachment #8731132 - Flags: review?(cmanchester) → review+
Comment on attachment 8731132 [details]
MozReview Request: Bug 1256988 - Move --with-gl-provider to moz.configure

https://reviewboard.mozilla.org/r/40393/#review37081
Comment on attachment 8731133 [details]
MozReview Request: Bug 1256988 - Move MOZ_PDF_PRINTING to moz.configure

https://reviewboard.mozilla.org/r/40395/#review37085
Attachment #8731133 - Flags: review?(cmanchester) → review+
Attachment #8731134 - Flags: review?(cmanchester) → review+
Comment on attachment 8731134 [details]
MozReview Request: Bug 1256988 - Move MOZ_INSTRUMENT_EVENT_LOOP to moz.configure

https://reviewboard.mozilla.org/r/40397/#review37091
Comment on attachment 8731135 [details]
MozReview Request: Bug 1256988 - Move USE_FC_FREETYPE to moz.configure

https://reviewboard.mozilla.org/r/40399/#review37093
Attachment #8731135 - Flags: review?(cmanchester) → review+
Comment on attachment 8731136 [details]
MozReview Request: Bug 1256988 - Remove MOZ_TOUCH, nothing is using it

https://reviewboard.mozilla.org/r/40401/#review37095
Attachment #8731136 - Flags: review?(cmanchester) → review+
(In reply to Chris Manchester (:chmanchester) from comment #64)
> So if we have MOZ_WIDGET_TOOLKIT == qt, and WITHOUT_X11, XT_LIBS would
> possibly be non-empty before, but it's never set now when MOZ_WIDGET_TOOLKIT
> == qt.
> 
> I'm trusting this is either impossible, or unimportant.

Looking at the intent declared in the code flow in old-configure.in, XT_LIBS is only supposed to be set when X11 is enabled. My thought is that it's a flaw in the logic that disabling X11 was done after XT_LIBS was set from code that's supposed to be skipped if X11 is disabled. If that happens to break building Qt without X, people interested in that configuration will file a bug.
https://reviewboard.mozilla.org/r/40373/#review37009

> Should this be "startswith" throughout? I guess it probably doesn't matter.

I pondered about it, but I went with the shorter one.
Blocks: 1262019
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.