Closed Bug 944634 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Missed another problematic define and a typo in mozmake.py.
Attachment #8340283 - Flags: review?(ted)
Attachment #8340273 - Attachment is obsolete: true
Attachment #8340273 - Flags: review?(ted)
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.
Attachment #8340283 - Flags: review?(ted) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/04f25103c4a9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.