Port some DEFINES variables to moz.build

RESOLVED FIXED in mozilla30

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Assignee: nobody → ehsan
Attachment #8376646 - Flags: review?(mshal)
Attachment #8376646 - Flags: review?(mh+mozilla)
Attachment #8376646 - Flags: review?(gps)
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'].
Attachment #8376646 - Flags: review?(mshal)
Attachment #8376646 - Flags: review?(mh+mozilla)
Attachment #8376646 - Flags: review?(gps)
Attachment #8376646 - Flags: review-
(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!)
Attachment #8377393 - Flags: review?(mshal)
Attachment #8377393 - Flags: review?(mh+mozilla)
Attachment #8377393 - Flags: review?(gps)
(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.
Attachment #8377393 - Flags: review?(mshal)
Attachment #8377393 - Flags: review?(mh+mozilla)
Attachment #8377393 - Flags: review?(gps)
Attachment #8377393 - Flags: review+
(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/.
https://hg.mozilla.org/mozilla-central/rev/5ecd0339a087
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Posted patch Patch (v3)Splinter Review
Attachment #8376646 - Attachment is obsolete: true
Attachment #8377393 - Attachment is obsolete: true
Attachment #8377755 - Flags: review?(mshal)
Attachment #8377755 - Flags: review?(mh+mozilla)
Attachment #8377755 - Flags: review?(gps)
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']
Attachment #8377755 - Flags: review?(mshal)
Attachment #8377755 - Flags: review?(mh+mozilla)
Attachment #8377755 - Flags: review?(gps)
Attachment #8377755 - Flags: review+
Filed follow-up bug 974255.
https://hg.mozilla.org/mozilla-central/rev/95b4dc85e526
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.