944634 - mozmake.py (for gyp) handling of escaping (or lack thereof) for defines is wrong

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

See media/webrtc/trunk/tools/gyp/test/defines-escaping/defines-escaping.gyp and ./media/webrtc/trunk/tools/gyp/test/defines-escaping/defines-escaping.c. The way defines work in gyp is that what is passed is what the compiler needs to see literally in its arguments list. So 'TEST_FORMAT="foo"' in defines would translate, on a shell command line, with proper escaping, into -DTEST_FORMAT=\"foo\", or -DTEST_FORMAT='"foo"', or '-DTEST_FORMAT="foo"', or anything else that gives the same value in the args list in the compiler main().

However, mozmake.py just takes the value from defines, and writes it verbatim in the makefile, which, in turn, makes the shell command line be literally -DTEST_FORMAT="foo", which, in compiler's main(), translates to -DTEST_FORMAT=foo. Yeah, that's not going to work.

Consequently, we've written .gyp definitions to work around this bad behaviour, which, in turn, means those .gyp files wouldn't work with anything else than mozmake.py.

Comment 1

[Mike Hommey [:glandium]]

Attachment: mozmake.py (for gyp) handling of escaping (or lack thereof) for defines is wrong

Comment 2

[Mike Hommey [:glandium]]

Attachment: mozmake.py (for gyp) handling of escaping (or lack thereof) for defines is wrong

Missed another problematic define and a typo in mozmake.py.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8340283 [details] [diff] [review]
mozmake.py (for gyp) handling of escaping (or lack thereof) for defines is wrong

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

Did you look at what other gyp backends do? Seems like our quoting should be in line with that.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Comment on attachment 8340283 [details] [diff] [review]
> mozmake.py (for gyp) handling of escaping (or lack thereof) for defines is
> wrong
> 
> Review of attachment 8340283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you look at what other gyp backends do? Seems like our quoting should be
> in line with that.

https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/tools/gyp/pylib/gyp/generator/make.py#586

which is essentially what the patch does, except for the $ and # escaping. But we're not uhaving problems with those. I'm going to heavily modify mozmake.py in the near future, so this patch is good enough for now.

Comment 5

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04f25103c4a9

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04f25103c4a9

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #4)
> which is essentially what the patch does, except for the $ and # escaping.
> But we're not uhaving problems with those. I'm going to heavily modify
> mozmake.py in the near future, so this patch is good enough for now.

Amen to that.