Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Move ASFILES to moz.build

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

3.39 KB, patch
Details | Diff | Splinter Review
2.90 KB, patch
mshal
: review+
Details | Diff | Splinter Review
12.20 KB, patch
mshal
: review-
Details | Diff | Splinter Review
1.91 KB, patch
gps
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → joey

Updated

4 years ago
Blocks: 847009
(Assignee)

Updated

4 years ago
Blocks: 870076
(Assignee)

Comment 1

4 years ago
Created attachment 747106 [details] [diff] [review]
move ASFILES to moz.build.
(Assignee)

Comment 2

4 years ago
Comment on attachment 747106 [details] [diff] [review]
move ASFILES to moz.build.

First pass at bulk moving ASFILES variables over to moz.build.
Attachment #747106 - Flags: review?(mshal)

Comment 3

4 years ago
Are we making moz.build changes to NSPR - I thought not? If we are, the NSPR changes will need breaking out into another patch, for application upstream.
(In reply to Ed Morley [:edmorley UTC+1] from comment #3)
> Are we making moz.build changes to NSPR - I thought not?

No.
(Assignee)

Updated

4 years ago
Blocks: 870366
(Assignee)

Updated

4 years ago
No longer blocks: 870366
(Assignee)

Comment 5

4 years ago
Created attachment 747428 [details] [diff] [review]
move ASFILES to moz.build.
(Assignee)

Updated

4 years ago
Attachment #747106 - Attachment is obsolete: true
Attachment #747106 - Flags: review?(mshal)
(Assignee)

Comment 6

4 years ago
Comment on attachment 747428 [details] [diff] [review]
move ASFILES to moz.build.

removed nsprpub/ files from the patch
Attachment #747428 - Flags: review?(mshal)
(Assignee)

Comment 7

4 years ago
Created attachment 747961 [details] [diff] [review]
mozbuild logic for the ASFILES conversion

Added ASFILES as a passthrough variable.
Changed passthrough init to use a dict+loop to avoid potential copy/paste typos.
Unit test updated to use dynamic value collection in place of hardcoded offsets.  Will reduce the chance of unrelated failures when adding new variables.
Attachment #747961 - Flags: review?(gps)
Attachment #747961 - Flags: feedback?(mshal)
Comment on attachment 747961 [details] [diff] [review]
mozbuild logic for the ASFILES conversion

>From: Joey Armstrong <joey@mozilla.com>
>
>bug 869135: move ASFILES to moz.build.
>
>diff --git a/python/mozbuild/mozbuild/frontend/emitter.py b/python/mozbuild/mozbuild/frontend/emitter.py
>--- a/python/mozbuild/mozbuild/frontend/emitter.py
>+++ b/python/mozbuild/mozbuild/frontend/emitter.py
>@@ -71,24 +71,30 @@ class TreeMetadataEmitter(object):
>             sub.output_path = os.path.join(sandbox['OBJDIR'], path)
>             sub.relpath = path
>             yield sub
> 
>         # Proxy some variables as-is until we have richer classes to represent
>         # them. We should aim to keep this set small because it violates the
>         # desired abstraction of the build definition away from makefiles.
>         passthru = VariablePassthru(sandbox)
>-        if sandbox['MODULE']:
>-            passthru.variables['MODULE'] = sandbox['MODULE']
>-        if sandbox['XPIDL_SOURCES']:
>-            passthru.variables['XPIDLSRCS'] = sandbox['XPIDL_SOURCES']
>-        if sandbox['XPIDL_MODULE']:
>-            passthru.variables['XPIDL_MODULE'] = sandbox['XPIDL_MODULE']
>-        if sandbox['XPIDL_FLAGS']:
>-            passthru.variables['XPIDL_FLAGS'] = sandbox['XPIDL_FLAGS']
>+        varmap = \
>+            {
>+            # Makefile.in    # moz.build

This looks backwards - XPIDL_SOURCES is the moz.build name, and XPIDLSRCS is the Makefile.in name. Maybe just switch the comment?

>+            'ASFILES'      : 'ASFILES',
>+
>+            'MODULE'       : 'MODULE',
>+

nit: Why the extra lines in between some of these variables?

>+            'XPIDL_FLAGS'  : 'XPIDL_FLAGS',
>+            'XPIDL_MODULE' : 'XPIDL_MODULE',
>+            'XPIDL_SOURCES': 'XPIDLSRCS',
>+            }
>+        for mak,moz in varmap.items():
>+            if sandbox[mak]:
>+                passthru.variables[moz] = sandbox[mak]

Similarly here, probably want to swap mak/moz names.

>diff --git a/python/mozbuild/mozbuild/frontend/sandbox_symbols.py b/python/mozbuild/mozbuild/frontend/sandbox_symbols.py
>--- a/python/mozbuild/mozbuild/frontend/sandbox_symbols.py
>+++ b/python/mozbuild/mozbuild/frontend/sandbox_symbols.py
>@@ -161,17 +167,17 @@ VARIABLES = {
>         directory. Files can also be appended to a field to indicate which
>         subdirectory they should be exported to. For example, to export 'foo.h'
>         to the top-level directory, and 'bar.h' to mozilla/dom/, append to
>         EXPORTS like so:
> 
>         EXPORTS += ['foo.h']
>         EXPORTS.mozilla.dom += ['bar.h']
>         """),
>-
>+ 

nit: Extraneous whitespace edit?
Attachment #747961 - Flags: feedback?(mshal) → feedback+

Comment 9

4 years ago
Comment on attachment 747961 [details] [diff] [review]
mozbuild logic for the ASFILES conversion

Review of attachment 747961 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see final revision before r+. That review should largely be a rubber stamp. Just looking for style nit fixes.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +76,5 @@
>          # them. We should aim to keep this set small because it violates the
>          # desired abstraction of the build definition away from makefiles.
>          passthru = VariablePassthru(sandbox)
> +        varmap = \
> +            {

Nit: Avoid line continuations (\) as much as possible. If you want to use curly braces, you should write as:

varmap = {
    'FOO': 'BAR',
}

A popular convention for writing large dicts is to instead use the dict constructor:

varmap = dict(
    ASFILES='ASFILES',
    MODULE='MODULE',
    XPIDL_FLAGS='XPIDL_FLAGS',
)

This saves some redundant typing.

Also, Python style frowns upon vertical alignment of entries. I used to think this was a good idea. But, it just breaks blame and takes more effort to change over time.

@@ +86,5 @@
> +            'XPIDL_FLAGS'  : 'XPIDL_FLAGS',
> +            'XPIDL_MODULE' : 'XPIDL_MODULE',
> +            'XPIDL_SOURCES': 'XPIDLSRCS',
> +            }
> +        for mak,moz in varmap.items():

Nit: space after comma

@@ +88,5 @@
> +            'XPIDL_SOURCES': 'XPIDLSRCS',
> +            }
> +        for mak,moz in varmap.items():
> +            if sandbox[mak]:
> +                passthru.variables[moz] = sandbox[mak]

Variables are backwards.

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
@@ +138,5 @@
>          lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:-1]]
> +
> +        expected = {
> +            'ASFILES': \
> +                [

Don't use line continuations this way.

I'd install flake8 and use it to help validate your style. Perhaps we should just add it to the tree and make it easier to run...

@@ +158,5 @@
> +                ],
> +            }
> +
> +        for var,val in expected.items():
> +            if False:

Just comment out the print line instead.
Attachment #747961 - Flags: review?(gps) → feedback+
(Assignee)

Comment 10

4 years ago
> >+            'ASFILES'      : 'ASFILES',
> >+
> >+            'MODULE'       : 'MODULE',
> >+
> 
> nit: Why the extra lines in between some of these variables?

Just spacing for readability

> >         EXPORTS += ['foo.h']
> >         EXPORTS.mozilla.dom += ['bar.h']
> >         """),
> >-
> >+ 
> 
> nit: Extraneous whitespace edit?

Probably changing a line only containing space into a blank line.
Comment on attachment 747428 [details] [diff] [review]
move ASFILES to moz.build.

>diff --git a/gfx/ycbcr/moz.build b/gfx/ycbcr/moz.build
>--- a/gfx/ycbcr/moz.build
>+++ b/gfx/ycbcr/moz.build
>@@ -8,8 +8,11 @@ MODULE = 'ycbcr'
> 
> EXPORTS += [
>     'chromium_types.h',
>     'ycbcr_to_rgb565.h',
>     'yuv_convert.h',
>     'yuv_row.h',
> ]
> 
>+
>+if 'arm' == CONFIG['OS_TEST'] and CONFIG['HAVE_ARM_NEON']:

I think everywhere else is using "if CONFIG['foo'] == 'bar'" rather than "if 'bar' == CONFIG['foo']"

>+    ASFILES += ['yuv_row_arm.%' % CONFIG['ASM_SUFFIX']]

You need 'yuv_row_arm.%s', right?

> # When building standalone, we need to include mfbt sources, and to declare
>diff --git a/js/src/moz.build b/js/src/moz.build
>--- a/js/src/moz.build
>+++ b/js/src/moz.build
>@@ -79,8 +79,35 @@ EXPORTS.js += [
>     'RequiredDefines.h',
>     'RootingAPI.h',
>     'TemplateLib.h',
>     'Utility.h',
>     'Value.h',
>     'Vector.h',
> ]
> 
>+
>+if CONFIG['ENABLE_METHODJIT'] and '86' in CONFIG['TARGET_CPU']:
>+
>+    if 'x86_64' == CONFIG['TARGET_CPU']:

Similar here, use CONFIG['TARGET_CPU'] == 'x86_64' for consistency.

>+        if CONFIG['_MSC_VER']:
>+            ASFILES += ['TrampolineMasmX64.asm']
>+        if CONFIG['OS_ARCH'] == 'WINNT' and CONFIG['GNU_CC']:
>+            ASFILES += ['TrampolineMingwX64.s']
>+
>+        if CONFIG['SOLARIS_SUNPRO_CXX']:
>+            ASFILES += ['TrampolineSUNWX64.s']
>+
>+    elif CONFIG['SOLARIS_SUNPRO_CXX']:
>+        ASFILES += ['TrampolineSUNWX86.s']
>+
>+

nit: Extra newline here.

>+if CONFIG['ENABLE_METHODJIT'] or CONFIG['ENABLE_ION'] or CONFIG['ENABLE_YARR_JIT']:

I don't think the jswin64.asm is inside this if-block in the Makefile.in. The ifneq (,$(ENABLE_METHODJIT)$(ENABLE_ION)$(ENABLE_YARR_JIT)) goes from line 227 to line 252, but jswin64.asm is added in line 319.

>+    if not CONFIG['_MSC_VER'] or CONFIG['OS_TEST'] != 'x86_64':
>+        pass
>+    elif CONFIG['_MSC_VER'] in ('1400', '1500'):
>+        # for compiler bug (http://support.microsoft.com/kb/982107) for MSVC x64
>+        ASFILES += ['jswin64.asm']

Could this be simplified to say:

if CONFIG['_MSC_VER'] in ('1400', '1500') and CONFIG['OS_TEST'] != 'x86_64':
    ASFILES += ['jswin64.asm']

I don't see what the point of the if-statement with "pass" in the body is.

>+
>+
>+

nit: More extra newlines :)

>+if CONFIG['ENABLE_METHODJIT'] and 'sparc' == CONFIG['TARGET_CPU']:

Again please put CONFIG['TARGET_CPU'] == 'sparc'

>diff --git a/media/libtheora/lib/moz.build b/media/libtheora/lib/moz.build
>--- a/media/libtheora/lib/moz.build
>+++ b/media/libtheora/lib/moz.build
>@@ -1,8 +1,21 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # 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/.
> 
> MODULE = 'theora'
> 
>+theora_asfiles = [
>+    'armbits.s',
>+    'armfrag.s',
>+    'armidct.s',
>+    'armloop.s',
>+    ]
>+
>+def morph_to_as(path):
>+    stem = os.path.splitext(fyl)[0]
>+    return "%s-gnu.%s" % (stem, CONFIG['AS_SUFFIX']);
>+
>+if CONFIG['GNU_AS'] and CONFIG['OS_TEST'] == 'arm':
>+    ASFILES += [morph_to_as(fyl) for fyl in theora_asfiles]

Why not just do:

if CONFIG['GNU_AS'] and CONFIG['OS_TEST'] == 'arm':
    ASFILES += [
         'armbits-gnu.s',
         'armfrag-gnu.s',
         'armidct-gnu.s',
         'armloop-gnu.s',
    ]

I realize what you have is more similar to what the Makefile.in is doing, but there's no reason to do that. The THEORA_ASFILES variable isn't used anywhere else in the Makefile.in, so having that and a function to convert it to the actual values we want doesn't buy us anything. Also in IRC you mentioned the os.path is causing issues on certain platforms, so this might help just avoid it altogether since it isn't necessary here.

>diff --git a/xpcom/reflect/xptcall/src/md/unix/moz.build b/xpcom/reflect/xptcall/src/md/unix/moz.build
>--- a/xpcom/reflect/xptcall/src/md/unix/moz.build
>+++ b/xpcom/reflect/xptcall/src/md/unix/moz.build

This might be a pain to merge with CPPSRCS :/

>+######################################################################
>+# Solaris/Intel
>+######################################################################
>+if CONFIG['OS_ARCH'] == 'SunOS' and not CONFIG['GNU_CC']:
>+    if x86_64 and not CONFIG['GNU_CC']:

You don't need the "not CONFIG['GNU_CXX']" in the two inner conditionals, since it is already in the outer conditional. 

>+        ASFILES += ['xptcstubs_asm_x86_64_solaris_SUNW.s']
>+    # 28817: if Solaris Intel OS, and native compiler, always build optimised.
>+    if x86 == CONFIG['OS_TEST'] and not CONFIG['GNU_CC']:

I think you just want "if x86" here, since you are testing your convenience variable, which has already been defined earlier in the moz.build file? Or, did you want "if CONFIG['OS_TEST'] == 'x86'" (the 'x86' string, not the x86 variable).

>+
>+######################################################################
>+# HPPA
>+######################################################################
>+if CONFIG['OS_ARCH'] == 'HP-UX' and CONFIG['CC'] != gcc:
>+    if CONFIG['OS_TEST'] != 'ia64':
>+        ASFILES += ['xptcstubs_asm_pa32.s', 'xptcinvoke_asm_pa32.s']
>+    else:
>+        ASFILES += ['xptcstubs_asm_ipf32.s', 'xptcinvoke_asm_ipf32.s']

Please swap the if and else so we don't have a double negative:

    if CONFIG['OS_TEST'] == 'ia64':
        ASFILES += ['xptcstubs_asm_ipf32.s', 'xptcinvoke_asm_ipf32.s']
    else:
        ASFILES += ['xptcstubs_asm_pa32.s', 'xptcinvoke_asm_pa32.s']

>+
>+######################################################################
>+# Linux/HPPA/gcc
>+######################################################################
>+if CONFIG['OS_ARCH'] == 'Linux':
>+
>+    if CONFIG['OS_TEST'] in ('filter hppa hppa2.0 hppa1.1'):

'filter' is the make keyword, not an OS. Also we probably want a dict rather than a string:

if CONFIG['OS_TEST'] in ('hppa', 'hppa2.0', 'hppa1.1'):

>+        if CONFIG['GNU_CXX']:
>+            ASFILES += ['xptcstubs_asm_parisc_linux.s', 'xptcinvoke_asm_parisc_linux.s']
>+        else:
>+            raise("Unknown C++ compiler, xptcall assembly will probably be incorrect.")
>+
>+    if CONFIG['OS_TEST'] == 'mips64':
>+        ASFLAGS += [ '-I$(DIST)/include', '-x assembler-with-cpp' ]
>+        ASFILES += ['xptcinvoke_asm_mips64.s', 'xptcstubs_asm_mips64.s']
>+
>+    if CONFIG['OS_TEST'] == 'mips':
>+        ASFLAGS += [ '-I$(DIST)/include', '-x assembler-with-cpp' ]
>+        ASFILES += ['xptcinvoke_asm_mips.s', 'xptcstubs_asm_mips.s']

Are there mips architectures other than 'mips' and 'mips64'? The Makefile uses findstring, so if there is a 'mips32' or something, it would use the 'mips' case (xptcinvoke_asm_mips.s and xptcstubs_asm_mips.s), but this functionality is not present in the moz.build file. I have no idea if that is an issue or not.

>+# Linux/PPC/PPC64
>+if 'Linuxpowerpc' in CONFIG['OS_ARCH'] and 'FreeBSDpowerpc' in CONFIG['OS_TEST']:
>+    if CONFIG['OS_ARCH'][-2:] == '64' and CONFIG['OS_TEST'][-2:] == '64' :
>+        ASFILES += ['xptcinvoke_asm_ppc64_linux.s', 'xptcstubs_asm_ppc64_linux.s']
>+    else:
>+        ASFILES += ['xptcinvoke_asm_ppc_linux.s', 'xptcstubs_asm_ppc_linux.s']

This seems like a strange way to implement these conditionals. What about:

if CONFIG['OS_ARCH'] in ('Linux', 'FreeBSD'):
    if CONFIG['OS_TEST'] == 'powerpc':
        ASFILES += ['xptcinvoke_asm_ppc_linux.s', 'xptcstubs_asm_ppc_linux.s']
    elif CONFIG['OS_TEST'] == 'powerpc64':
        ASFILES += ['xptcinvoke_asm_ppc64_linux.s', 'xptcstubs_asm_ppc64_linux.s']

>+
>+# NetBSD/PPC
>+wanted = ['NetBSDmacppc', 'NetBSDbebox', 'NetBSDofppc', 'NetBSDprep', 'NetBSDamigappc']
>+if CONFIG['OS_ARCH'] in wanted or CONFIG['OS_TEST'] in wanted:
>+    ASFILES += ['xptcinvoke_asm_ppc_netbsd.s', 'xptcstubs_asm_ppc_netbsd.s']

I don't think this matches the Makefile. This is true for any NetBSD, or for any macppc, bebox, etc. I think you want:

if CONFIG['OS_ARCH'] == 'NetBSD' and CONFIG['OS_TEST'] in ('macppc', 'bebox', 'ofppc', 'prep', 'amigappc'):

>+
>+# OpenBSD/PPC
>+if CONFIG['OS_ARCH'] == 'OpenBSDpowerpc' or CONFIG['OS_TEST'] == 'OpenBSDpowerpc':
>+    ASFILES += ['xptcinvoke_asm_ppc_openbsd.s', 'xptcstubs_asm_ppc_openbsd.s']

This is also not correct. The OS_ARCH will be 'OpenBSD' and OS_TEST is 'powerpc'. Or am I misunderstanding these variables?

>+
>+# Darwin/PPC
>+if CONFIG['OS_ARCH'] == 'Darwin' and CONFIG['TARGET_CPU'] == 'powerpc':
>+    ASFILES += ['xptcinvoke_asm_ppc_rhapsody.s', 'xptcstubs_asm_ppc_darwin.s']
>+    CONFIG['AS'] = "%s -c -x assembler-with-cpp" % CONFIG['CC']

I don't believe you can assign to CONFIG['AS'] like this. If assigning to ASFLAGS doesn't work here, you'll need to discuss with gps to find an alternative.

>+
>+
>+######################################################################
>+# SPARC
>+######################################################################
>+
>+# Linux/SPARC
>+if CONFIG['OS_ARCH'] == 'Linux' and CONFIG['OS_TEST'] == 'sparc':

The Makefile.in is using findstring on 'sparc', not equality. I don't know if this matters or not.

>+# NetBSD/SPARC
>+if CONFIG['OS_ARCH'] == 'NetBSDsparc' or CONFIG['OS_TEST'] == 'NetBSDsparc':
>+    ASFILES += ['xptcinvoke_asm_sparc_netbsd.s', 'xptcstubs_asm_sparc_netbsd.s']

Similar to previous conditionals, I believe you want OS_ARCH == 'NetBSD' and OS_TEST == 'sparc'

>+
>+# OpenBSD/SPARC -- ?-should FreeBSDsparc be a comparison string here-?
>+if CONFIG['OS_ARCH'] == 'OpenBSDsparc' or CONFIG['OS_TEST'] == 'OpenBSDsparc':
>+    ASFILES += ['xptcinvoke_asm_sparc_openbsd.s', 'xptcstubs_asm_sparc_openbsd.s']

OS_ARCH == 'OpenBSD' and OS_TEST == 'sparc'

>+
>+# OpenBSD/SPARC64
>+wanted = ['OpenBSDsparc64', 'FreeBSDsparc64']
>+if CONFIG['OS_ARCH'] in wanted or CONFIG['OS_TEST'] in wanted:
>+    ASFILES += ['xptcinvoke_asm_sparc64_openbsd.s', 'xptcstubs_asm_sparc64_openbsd.s']

OS_TEST == 'sparc64' and OS_ARCH in ('OpenBSD', 'FreeBSD')

>+
>+# Solaris/SPARC
>+if CONFIG['OS_ARCH'] == 'SunOS':
>+
>+    if x86_64:
>+        CONFIG['ASFLAGS'] += [ '-xarch=v9' ]

Again I don't think we can modify CONFIG variables in the moz.build file. Just do ASFLAGS += ['-xarch=v9']

>diff --git a/xpcom/reflect/xptcall/src/md/win32/moz.build b/xpcom/reflect/xptcall/src/md/win32/moz.build
>--- a/xpcom/reflect/xptcall/src/md/win32/moz.build
>+++ b/xpcom/reflect/xptcall/src/md/win32/moz.build
>@@ -1,8 +1,13 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # 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/.
> 
> MODULE = 'xpcom'
> 
>+if not CONFIG['GNU_CXX']:
>+    if CONFIG['TARGET_CPU'] == 'x86_64':
>+        ASFILES += ['xptcinvoke_asm_x86_64.asm', 'xptcstubs_asm_x86_64.asm']
>+    if CONFIG['TARGET_CPU'] != 'x86_64':
>+        ASFILES += ['xptcinvoke_asm_x86_64_gnu.s']

I think this conditional doesn't match the Makefile.in. It looks like it should be:

if CONFIG['TARGET_CPU'] != 'x86_64':
    if CONFIG['GNU_CXX']:
        ASFILES += ['xptcinvoke_asm_x86_64_gnu.s']
    else:
        ASFILES += ['xptcinvoke_asm_x86_64.asm', 'xptcstubs_asm_x86_64.asm']

The x86_64 test is the outer condition, and GNU_CXX is in the inner condition. Note also I flipped the GNU_CXX test to do:

if CONFIG['GNU_CXX']:
else:

rather than

if not CONFIG['GNU_CXX']:
else:

to avoid a double-negative in the meaning of the else branch (not not CONFIG['GNU_CXX']).

r- for now because of the mis-converted conditionals, but otherwise it looks close.
Attachment #747428 - Flags: review?(mshal) → review-
(Assignee)

Comment 12

4 years ago
> >+if CONFIG['ENABLE_METHODJIT'] and '86' in CONFIG['TARGET_CPU']:
> >+
> >+    if 'x86_64' == CONFIG['TARGET_CPU']:
> 
> Similar here, use CONFIG['TARGET_CPU'] == 'x86_64' for consistency.

Most of the argument ordering comes from makefile syntax + converting.
May defer this cleanup to a phase-2 bug as you suggested.

> >+    if not CONFIG['_MSC_VER'] or CONFIG['OS_TEST'] != 'x86_64':
> >+        pass
> >+    elif CONFIG['_MSC_VER'] in ('1400', '1500'):
> >+        # for compiler bug (http://support.microsoft.com/kb/982107) for MSVC x64
> >+        ASFILES += ['jswin64.asm']
> 
> Could this be simplified to say:
> 
> if CONFIG['_MSC_VER'] in ('1400', '1500') and CONFIG['OS_TEST'] != 'x86_64':
>     ASFILES += ['jswin64.asm']
> 
> I don't see what the point of the if-statement with "pass" in the body is.

_MSC_VER should be tested for undef before checking values.
Pass was used to break up the conditional for readability and fit +/- 80cpl.


I'll have to look at the rest of the conditionals along with the os.path.join() failure a try job logged on windows.
(Assignee)

Comment 13

4 years ago
> 
> Don't use line continuations this way.
> 
> I'd install flake8 and use it to help validate your style. Perhaps we should
> just add it to the tree and make it easier to run...

[fyi] flake8 will generate a lot of warnings when run bneath python/mozbuild.
(Assignee)

Comment 14

4 years ago
> ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py
> @@ +138,5 @@
> >          lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:-1]]
> > +
> > +        expected = {
> > +            'ASFILES': \
> > +                [
> 
> Don't use line continuations this way.
> 
> I'd install flake8 and use it to help validate your style. Perhaps we should
> just add it to the tree and make it easier to run...

I'll fold these lines together but the list items are a lot harder to read imho.  Also inserting lines will no longer be a simple single line copy & paste.
(Assignee)

Comment 15

4 years ago
Created attachment 748199 [details] [diff] [review]
mozbuild logic for the ASFILES conversion

nit cleanups:
  o whitespace
  o collapse hash assignments, remove line continuation and use dict() to init.
  o fixed mak/moz variables, XPIDL_SOURCES was the only problem value.
  o debugging/test identifier line commented out.

r=mshal carried forward
Attachment #747961 - Attachment is obsolete: true
Attachment #748199 - Flags: review?(gps)
(In reply to Joey Armstrong [:joey] from comment #15)
> r=mshal carried forward

I can only give feedback on this patch, I can't do r.
(Assignee)

Comment 17

4 years ago
ping on the code review.

yes the r=mshal should probably be an f=mshal, just added to note a review.
Comment on attachment 748199 [details] [diff] [review]
mozbuild logic for the ASFILES conversion

Review of attachment 748199 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +83,5 @@
> +            XPIDL_FLAGS='XPIDL_FLAGS',
> +            XPIDL_MODULE='XPIDL_MODULE',
> +            XPIDLSRCS='XPIDL_SOURCES',
> +            )
> +        for mak,moz in varmap.items():

Nit: space "mak, moz" not "mak,moz"

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +56,5 @@
>  #   (type, default_value, docs)
>  #
>  VARIABLES = {
>      # Variables controlling reading of other frontend files.
> +    'ASFILES': (list, [],

Can we rename this AS_FILES? Although, I have a feeling the way we manage libraries, etc will change soon enough, so I'm inclined to not care too much.
Attachment #748199 - Flags: review?(gps) → review+
(Assignee)

Comment 19

4 years ago
Created attachment 749266 [details] [diff] [review]
move ASFILES to moz.build.
(Assignee)

Updated

4 years ago
Attachment #748199 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Comment on attachment 749266 [details] [diff] [review]
move ASFILES to moz.build.

Added whitespace around moz,mak & var,val dictionary iterators.
Bug 871992 opened to address renaming the ASFILES variable.
r=gps carried forward.
(Assignee)

Comment 21

4 years ago
inbound push, mozbuild program edits.
committed changeset 131994:f232f54d3548
https://hg.mozilla.org/integration/mozilla-inbound/rev/f232f54d3548
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/f232f54d3548
Flags: in-testsuite+
(Assignee)

Comment 23

4 years ago
A little late posting but try results for the logic portion:
https://tbpl.mozilla.org/?tree=Try&rev=467b8566e58b
(Assignee)

Comment 24

4 years ago
Created attachment 751078 [details] [diff] [review]
move ASFILES to mozbuild.
(Assignee)

Updated

4 years ago
Attachment #749266 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Comment on attachment 751078 [details] [diff] [review]
move ASFILES to mozbuild.

Rename 'ASFILES' to 'AS_FILES'
Attachment #751078 - Flags: review?(gps)
(Assignee)

Comment 26

4 years ago
Created attachment 751082 [details] [diff] [review]
move ASFILES to mozbuild (logic patch #1)

Initial file conversion so the ASFILE transition to mozbuild can be announced.
This patch depends on the 2nd attachment renaming ASFILES= to AS_FILES=.
Attachment #747428 - Attachment is obsolete: true
Attachment #751082 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
Attachment #751078 - Flags: review?(gps)
(Assignee)

Comment 27

4 years ago
Created attachment 751136 [details] [diff] [review]
move ASFILES to moz.build (config patch 1)

ASFILES conversion already in m-c so keep going.
media/libjpeg/Makefile.in converted so the conversion can be announced.
Attachment #751082 - Attachment is obsolete: true
Attachment #751082 - Flags: review?(mshal)
Attachment #751136 - Flags: review?(mshal)
(Assignee)

Comment 28

4 years ago
ping on the code review
Comment on attachment 751136 [details] [diff] [review]
move ASFILES to moz.build (config patch 1)

I think you forgot to change ASFILES to AS_FILES in the moz.build files, since it doesn't build. Everything else looks fine to me.
Attachment #751136 - Flags: review?(mshal) → review-
(Assignee)

Comment 30

4 years ago
(In reply to Michael Shal [:mshal] from comment #29)
> Comment on attachment 751136 [details] [diff] [review]
> move ASFILES to moz.build (config patch 1)
> 
> I think you forgot to change ASFILES to AS_FILES in the moz.build files,
> since it doesn't build. Everything else looks fine to me.

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

Did you have the 2nd patch attached to this bug applied to your sandbox ?  Syntax expected in m-c right now is 'ASFILES', the conversion.  Another bug is open to handle the translation/rename to another variable.
(Assignee)

Updated

4 years ago
Attachment #751136 - Flags: review- → review?(mshal)
(Assignee)

Comment 31

4 years ago
>>> Did you have the 2nd patch attached to this bug applied to your sandbox ? 

If so ignore/remove it and should be able to build.
Comment on attachment 751136 [details] [diff] [review]
move ASFILES to moz.build (config patch 1)

Ahh sorry, I didn't look closely at the other patch, and thought it was the mozbuild support - I didn't realize it was already there.
Attachment #751136 - Flags: review?(mshal) → review+
(Assignee)

Comment 33

4 years ago
(In reply to Michael Shal [:mshal] from comment #32)
> Comment on attachment 751136 [details] [diff] [review]
> move ASFILES to moz.build (config patch 1)
> 
> Ahh sorry, I didn't look closely at the other patch, and thought it was the
> mozbuild support - I didn't realize it was already there.

np
(Assignee)

Comment 34

4 years ago
Inbound push:
changeset:    132681:7e00fac588dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e00fac588dc
https://hg.mozilla.org/mozilla-central/rev/7e00fac588dc
(Assignee)

Comment 36

4 years ago
Created attachment 755481 [details] [diff] [review]
move ASFILES to mozbuild - file batch #2

Converted, try results pending:
gfx/ycbcr/
media/libtheora/lib/
media/libvpx/
Attachment #755481 - Flags: review?(mshal)
Comment on attachment 755481 [details] [diff] [review]
move ASFILES to mozbuild - file batch #2

>From: Joey Armstrong <joey@mozilla.com>
>
>bug 869135: move ASFILES to moz.build (file batch #2)
>
>diff --git a/gfx/ycbcr/Makefile.in b/gfx/ycbcr/Makefile.in
>--- a/gfx/ycbcr/Makefile.in
>+++ b/gfx/ycbcr/Makefile.in
>@@ -8,17 +8,17 @@ include $(DEPTH)/config/autoconf.mk
> LIBRARY_NAME = ycbcr
> LIBXUL_LIBRARY = 1
> EXPORT_LIBRARY = 1
> 
> DEFINES += -D_IMPL_NS_GFX
> 
> ifeq (arm,$(findstring arm,$(OS_TEST)))
> ifdef HAVE_ARM_NEON
>-ASFILES = yuv_row_arm.$(ASM_SUFFIX) \
>+DISABLED_ASFILES = yuv_row_arm.$(ASM_SUFFIX) \
>           $(NULL)
> endif
> endif

It doesn't look like yuv_row_arm.$(ASM_SUFFIX) made it to gfx/ycbcr/moz.build

>diff --git a/media/libtheora/lib/moz.build b/media/libtheora/lib/moz.build
>--- a/media/libtheora/lib/moz.build
>+++ b/media/libtheora/lib/moz.build
>@@ -1,8 +1,20 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # This Source Code Form is subject to the terms of the Mozilla Public
> # 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/.
> 
> MODULE = 'theora'
> 
>+theora_asfiles = [
>+  'armbits.s',
>+  'armfrag.s',
>+  'armidct.s',
>+  'armloop.s',
>+]

nit: files are indented only 2 spaces instead of 4.

>+
>+## 0:15.59 /media/_Q/mozilla/bugs/869135/config/rules.mk:1123: target `[u'armbits-gnu.s',' doesn't match the target pattern
>+ASFILES += [
>+    # rules.mk requires non-unicode for patterns to match
>+    [ "%s-gnu.%s".decode('utf-8') % (x[0:-2], CONFIG['ASM_SUFFIX']) for x in theora_asfiles]
>+]

Since THEORA_ASFILES isn't needed anywhere else in the Makefile.in, can we just remove it from the moz.build file to avoid the ugly substitution / UTF conversion? ie:

asm_suffix = CONFIG['ASM_SUFFIX']
ASFILES += [
    'armbits-gnu.' + asm_suffix,
    'armfrag-gnu.' + asm_suffix,
    'armidct-gnu.' + asm_suffix,
    'armloop-gnu.' + asm_suffix,
]

>diff --git a/media/libvpx/moz.build b/media/libvpx/moz.build
>--- a/media/libvpx/moz.build
>+++ b/media/libvpx/moz.build
>+vpx_asfiles = None
>+if CONFIG['VPX_ARM_ASM']:
>+    # Files which depend on asm_com_offsets.asm
>+    vpx_asm_com_offsets_srcs = [
>+        'vp8_vpxyv12_copy_y_neon.asm',
>+        'vp8_vpxyv12_copyframe_func_neon.asm',
>+        'vp8_vpxyv12_extendframeborders_neon.asm',
>+    ]  

nit: whitespace at the end of the ']' line

>+    vpx_asfiles += [
>+       'bilinearfilter_v6.asm',
>+        'copymem16x16_v6.asm',

nit: entries not aligned
Attachment #755481 - Flags: review?(mshal) → feedback+
(Assignee)

Comment 38

4 years ago
(In reply to Michael Shal [:mshal] from comment #37)
> Comment on attachment 755481 [details] [diff] [review]
> move ASFILES to mozbuild - file batch #2
> >
> >diff --git a/media/libtheora/lib/moz.build b/media/libtheora/lib/moz.build
> >--- a/media/libtheora/lib/moz.build
> >+++ b/media/libtheora/lib/moz.build
> >@@ -1,8 +1,20 @@
> > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> > # vim: set filetype=python:
> > # This Source Code Form is subject to the terms of the Mozilla Public
> > # 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/.
> > 
> > MODULE = 'theora'
> > 
> >+theora_asfiles = [
> >+  'armbits.s',
> >+  'armfrag.s',
> >+  'armidct.s',
> >+  'armloop.s',
> >+]
> 
> nit: files are indented only 2 spaces instead of 4.

ah these conversions are loads of fun

 
> >+
> >+## 0:15.59 /media/_Q/mozilla/bugs/869135/config/rules.mk:1123: target `[u'armbits-gnu.s',' doesn't match the target pattern
> >+ASFILES += [
> >+    # rules.mk requires non-unicode for patterns to match
> >+    [ "%s-gnu.%s".decode('utf-8') % (x[0:-2], CONFIG['ASM_SUFFIX']) for x in theora_asfiles]
> >+]
> 
> Since THEORA_ASFILES isn't needed anywhere else in the Makefile.in, can we
> just remove it from the moz.build file to avoid the ugly substitution / UTF
> conversion? ie:
> 
> asm_suffix = CONFIG['ASM_SUFFIX']
> ASFILES += [
>     'armbits-gnu.' + asm_suffix,
>     'armfrag-gnu.' + asm_suffix,
>     'armidct-gnu.' + asm_suffix,
>     'armloop-gnu.' + asm_suffix,
> ]

Hmmm hardcoding filenames would be simpler but we would lose context in the assignment.  '*-gnu*' files in this case are derived and that detail was documented in the makefile with THEORA_FILES= and $(patsubst ).

Not sure if we striving for simplicity or clarity within the config files ?
(Assignee)

Comment 39

4 years ago
Created attachment 755992 [details] [diff] [review]
move ASFILES to mozbuild - file batch #2

nit fixes.
yuv_row_arm was lost in a revert, subsequent conversion runs short-circuited because Makefile.in had been modified.  Re-added the file.

Ignore context for now and hardcode the assembly file list to keep the conversions moving.  Filenames share a common prefix so should be easy enough to match up manually when needed.
Attachment #755481 - Attachment is obsolete: true
Attachment #755992 - Flags: review?(mshal)
(Assignee)

Comment 40

4 years ago
Comment on attachment 755992 [details] [diff] [review]
move ASFILES to mozbuild - file batch #2

Cancel review, inlined -gnu.s files not matching makefile pattern rules again.
Attachment #755992 - Flags: review?(mshal)
(Assignee)

Updated

4 years ago
See Also: → bug 886689

Comment 41

4 years ago
Created attachment 768921 [details] [diff] [review]
move ASFILES to moz.build (file batch #2)

Updated

4 years ago
Attachment #755992 - Attachment is obsolete: true

Updated

4 years ago
Assignee: joey → jarmstrong
(Assignee)

Comment 42

4 years ago
Comment on attachment 768921 [details] [diff] [review]
move ASFILES to moz.build (file batch #2)

Earlier nits cleaned up.
Repaired a few CONFIG[] conditional problems including ('arm' != 'armv7').

Try build results:
  http://tbpl.mozilla.org/?tree=Try&rev=5affa6beb1b7
  http://tbpl.mozilla.org/?tree=Try&rev=ced30d97f808
    * Lingering unagi failure, undefined ALOGE ref appears to now be defined in the librecovery git repository so the missing symbol problem should resolve its-self.

Try test results are on the way.
Attachment #768921 - Flags: review?(mshal)
(Assignee)

Comment 43

4 years ago
The unagi comment was not for this bug -> bug 880773/SSRCS



(In reply to Joey Armstrong [:joey] from comment #42)
> Comment on attachment 768921 [details] [diff] [review]
> move ASFILES to moz.build (file batch #2)
> 
> Earlier nits cleaned up.
> Repaired a few CONFIG[] conditional problems including ('arm' != 'armv7').
> 
> Try build results:
>   http://tbpl.mozilla.org/?tree=Try&rev=5affa6beb1b7
>   http://tbpl.mozilla.org/?tree=Try&rev=ced30d97f808
>     * Lingering unagi failure, undefined ALOGE ref appears to now be defined
> in the librecovery git repository so the missing symbol problem should
> resolve its-self.
> 
> Try test results are on the way.
Comment on attachment 768921 [details] [diff] [review]
move ASFILES to moz.build (file batch #2)

media/libtheora/lib looks good - I can r+ a patch with just that. However, I don't think we should move over media/libvpx just yet because there are too many custom rules in the Makefile that depend on ASFILES variables. We won't be able to get rid of the DISABLED_* variables until those rules are converted somehow, so I think we should convert the rules first and then move ASFILES. Otherwise we'll be stuck with duplicate data in the Makefile.in/moz.build file while those rules linger.

I believe what we'll need to do is:

1) Generate asm_enc_offsets.asm in the export phase, and remove the custom rules that add dependencies to it (I'm assuming the asm compilation rules automatically pick up include dependencies?)

2) Make VPX_ASFILES a top-level variable (or maybe ARM_ASFILES?), similar to ASFILES. The backend can generate rules to do the ARM -> GNU syntax conversion before compiling.

With this we can then get rid of the local variables like vpx_asfiles and vpx_asm_enc_offsets_srcs - the moz.build file will just append to ASFILES and VPX_ASFILES.

Thoughts / concerns / corrections?
Attachment #768921 - Flags: review?(mshal) → review-
(Assignee)

Comment 45

4 years ago
Created attachment 799010 [details] [diff] [review]
move ASFILES to mozbuild.
(Assignee)

Updated

4 years ago
Assignee: jarmstrong → joey
Status: NEW → ASSIGNED
(Assignee)

Comment 46

4 years ago
Comment on attachment 799010 [details] [diff] [review]
move ASFILES to mozbuild.

slice media/libtheora/lib from the original patch to pickup an r+ {comment #44} for landing.
Try results: http://tbpl.mozilla.org/?tree=Try&rev=412831d64168
Attachment #799010 - Flags: review?(gps)
Comment on attachment 799010 [details] [diff] [review]
move ASFILES to mozbuild.

Review of attachment 799010 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libtheora/lib/moz.build
@@ +13,5 @@
> +    ASFILES += [
> +        'armbits-gnu.%s' % (asm_suffix),
> +        'armfrag-gnu.%s' % (asm_suffix),
> +        'armidct-gnu.%s' % (asm_suffix),
> +        'armloop-gnu.%s' % (asm_suffix),

Nit: No need for parens if there's a single argument.
Attachment #799010 - Flags: review?(gps) → review+
(Assignee)

Comment 48

4 years ago
(In reply to Gregory Szorc [:gps] from comment #47)
> Comment on attachment 799010 [details] [diff] [review]
> move ASFILES to mozbuild.
> 
> Review of attachment 799010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libtheora/lib/moz.build
> @@ +13,5 @@
> > +    ASFILES += [
> > +        'armbits-gnu.%s' % (asm_suffix),
> > +        'armfrag-gnu.%s' % (asm_suffix),
> > +        'armidct-gnu.%s' % (asm_suffix),
> > +        'armloop-gnu.%s' % (asm_suffix),
> 
> Nit: No need for parens if there's a single argument.

That will contribute to inconsistent usage.  If parens are always added the next person to come along simply needs to insert extra arguments, parens are already there.
(Assignee)

Comment 49

4 years ago
Inbound push for media/libtheora/lib:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09861acec08e
https://hg.mozilla.org/mozilla-central/rev/09861acec08e
instead OS_TEST I guess should be CPU_ARCH used or
if CONFIG['OS_TEST'].startswith('arm')
Depends on: 932178
(Assignee)

Updated

4 years ago
Assignee: joey → nobody

Comment 52

3 years ago
This can't be in "ASSIGNED" status with nobody as the assignee.
Status: ASSIGNED → NEW
This looks to be done:
https://dxr.mozilla.org/mozilla-central/search?q=ASFILES+path%3AMakefile.in&redirect=false&case=false
Assignee: nobody → joey
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.