874266 - move DEFINES to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Joey Armstrong [:joey]]

Attachment: move DEFINES to moz.build (logic)

Comment 2

[Joey Armstrong [:joey]]

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

Variable passthrough for DEFINES=

Comment 3

[Gregory Szorc [:gps]]

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.

Comment 4

[(not currently active) Ted Mielczarek]

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.

Comment 5

[Gregory Szorc [:gps]]

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

Comment 6

[Joey Armstrong [:joey]]

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

Comment 7

[Joshua Cranmer [:jcranmer]]

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

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Joey Armstrong [:joey]]

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

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4aad0a589b5b

Comment 11

[Joshua Cranmer [:jcranmer]]

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.

Comment 12

[(not currently active) Ted Mielczarek]

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.

Comment 13

[(not currently active) Ted Mielczarek]

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?

Comment 14

[Benoit Girard (:BenWa)]

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.

Comment 15

[(not currently active) Ted Mielczarek]

Probably just horrible historical reasons.

Comment 16

[Joshua Cranmer [:jcranmer]]

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

Comment 17

[Gregory Szorc [:gps]]

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.

Comment 18

[Michael Shal [:mshal]]

Attachment: Support quoted strings in DEFINES in moz.build

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 19

[Joshua Cranmer [:jcranmer]]

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

Comment 20

[Michael Shal [:mshal]]

Attachment: Move DEFINES to moz.build (part 1)

Comment 21

[Michael Shal [:mshal]]

Attachment: Move DEFINES to moz.build (part 2)

Comment 22

[Michael Shal [:mshal]]

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

Comment 23

[Gregory Szorc [:gps]]

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?

Comment 24

[Benoit Girard (:BenWa)]

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.

Comment 25

[Michael Shal [:mshal]]

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.

Comment 26

[Joshua Cranmer [:jcranmer]]

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.

Comment 27

[Michael Shal [:mshal]]

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?

Comment 28

[Benjamin Smedberg]

DEFINES['BAZ'] = "foo" is fine, or you can also do;

DEFINES.update({'BAZ': 'abcd',

Comment 29

[Michael Shal [:mshal]]

Attachment: 0001-Bug-874266-Convert-DEFINES-to-be-a-dict-instead-of-a.patch

Comment 30

[Michael Shal [:mshal]]

Attachment: Convert DEFINES to moz.build part-1

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.

Comment 31

[Michael Shal [:mshal]]

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

Comment 32

[Gregory Szorc [:gps]]

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'

Comment 33

[Mike Hommey [:glandium]]

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

[Gregory Szorc [:gps]]

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 35

[Gregory Szorc [:gps]]

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?

Comment 36

[Michael Shal [:mshal]]

(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...? :)

Comment 37

[Gregory Szorc [:gps]]

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

Comment 38

[Michael Shal [:mshal]]

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

[Gregory Szorc [:gps]]

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 40

[(not currently active) Ted Mielczarek]

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.

Comment 41

[Glenn Randers-Pehrson]

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.

Comment 42

[Michael Shal [:mshal]]

Attachment: 0001-Bug-874266-Convert-DEFINES-to-be-a-dict-instead-of-a.patch

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 43

[Gregory Szorc [:gps]]

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.

Comment 44

[Michael Shal [:mshal]]

Attachment: 0001-Bug-874266-Convert-DEFINES-to-be-a-dict-instead-of-a.patch

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

Comment 45

[Michael Shal [:mshal]]

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 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a1aa932eaf79

Comment 47

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 48

[Mike Hommey [:glandium]]

Attachment: Move all DEFINES that can be moved to moz.build

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 49

[Michael Shal [:mshal]]

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?

Comment 50

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd4b546eed4

Comment 51

[Mike Hommey [:glandium]]

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.

Comment 52

[Mike Hommey [:glandium]]

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

Comment 53

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b143a9c624

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4b143a9c624

Comment 55

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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