880773 - move SSRCS to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Joey Armstrong [:joey]]

Attachment: move SSRCS to moz.build (logic).

Comment 2

[Joey Armstrong [:joey]]

Comment on attachment 759863 [details] [diff] [review]
move SSRCS to moz.build (logic).

Add SSRCS as a passthrough variable

Comment 3

[Joey Armstrong [:joey]]

Attachment: move SSRCS to moz.build (logic).

Comment 4

[Joey Armstrong [:joey]]

Comment on attachment 760369 [details] [diff] [review]
move SSRCS to moz.build (logic).

rebase, r=ted carried forward.

Comment 5

[Joey Armstrong [:joey]]

Comment on attachment 760369 [details] [diff] [review]
move SSRCS to moz.build (logic).

Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b15bb52a236
committed changeset 134505:1b15bb52a236

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b15bb52a236

Comment 7

[Joey Armstrong [:joey]]

Attachment: move SSRCS to moz.build (file batch #1).

Comment 8

[jarmstrong]

Attachment: move SSRCS to mozbuild (file batch #1)

gfx/skia - Triple negative Makefile.in conditional for INTEL_ARCHITECTURE should be inverted when memset.arm.S is assigned.

Comment 9

[jarmstrong]

Comment on attachment 767365 [details] [diff] [review]
move SSRCS to mozbuild (file batch #1)

https://tbpl.mozilla.org/?tree=Try&rev=3f0bd084ae32

USE_ARM_NEON_GCC and USE_ARM_SIMD_GCC are both local defines.  Test HAVE_ARM_NEON and HAVE_ARM_SIMD instead.

Comment 10

[jarmstrong]

Attachment: move SSRCS to mozbuild (file batch #1).

Comment 11

[jarmstrong]

Comment on attachment 768535 [details] [diff] [review]
move SSRCS to mozbuild (file batch #1).

Try results: http://tbpl.mozilla.org/?tree=Try&rev=2f51dbe8607d
Bg failure (x2) is known: bug 848604

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 768535 [details] [diff] [review]
move SSRCS to mozbuild (file batch #1).

>diff --git a/gfx/cairo/libpixman/src/moz.build b/gfx/cairo/libpixman/src/moz.build
>--- a/gfx/cairo/libpixman/src/moz.build
>+++ b/gfx/cairo/libpixman/src/moz.build
>@@ -6,8 +6,19 @@
> 
> MODULE = 'libpixman'
> 
> EXPORTS += [
>     'pixman-version.h',
>     'pixman.h',
> ]
> 
>+if CONFIG['OS_ARCH'] != 'Darwin' and CONFIG['GNU_CC']:
>+    if CONFIG['HAVE_ARM_NEON']:
>+        SSRCS += [
>+            'pixman-arm-neon-asm-bilinear.S',
>+            'pixman-arm-neon-asm.S',
>+        ]
>+    if CONFIG['HAVE_ARM_SIMD']:
>+        SSRCS += [
>+            'pixman-arm-simd-asm-scaled.S',
>+            'pixman-arm-simd-asm.S',
>+        ]

The Makefile.in also has a check for 'arm' in OS_TEST before setting these. Do we not need this because HAVE_ARM_NEON and HAVE_ARM_SIMD only get defined if we're on arm? (That would make sense, so I wonder why the Makefile has the check...)

Also recommend moving this comment over from the Makefile.in:

# 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.

>diff --git a/gfx/skia/moz.build b/gfx/skia/moz.build
>--- a/gfx/skia/moz.build
>+++ b/gfx/skia/moz.build
>@@ -469,10 +469,15 @@ CPP_SOURCES += [
>     'SkTypeface.cpp',
>     'SkTypefaceCache.cpp',
>     'SkUnPreMultiply.cpp',
>     'SkUtils.cpp',
>     'SkWriter32.cpp',
>     'SkXfermode.cpp',
> ]
> 
>+if not CONFIG['INTEL_ARCHITECTURE'] and CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC'] == '1':
>+    SSRCS += [
>+        'memset.arm.S',
>+    ]
>+

You can just use CONFIG['GNU_CC'] rather than explicitly checking that it == '1'.

Comment 13

[Joey Armstrong [:joey]]

Comment on attachment 768535 [details] [diff] [review]
move SSRCS to mozbuild (file batch #1).

Try results: https://tbpl.mozilla.org/?tree=Try&rev=2f51dbe8607d

"GAIA_APPDIRS is not defined": osx-1.6-opt, Fedora-opt

Comment 14

[Joey Armstrong [:joey]]

(In reply to Michael Shal [:mshal] from comment #12)
> Comment on attachment 768535 [details] [diff] [review]
> move SSRCS to mozbuild (file batch #1).
> 
> >diff --git a/gfx/cairo/libpixman/src/moz.build b/gfx/cairo/libpixman/src/moz.build
> >--- a/gfx/cairo/libpixman/src/moz.build
> >+++ b/gfx/cairo/libpixman/src/moz.build
> >@@ -6,8 +6,19 @@
> > 
> > MODULE = 'libpixman'
> > 
> > EXPORTS += [
> >     'pixman-version.h',
> >     'pixman.h',
> > ]
> > 
> >+if CONFIG['OS_ARCH'] != 'Darwin' and CONFIG['GNU_CC']:
> >+    if CONFIG['HAVE_ARM_NEON']:
> >+        SSRCS += [
> >+            'pixman-arm-neon-asm-bilinear.S',
> >+            'pixman-arm-neon-asm.S',
> >+        ]
> >+    if CONFIG['HAVE_ARM_SIMD']:
> >+        SSRCS += [
> >+            'pixman-arm-simd-asm-scaled.S',
> >+            'pixman-arm-simd-asm.S',
> >+        ]
> 
> The Makefile.in also has a check for 'arm' in OS_TEST before setting these.
> Do we not need this because HAVE_ARM_NEON and HAVE_ARM_SIMD only get defined
> if we're on arm? (That would make sense, so I wonder why the Makefile has
> the check...)

[fyi] The OS_TEST check is not reliable according to bug 886689
http://bugzilla.mozilla.org/show_bug.cgi?id=886689

To answer your question yes, configure is explicitly testing CPU_ARCH == 'arm' so testing for it would be redundant.


> Also recommend moving this comment over from the Makefile.in:
> 
> # 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.

Will do

> >diff --git a/gfx/skia/moz.build b/gfx/skia/moz.build
> >--- a/gfx/skia/moz.build
> >+++ b/gfx/skia/moz.build
> >+if not CONFIG['INTEL_ARCHITECTURE'] and CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_CC'] == '1':
> >+    SSRCS += [
> >+        'memset.arm.S',
> >+    ]
> >+
> 
> You can just use CONFIG['GNU_CC'] rather than explicitly checking that it ==
> '1'.

Literal translation from the conversion, will also crush this down to a boolean test.

Comment 15

[Joey Armstrong [:joey]]

Attachment: move SSRCS to mozbuild (file batch #1)

Same patch as last time but added the apple comment block.
Crushed if x==1 conditional to if x

Comment 16

[Michael Shal [:mshal]]

Comment on attachment 769776 [details] [diff] [review]
move SSRCS to mozbuild (file batch #1)

Looks good to me now!

Comment 17

[Joey Armstrong [:joey]]

Comment on attachment 769776 [details] [diff] [review]
move SSRCS to mozbuild (file batch #1)

Inbound push
https://hg.mozilla.org/integration/mozilla-inbound/rev/024f4fcbdfdb
committed changeset 137061:024f4fcbdfdb

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/024f4fcbdfdb

Comment 19

[Joey Armstrong [:joey]]

Attachment: Cleanup/final patch - remove DISABLED_SSRCS.

Comment 20

[Joey Armstrong [:joey]]

Comment on attachment 775958 [details] [diff] [review]
Cleanup/final patch - remove DISABLED_SSRCS.

Remove DISABLED_ tokens and SSRCS can be crossed off the list.

Comment 21

[Michael Shal [:mshal]]

Comment on attachment 775958 [details] [diff] [review]
Cleanup/final patch - remove DISABLED_SSRCS.

Looks good!

Comment 22

[Joey Armstrong [:joey]]

Comment on attachment 775958 [details] [diff] [review]
Cleanup/final patch - remove DISABLED_SSRCS.

Inbound push http://hg.mozilla.org/integration/mozilla-inbound/rev/3d33faf4d81f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d33faf4d81f