Comment on attachment 747106 [details] [diff] [review] move ASFILES to moz.build. First pass at bulk moving ASFILES variables over to moz.build.

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.

Comment on attachment 747428 [details] [diff] [review] move ASFILES to moz.build. removed nsprpub/ files from the patch

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.

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?

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.

> >+ '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.

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

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

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

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

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

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.

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.

inbound push, mozbuild program edits. committed changeset 131994:f232f54d3548 https://hg.mozilla.org/integration/mozilla-inbound/rev/f232f54d3548

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

Comment on attachment 751078 [details] [diff] [review] move ASFILES to mozbuild. Rename 'ASFILES' to 'AS_FILES'

Initial file conversion so the ASFILE transition to mozbuild can be announced. This patch depends on the 2nd attachment renaming ASFILES= to AS_FILES=.

ASFILES conversion already in m-c so keep going. media/libjpeg/Makefile.in converted so the conversion can be announced.

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.

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

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

(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

Inbound push: changeset: 132681:7e00fac588dc https://hg.mozilla.org/integration/mozilla-inbound/rev/7e00fac588dc

Converted, try results pending: gfx/ycbcr/ media/libtheora/lib/ media/libvpx/

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

(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 ?

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.

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.

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.

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?

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

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.

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

Inbound push for media/libtheora/lib: https://hg.mozilla.org/integration/mozilla-inbound/rev/09861acec08e

instead OS_TEST I guess should be CPU_ARCH used or if CONFIG['OS_TEST'].startswith('arm')

This can't be in "ASSIGNED" status with nobody as the assignee.

This looks to be done: https://dxr.mozilla.org/mozilla-central/search?q=ASFILES+path%3AMakefile.in&redirect=false&case=false