Comment on attachment 8376646 [details] [diff] [review] Port some DEFINES variables to moz.build Review of attachment 8376646 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/atk/Makefile.in @@ -12,5 @@ > ifdef MOZ_ENABLE_DBUS > CXXFLAGS += $(MOZ_DBUS_CFLAGS) > endif > - > -ifneq ($(A11Y_LOG),0) See bug 935548 ::: dom/apps/src/moz.build @@ +46,5 @@ > '/js/xpconnect/wrappers', > ] > > +if CONFIG['MOZ_DEBUG']: > + DEFINES['MOZ_DEBUG'] = 1 Better to fix Webapps.jsm to use DEBUG ::: toolkit/mozapps/extensions/Makefile.in @@ -2,5 @@ > -# License, v. 2.0. If a copy of the MPL was not distributed with this > -# file, You can obtain one at http://mozilla.org/MPL/2.0/. > - > -# Additional debugging info is exposed by setting the MOZ_EM_DEBUG > -# environment variable when building. I don't think this will still work

(In reply to :Ms2ger from comment #2) > Comment on attachment 8376646 [details] [diff] [review] > Port some DEFINES variables to moz.build > > Review of attachment 8376646 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/atk/Makefile.in > @@ -12,5 @@ > > ifdef MOZ_ENABLE_DBUS > > CXXFLAGS += $(MOZ_DBUS_CFLAGS) > > endif > > - > > -ifneq ($(A11Y_LOG),0) > > See bug 935548 Doesn't have to block bug 928196. > ::: dom/apps/src/moz.build > @@ +46,5 @@ > > '/js/xpconnect/wrappers', > > ] > > > > +if CONFIG['MOZ_DEBUG']: > > + DEFINES['MOZ_DEBUG'] = 1 > > Better to fix Webapps.jsm to use DEBUG Please file a follow-up. > ::: toolkit/mozapps/extensions/Makefile.in > @@ -2,5 @@ > > -# License, v. 2.0. If a copy of the MPL was not distributed with this > > -# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > - > > -# Additional debugging info is exposed by setting the MOZ_EM_DEBUG > > -# environment variable when building. > > I don't think this will still work Because of the environment variable bit? If yes I'll land the patch without this change.

Comment on attachment 8376646 [details] [diff] [review] Port some DEFINES variables to moz.build Review of attachment 8376646 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/atk/Makefile.in @@ -12,5 @@ > ifdef MOZ_ENABLE_DBUS > CXXFLAGS += $(MOZ_DBUS_CFLAGS) > endif > - > -ifneq ($(A11Y_LOG),0) Please fix bug 935548 instead. ::: dom/apps/src/moz.build @@ +46,5 @@ > '/js/xpconnect/wrappers', > ] > > +if CONFIG['MOZ_DEBUG']: > + DEFINES['MOZ_DEBUG'] = 1 I agree with Ms2ger, and it's a one-liner anyways. Better to do it now than to leave it there forever (I doubt someone would actually care to fix the followup you'd file). ::: gfx/cairo/libpixman/src/Makefile.in @@ +65,5 @@ > endif > > > ifdef USE_MMX > CSRCS += pixman-mmx.c I'd rather move the CSRCS at the same time. ::: toolkit/mozapps/extensions/Makefile.in @@ -5,5 @@ > -# Additional debugging info is exposed by setting the MOZ_EM_DEBUG > -# environment variable when building. > -ifneq (,$(MOZ_EM_DEBUG)) > -DEFINES += -DMOZ_EM_DEBUG=1 > -endif Add AC_SUBST(MOZ_EM_DEBUG) somewhere in configure.in. And set MOZ_EM_DEBUG=1 on MOZ_DEBUG builds. ::: toolkit/mozapps/extensions/moz.build @@ +52,5 @@ > # out of sync. > DEFINES['MOZ_EXTENSIONS_DB_SCHEMA'] = 16 > > # Additional debugging info is exposed in debug builds > +if CONFIG['MOZ_DEBUG'] or CONFIG['MOZ_EM_DEBUG']: Then you can make this if CONFIG['MOZ_EM_DEBUG'].

(In reply to Mike Hommey [:glandium] from comment #4) > Comment on attachment 8376646 [details] [diff] [review] > Port some DEFINES variables to moz.build > > Review of attachment 8376646 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/atk/Makefile.in > @@ -12,5 @@ > > ifdef MOZ_ENABLE_DBUS > > CXXFLAGS += $(MOZ_DBUS_CFLAGS) > > endif > > - > > -ifneq ($(A11Y_LOG),0) > > Please fix bug 935548 instead. Again, it doesn't have to block bug 928196. But if you want to make it so, I'll remove these changes from my patch. I am not the right person to fix bug 935548. > ::: dom/apps/src/moz.build > @@ +46,5 @@ > > '/js/xpconnect/wrappers', > > ] > > > > +if CONFIG['MOZ_DEBUG']: > > + DEFINES['MOZ_DEBUG'] = 1 > > I agree with Ms2ger, and it's a one-liner anyways. Better to do it now than > to leave it there forever (I doubt someone would actually care to fix the > followup you'd file). Fine. FWIW when I ask someone to file something as a follow-up, I'll fix it. If I don't want to fix something I'll explicitly say so (see above!)

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #5) > (In reply to Mike Hommey [:glandium] from comment #4) > > Comment on attachment 8376646 [details] [diff] [review] > > Port some DEFINES variables to moz.build > > > > Review of attachment 8376646 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: accessible/src/atk/Makefile.in > > @@ -12,5 @@ > > > ifdef MOZ_ENABLE_DBUS > > > CXXFLAGS += $(MOZ_DBUS_CFLAGS) > > > endif > > > - > > > -ifneq ($(A11Y_LOG),0) > > > > Please fix bug 935548 instead. > > Again, it doesn't have to block bug 928196. But if you want to make it so, > I'll remove these changes from my patch. I am not the right person to fix > bug 935548. Well, the thing is either you fix bug 935548, or your changes break this A11Y_LOG thing, because CONFIG['A11Y_LOG'] doesn't exist. We should probably error out on unknown variables btw... That being said, anyone can fix bug 935548. It's just a matter of moving A11Y_LOG to configure, and make it an AC_DEFINE.

(In reply to Mike Hommey [:glandium] from comment #7) > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, > emailapocalypse) from comment #5) > > (In reply to Mike Hommey [:glandium] from comment #4) > > > Comment on attachment 8376646 [details] [diff] [review] > > > Port some DEFINES variables to moz.build > > > > > > Review of attachment 8376646 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: accessible/src/atk/Makefile.in > > > @@ -12,5 @@ > > > > ifdef MOZ_ENABLE_DBUS > > > > CXXFLAGS += $(MOZ_DBUS_CFLAGS) > > > > endif > > > > - > > > > -ifneq ($(A11Y_LOG),0) > > > > > > Please fix bug 935548 instead. > > > > Again, it doesn't have to block bug 928196. But if you want to make it so, > > I'll remove these changes from my patch. I am not the right person to fix > > bug 935548. > > Well, the thing is either you fix bug 935548, or your changes break this > A11Y_LOG thing, because CONFIG['A11Y_LOG'] doesn't exist. Oh, I misunderstood what you mean here, sorry. This is a good point. > We should probably > error out on unknown variables btw... That is a great idea. > That being said, anyone can fix bug 935548. It's just a matter of moving > A11Y_LOG to configure, and make it an AC_DEFINE. Hmm, well, attachment 828855 [details] [diff] [review] definitely tries to do a lot more. Why is that?

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #8) > Hmm, well, attachment 828855 [details] [diff] [review] definitely tries to > do a lot more. Why is that? For the same kind of reason you flattened intl/.

I got Talos regressions like this: Regression: Mozilla-Inbound-Non-PGO - Tab Animation Test - WINNT 5.1 (ix) - 7.06% increase ------------------------------------------------------------------------------------------ Previous: avg 5.043 stddev 0.021 of 12 runs up to revision 6a647f7bafe2 New : avg 5.399 stddev 0.053 of 12 runs since revision 5ecd0339a087 Change : +0.356 (7.06% / z=17.287) Graph : http://mzl.la/1geoNXn Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6a647f7bafe2&tochange=5ecd0339a087 Changesets: * http://hg.mozilla.org/integration/mozilla-inbound/rev/5ecd0339a087 : Ehsan Akhgari <ehsan@mozilla.com> - Bug 973143 - Move some variables to moz.build; r=glandium : http://bugzilla.mozilla.org/show_bug.cgi?id=973143 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=973143 - Port some DEFINES variables to moz.build _______________________________________________ dev-tree-management mailing list dev-tree-management@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-tree-management So I backed out the patch for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/22693bfef4d8

Figured out the problem, these USE_foobar variables were defined in the Makefile.in, so we need to port them over as well.

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #13) > Figured out the problem, these USE_foobar variables were defined in the > Makefile.in, so we need to port them over as well. Filed bug 974060 to make this a build time error.

Comment on attachment 8377755 [details] [diff] [review] Patch (v3) Review of attachment 8377755 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/cairo/libpixman/src/Makefile.in @@ +22,3 @@ > endif > ifeq (arm,$(findstring arm,$(OS_TEST))) > USE_ARM_SIMD_MSVC=1 You can remove this while you're here, it's not used. @@ +61,1 @@ > ARM_NEON_CFLAGS = -mfpu=neon Might as well move that in the ifdef HAVE_ARM_NEON and remove USE_ARM_NEON_GCC ::: gfx/cairo/libpixman/src/moz.build @@ +75,5 @@ > DEFINES['PACKAGE'] = 'mozpixman' > > DEFINES['_USE_MATH_DEFINES'] = True > + > +if CONFIG['_MSC_VER'] and CONFIG['OS_TEST'].find('86') != -1 and \ '86' in CONFIG['OS_TEST'] @@ +76,5 @@ > > DEFINES['_USE_MATH_DEFINES'] = True > + > +if CONFIG['_MSC_VER'] and CONFIG['OS_TEST'].find('86') != -1 and \ > + CONFIG['OS_TEST'].find('64') == -1: '64' not in CONFIG['OS_TEST'] @@ +78,5 @@ > + > +if CONFIG['_MSC_VER'] and CONFIG['OS_TEST'].find('86') != -1 and \ > + CONFIG['OS_TEST'].find('64') == -1: > + use_mmx = True > +if CONFIG['GNU_CC'] and CONFIG['OS_TEST'].find('86') != -1: '86' in CONFIG['OS_TEST'] Note this enables mmx on x86_64 gcc, while we don't do that on msvc. I don't think it makes sense to enable mmx when there is sse2. Followup bug? @@ +81,5 @@ > + use_mmx = True > +if CONFIG['GNU_CC'] and CONFIG['OS_TEST'].find('86') != -1: > + use_mmx = True > + > +if CONFIG['_MSC_VER'] and CONFIG['OS_TEST'].find('86') != -1: '86' in CONFIG['OS_TEST'] @@ +84,5 @@ > + > +if CONFIG['_MSC_VER'] and CONFIG['OS_TEST'].find('86') != -1: > + use_sse2 = True > +if CONFIG['GNU_CC'] and CONFIG['OS_TEST'].find('86') != -1 and \ > + (CONFIG['OS_TEST'].find('64') != -1 or CONFIG['HAVE_GCC_ALIGN_ARG_POINTER']): '86' in CONFIG['OS_TEST'] '64' in CONFIG['OS_TEST'] Note I think those tests above would be clearer if they were grouped by OS_TEST tests. Like: if '86' in CONFIG['OS_TEST']: if '64' in CONFIG['OS_TEST']: stuff for x86_64 depending GNU_CC or _MSC_VER else: ... @@ +87,5 @@ > +if CONFIG['GNU_CC'] and CONFIG['OS_TEST'].find('86') != -1 and \ > + (CONFIG['OS_TEST'].find('64') != -1 or CONFIG['HAVE_GCC_ALIGN_ARG_POINTER']): > + use_sse2 = True > + > +if CONFIG['GNU_CC'] and CONFIG['OS_TEST'].find('ppc') != -1: 'ppc' in CONFIG['OS_TEST'] @@ +94,5 @@ > +# Apple's arm assembler doesn't support the same syntax as > +# the standard GNU assembler, so use the C fallback paths for now. > +# This may be fixable if clang's ARM/iOS assembler improves into a > +# viable solution in the future. > +if CONFIG['OS_TEST'].find('arm') != -1 and CONFIG['OS_ARCH'] != 'Darwin': 'arm' in CONFIG['OS_TEST']

Filed follow-up bug 974255.