Move ASFLAGS to moz.build

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Depends on: 1162846
Depends on: 1162847
Posted patch Move ASFLAGS to moz.build (obsolete) — Splinter Review
Note the removal of -I$(DIST)/include in xpcom/reflect/... which shouldn't matter because that's already in LOCAL_INCLUDES, which is passed to the assembler invocations for .S files, which are those that need preprocessing. Although, two of the .s files for SunOS do have a #include, but it's for a file that's in the current directory, not $(DIST)/include.
Attachment #8603172 - Flags: review?(mshal)
Blocks: 1162852
Comment on attachment 8603172 [details] [diff] [review]
Move ASFLAGS to moz.build

Err, subtle problem, some Makefiles are not doing +=, but =... and the build breaks for the latter ones.
Attachment #8603172 - Flags: review?(mshal)
Attachment #8603213 - Flags: review?(mshal)
Attachment #8603172 - Attachment is obsolete: true
Comment on attachment 8603213 [details] [diff] [review]
Move ASFLAGS to moz.build

>diff --git a/media/libtheora/moz.build b/media/libtheora/moz.build
>index 30ac65f..031a5ee 100644
>--- a/media/libtheora/moz.build
>+++ b/media/libtheora/moz.build
>+        # These flags are a lie; they're just used to enable the requisite
>+        # opcodes; actual arch detection is done at runtime.
>+        ASFLAGS += [
>+            '-march=armv7-a',
>+            '-mfpu=neon',
>+        ]

I think these ASFLAGS should be close to the GENERATED_SOURCES in this moz.build file, since those are the asm files. We could probably just move GENERATED_SOURCES down here where you added ASFLAGS, since it looks like those conditions are redundant. Eg:

from:
if CONFIG['CPU_ARCH'] == 'arm' and CONFIG['GNU_AS']:
    GENERATED_FILES ...

if CONFIG['GNU_AS']:
    if 'arm' in CONFIG['OS_TEST']:
        ASFLAGS ...

to:
if CONFIG['GNU_AS']:
    if 'arm' in CONFIG['OS_TEST']:
        GENERATED_FILES ...
        ASFLAGS ...

>diff --git a/media/libvpx/moz.build b/media/libvpx/moz.build
>+ASFLAGS += [
>+    '-I.',
>+    '-I%s/media/libvpx/' % TOPSRCDIR,
>+    '-I%s/media/libvpx/vpx_ports/' % TOPSRCDIR,
>+]
>+    ASFLAGS += [
>+        '-D__ANDROID__'
>+    ]

Can we just use LOCAL_INCLUDES / DEFINES instead of -I / -D? Or would that break the .c files?

Everything else looks fine as far as I can tell. I'm glad this moves the responsibility of whether or not to pass in -c out of the Makefile/moz.build...
Attachment #8603213 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #4)
> Can we just use LOCAL_INCLUDES / DEFINES instead of -I / -D? Or would that
> break the .c files?

Unfortunately, we don't use LOCAL_INCLUDES / DEFINES in the assembler command lines. We can certainly improve that, but I'd rather avoid the rabbit hole for now.
https://hg.mozilla.org/mozilla-central/rev/5583ce8972c5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.