Comment on attachment 751941 [details] [diff] [review] move DEFINES to moz.build (logic) Variable passthrough for DEFINES=

Comment on attachment 751941 [details] [diff] [review] move DEFINES to moz.build (logic) Review of attachment 751941 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +65,5 @@ > > This variable contains a list of files to invoke the assembler on. > """), > > + 'DEFINES': (StrictOrderingOnAppendList, list, [], Are there any cases where order is important for defines? I don't /think/ so, but I'm not up to date on all the inner workings of C/C++ compilers these days.

Only if you're trying to undefine something that was previously passed as a define, but anyone trying to do that in the same set of DEFINES should be taken out back anyway.

Well presumably we can detect this at moz.build traversal time, so it shouldn't be an issue.

(In reply to Gregory Szorc [:gps] from comment #3) > Comment on attachment 751941 [details] [diff] [review] > move DEFINES to moz.build (logic) > > Review of attachment 751941 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py > @@ +65,5 @@ > > > > This variable contains a list of files to invoke the assembler on. > > """), > > > > + 'DEFINES': (StrictOrderingOnAppendList, list, [], > > Are there any cases where order is important for defines? I don't /think/ > so, but I'm not up to date on all the inner workings of C/C++ compilers > these days. I made a quick sweep through the makefiles yesterday and did not see any glaring problems with switch order that might cause a problem for the sorted list. Yes on the woodshed if we are playing ping-pong with defines. It should be easy enough to have the conversion watch for macro/value names being referenced more than once and flag it as a potential problem.

Maybe it might make more sense to make DEFINES instead be a dictionary?

Or a [ordered] set? We'll revisit defines later when we roll out a decent ontology for defining libraries. So I'm fine with a list for now.

Inbound push changeset: 132804:4aad0a589b5b https://hg.mozilla.org/integration/mozilla-inbound/rev/4aad0a589b5b

In attempting to move the DEFINES in comm-central, I've noticed that mozbuild-migrate moves -DFOO=\"FOO\" [in the Makefile] to '-DFOO=\"FOO\"' [in the mozbuild, which would translate to -DFOO="FOO" as the string] which is output by mozbuild as -DFOO="FOO" [in the Makefile] which gets used by the source file as FOO instead of "FOO" I'm not sure if the DEFINES should be marked as r'' in the Python file or if they should be specified in a "clean" form and have the backend generator insert the extra slashes. However, for the time being, it is a major concern for using mozbuild-migrate.

We should probably specify the behavior we want here. Ideally what you put in DEFINES in moz.build would get passed unmolested to the compiler, so build backends should do whatever escaping necessary to make that happen.

I propose to codify what I said above: DEFINES in moz.build should be exactly what you want to be passed to the compiler, and the backend should escape as apropriate, so like: DEFINES = [ '-DFOO="FOO"', ] would then get expanded to: DEFINES := -DFOO=\"FOO\" in a Makefile. I'm not sure precisely what escaping we need here, probably just escaping shell metacharacters?

Do we have a good reason for having local DEFINES in sub directories? It makes it very difficult to get tools like clangcomplete to work when source files have different sets of includes. Is it possible to get the DEFINES to be global and all go in mozilla-config.h? I can't think of a case where a DEFINE would be required but hurt if it was project wide. It's also very confusing when developing that defines are not universal.

Probably just horrible historical reasons.

(In reply to Benoit Girard (:BenWa) from comment #14) > Do we have a good reason for having local DEFINES in sub directories? It > makes it very difficult to get tools like clangcomplete to work when source > files have different sets of includes. Is it possible to get the DEFINES to > be global and all go in mozilla-config.h? I can't think of a case where a > DEFINE would be required but hurt if it was project wide. It's also very > confusing when developing that defines are not universal. In the comm-central side, I'm planning on going through each of the DEFINES individually to evaluate their necessity. In practice, I've found DEFINES fall into a few categories: 1. -DNOMINMAX, -DUNICODE -> magic macros for Windows that we probably ought to define everywhere. 2. Defining a value based on an AC_SUBST that's not in an AC_DEFINE (comm-central uses MOZ_PSM to guard for S/MIME and has a more convoluted system to set up Movemail support; a few other things get defined in the build/ directories so that we don't try to add components for things that don't exist). 3. Non-C/C++ defines. E.g., calendar uses DEFINES to set up versions for its install.rdf files.

I'd love to kill one-off defines and compiler flags where possible. That can largely be done in parallel to moz.build conversion work.

This adds automatic escaping of quotes in the make backend. It is done for all passthru variables, which currently only affects DEFINES since no other variables have quotes in them yet. I've confirmed that it works in dom/system with the following moz.build addition: DEFINES += [ '-DDLL_PREFIX="%s"' % CONFIG['DLL_PREFIX'], '-DDLL_SUFFIX="%s"' % CONFIG['DLL_SUFFIX'], ] jcranmer: Can you confirm that this works as you'd expect in c-c?

Comment on attachment 777164 [details] [diff] [review] Support quoted strings in DEFINES in moz.build I'm slightly concerned about only escaping ", but this should be fine for now [famous last words].

try is happy through part 2: https://tbpl.mozilla.org/?tree=Try&rev=4d65e993e0c2

Comment on attachment 751941 [details] [diff] [review] move DEFINES to moz.build (logic) Review of attachment 751941 [details] [diff] [review]: ----------------------------------------------------------------- I'm having second thoughts about this r+. Mainly, what's the point in adding "-D" everywhere. I think we should omit this from moz.build. As a short term workaround, emitter.py could add it back in to the pass-thru value. Thoughts?

I was actually wondering why we would specify in moz.build. It doesn't provide any value in specifying and omitting it let's us abstract the implementation detail of how defines are passed to the compiler. Say if a compiler or static analysis tool wants a list of defines but uses a different syntax to pass them.

I am indifferent to removing/keeping the -D flag. If we go with the dict route as suggested by jcranmer I think it makes sense to remove it, because it would have an obviously different syntax from INCLUDES, *FLAGS, etc. If removing it now before we use a dict helps static analysis tools, then let's do it.

The original -D addition was to make moz.build conversion faster and easier. But thanks to the shell-quoting issue, I personally (nor would the c-c build peers) don't trust automatic migration here, which means the marginal extra difficulty in moving to a dict is much less. I don't see much value in having a list without the -D option, and I see less value in migrating to the list with the D than I do in just migrating to a dict in the first place.

Python n00b question: how do we handle easily adding multiple defines with a dict? Eg we might have: if CONFIG['BLAH']: DEFINES += [ '-DFOO', '-DBAR', ] DEFINES += [ '-DBAZ="abcd"', ] Does this become something like this? if CONFIG['BLAH']: DEFINES.update({ 'FOO': True, 'BAR': True, }) DEFINES['BAZ'] = "abcd"; Or do we try to overload +=, or use a function like add_tier_dir?

DEFINES['BAZ'] = "foo" is fine, or you can also do; DEFINES.update({'BAZ': 'abcd',

Here's an example conversion of dom/ using the dict style. Is this what you guys are expecting? I've locally hacked up mozbuild-migrate to support generating this automatically.

I should clarify: I've only tested the dict changes locally so far.

Gah, you posted a patch before I finished the comment I started typing earlier. Anyway, I was going to throw out the idea of using a set-like object. I'd like to support +=, but built-in set() doesn't support that, so we'd have to create a custom class. For the simple case of no value: DEFINES += 'FOO' DEFINES += ['FOO', 'BAR'] For the case of a value, we could assign a tuple: DEFINES += ('FOO', 'VALUE') DEFINES += [('FOO', 'VALUE'), ('BAR', 'VALUE2')] I haven't looked at your patch, but we can also use a dict-like object and intercept assignments: DEFINES += ('FOO', 'BAR') or DEFINES += ['FOO', 'BAR'] And then for defines with values: DEFINES['WITH_VALUE'] = 'VALUE'

(In reply to Gregory Szorc [:gps] from comment #32) > DEFINES += ('FOO', 'BAR') > or > DEFINES += ['FOO', 'BAR'] It's not clear at first sight what this does. Hence, I think this form should be avoided.

Comment on attachment 751941 [details] [diff] [review] move DEFINES to moz.build (logic) Review of attachment 751941 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling this r+ since it looks like we're switching to a dict.

Comment on attachment 778047 [details] [diff] [review] 0001-Bug-874266-Convert-DEFINES-to-be-a-dict-instead-of-a.patch Review of attachment 778047 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good. If this was applied on top of the previous patch, please merge them before next review. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +347,5 @@ > + defstr = define > + elif type(value) == int: > + defstr = '%s=%s' % (define, value) > + else: > + defstr = '%s=\'%s\'' % (define, value) I'd consider moving this logic to an API on the Defines class. Otherwise, I feel each backend will reinvent it. I'd also have the docs include when to use True vs 1, since I imagine some people may use them interchangeably. Also, if we only accept certain types, I'd prefer to catch invalid types at assignment time. I suppose we could defer that to a follow-up. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +84,5 @@ > + These are passed in to the compiler as -Dkey='value' for string values, > + -Dkey=value for numeric values, or -Dkey if the value is True. Note > + that for string values, the outer-level of single-quotes will be > + consumed by the shell. If you want to have a string-literal in the > + program, the value needs to have double-quotes. The docs should give some examples here, notably that True should be assigned for simple defines and a string literal for others. There should especially be docs around quoting, since the current docs are a bit confusing, IMO. ::: python/mozbuild/mozbuild/test/backend/data/defines/moz.build @@ +2,5 @@ > +# http://creativecommons.org/publicdomain/zero/1.0/ > + > +value = 'xyz' > +DEFINES = { > + 'FOO': True, 4-space indent! ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py @@ -153,5 @@ > ], > - 'DEFINES': [ > - 'DEFINES += -Dbar', > - 'DEFINES += -Dfoo', > - ], Wait, was this applied on top of the previous patch on this bug?

(In reply to Gregory Szorc [:gps] from comment #32) > Gah, you posted a patch before I finished the comment I started typing > earlier. Anyway, I was going to throw out the idea of using a set-like > object. I'd like to support +=, but built-in set() doesn't support that, so > we'd have to create a custom class. Sorry, I missed your earlier comment about considering a set. I thought the plan was '-Dfoo' now and transition to a dict later, but then we bumped that up to just be a dict now. I'm happy to consider other alternatives - the dict patches at least show what a working example would actually look like for discussion purposes. > > For the simple case of no value: > > DEFINES += 'FOO' > DEFINES += ['FOO', 'BAR'] > > For the case of a value, we could assign a tuple: > > DEFINES += ('FOO', 'VALUE') > DEFINES += [('FOO', 'VALUE'), ('BAR', 'VALUE2')] These look pretty reasonable to me - I thought the .update() call for the dict approach was a bit annoying. I'll see if I can get this working and then we can compare the two & pick (unless there are other approaches too...? :)

I think dict > list. We can always swap in a more intelligent class later that supports better assignment semantics.

(In reply to Gregory Szorc [:gps] from comment #34) > Comment on attachment 751941 [details] [diff] [review] > move DEFINES to moz.build (logic) > > Review of attachment 751941 [details] [diff] [review]: > ----------------------------------------------------------------- > > Cancelling this r+ since it looks like we're switching to a dict. This patch has already landed - DEFINES is available in mozbuild as a list, though no actual moz.build files use it yet so it is easy to change now.

Comment on attachment 751941 [details] [diff] [review] move DEFINES to moz.build (logic) Review of attachment 751941 [details] [diff] [review]: ----------------------------------------------------------------- Re-adding r+ since this landed already. Derp.

Comment on attachment 778052 [details] [diff] [review] Convert DEFINES to moz.build part-1 This seems pretty sane. The only awkwardness is the mapping of "define a symbol with no value", where you're mapping DEFINES += -DFOO to DEFINES['FOO'] = True ...but it's not the worst thing. The only better suggestion I can think of off the top of my head is to also allow assignment (like gps suggested), but only for strings, so that you could write: DEFINES += "FOO" ...but I'm not sure if that's actually clearer or not.

DEFINES are not getting passed to the assembler for functions mentioned in SSRCS. See bug #832390; the DEFINES get passed to libpng's arm/arm_init.c but not to arm/filter_neon.S.

Updated patch to latest m-c and included gps' feedback. This is still just the dict approach - I haven't tried any others. But ted is working on another bug and wanted DEFINES at least somewhat finalized so he could put them in a moz.build file, so we should probably go with this for now unless there are major objections.

Comment on attachment 805626 [details] [diff] [review] 0001-Bug-874266-Convert-DEFINES-to-be-a-dict-instead-of-a.patch Review of attachment 805626 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +206,5 @@ > backend_file.write('%s := %s\n' % (k, v)) > + > + elif isinstance(obj, Defines): > + for define in obj.get_defines(): > + backend_file.write('DEFINES += %s\n' % define) Bonus points if you write this out as a single line to make the parser do less work. ::: python/mozbuild/mozbuild/frontend/data.py @@ +169,5 @@ > + elif type(value) == int: > + defstr = '%s=%s' % (define, value) > + else: > + defstr = '%s=\'%s\'' % (define, value) > + yield('-D%s' % defstr) This is a slightly leaky abstraction since our build backend happens to take care of normalizing -D to /D for MSVC. But, this is the simplest and we can change it later without too much effort.

Updated to write DEFINES out in a single line. r+ carried forward.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1aa932eaf79 try results: (with a test dom/ conversion) https://tbpl.mozilla.org/?tree=Try&rev=934848513d53

Comment on attachment 778052 [details] [diff] [review] Convert DEFINES to moz.build part-1 Review of attachment 778052 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/moz.build @@ +69,5 @@ > 'PDocumentRenderer.ipdl', > 'PMemoryReportRequest.ipdl', > 'PTabContext.ipdlh', > ] > +DEFINES['BIN_SUFFIX'] = '"%s"' % CONFIG['BIN_SUFFIX'] I can't say I like the quotes much, but I don't have an alternative.

This involved a lot of manual work, so there might be some things wrong, although many things blew up on try and now look green after many fixups. https://tbpl.mozilla.org/?tree=Try&rev=a42b69c90b26

Comment on attachment 8339263 [details] [diff] [review] Move all DEFINES that can be moved to moz.build > # This is needed to avoid making run-b2g depend on mozglue >diff --git a/b2g/gaia/moz.build b/b2g/gaia/moz.build >--- a/b2g/gaia/moz.build >+++ b/b2g/gaia/moz.build >@@ -5,12 +5,16 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > PROGRAM = CONFIG['MOZ_APP_NAME'] > > if CONFIG['OS_ARCH'] == 'WINNT': > SOURCES += [ > 'run-b2g.cpp', > ] >+ DEFINES['B2G_NAME'] = 'L"%s-bin%s"' % (PROGRAM, CONFIG['BIN_SUFFIX']) >+ DEFINES['GAIA_PATH'] = 'L"gaia\\profile"' Are you sure GAIA_PATH has the right number of backslashes? With the Makefile version I get this on the command-line: -DB2G_NAME=L\"firefox-bin\" -DGAIA_PATH=L\"gaia\\\\profile\" And with the moz.build version I get: -DB2G_NAME='L"firefox-bin"' -DGAIA_PATH='L"gaia\profile"' Which gives me: /home/mshal/mozilla-central-git/b2g/gaia/run-b2g.c:36:46: warning: unknown escape sequence: '\p' [enabled by default] (I forced this code path in Linux, so maybe that's a false report). > else: > SOURCES += [ > 'run-b2g.c', > ] >+ DEFINES['B2G_NAME'] = '"%s-bin%s"' % (PROGRAM, CONFIG['BIN_SUFFIX']) >+ DEFINES['GAIA_PATH'] = 'L"gaia/profile"' The Makefile only specifies 'L' in GAIA_PATH for the WINNT branch, not the else branch. >diff --git a/gfx/angle/src/libEGL/moz.build b/gfx/angle/src/libEGL/moz.build >--- a/gfx/angle/src/libEGL/moz.build >+++ b/gfx/angle/src/libEGL/moz.build >@@ -20,8 +20,18 @@ SOURCES += [ > 'main.cpp', > 'Surface.cpp', > ] > > # On Windows, we don't automatically get "lib" prepended, but we need it. > LIBRARY_NAME = 'libEGL' > > FORCE_SHARED_LIB = True >+ >+for var in ('LIBEGL_EXPORTS', 'ANGLE_BUILD', 'NOMINMAX', >+ '_CRT_SECURE_NO_DEPRECATE', 'ANGLE_DISABLE_TRACE', >+ 'COMPILER_IMPLEMENTATION'): Where did 'COMPILER_IMPLEMENTATION' come from? I see it in gfx/angle/Makefile.in and gfx/angle/src/libGLESv2/Makefile.in, not in libEGL. >--- a/netwerk/sctp/src/Makefile.in >+++ b/netwerk/sctp/src/Makefile.in >@@ -21,75 +21,45 @@ LOCAL_INCLUDES = \ > # Android NDK r5c, used on the builders at the time of this writing, doesn't > # have the headers we need for IPv6 > ifeq ($(OS_TARGET),Android) > IPV6_DEFINE= > else > IPV6_DEFINE=-DINET6=1 > endif Mind as well remove this block too. >diff --git a/toolkit/xre/moz.build b/toolkit/xre/moz.build >--- a/toolkit/xre/moz.build >+++ b/toolkit/xre/moz.build >+for var in ('GRE_MILESTONE', 'MOZ_APP_ID'): >+ DEFINES[var] = CONFIG[var] The Makefile has -DAPP_ID="$(MOZ_APP_ID)", not -DMOZ_APP_ID. Maybe group it with MOZ_APP_VERSION rather than GRE_MILESTONE?

Let's close this bug when it reaches m-c. The remaining DEFINES are things that will need to be handled separately, they can use their own bug for that.

Backed out for ASAN and windows bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/e668d88804d1

Comment on attachment 8339263 [details] [diff] [review] Move all DEFINES that can be moved to moz.build Review of attachment 8339263 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libopus/Makefile.in @@ -3,5 @@ > # You can obtain one at http://mozilla.org/MPL/2.0/. > > -DEFINES += \ > - -DOPUS_BUILD \ > - -DOPUS_VERSION='"v1.1-beta-23-gf2446c2-mozilla"' \ Looks like this broke sed -e "s/-DOPUS_VERSION='\".*\"'/-DOPUS_VERSION='\"${version}-mozilla\"'/" \ ${TARGET}/Makefile.in > ${TARGET}/Makefile.in+ && \ mv ${TARGET}/Makefile.in+ ${TARGET}/Makefile.in in media/libopus/update.sh