Linux: make ninja respect environment:

Linux: make ninja respect environment:

- CFLAGS
- CXXFLAGS
- LDFLAGS

BUG=251122, gyp:360
R=evan@chromium.org, scottmg@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1691

[Paweł Hajdan Jr., 2013-06-25 00:39:17]

Nico, could you take a look? Thanks!

[Nico, 2013-06-25 00:42:35]

Can you add a test?

https://codereview.chromium.org/17359002/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/17359002/diff/1/pylib/gyp/generator/ninja.py#...
pylib/gyp/generator/ninja.py:762: config.get('cflags_cc', []))
Does it make sense to also check CPPFLAGS and add them to cflags?

I think we want to honor these at least on mac too.

[Paweł Hajdan Jr., 2013-07-24 23:16:55]

PTAL

https://codereview.chromium.org/17359002/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/17359002/diff/1/pylib/gyp/generator/ninja.py#...
pylib/gyp/generator/ninja.py:762: config.get('cflags_cc', []))
On 2013/06/25 00:42:35, Nico wrote:
> Does it make sense to also check CPPFLAGS and add them to cflags?
> 
> I think we want to honor these at least on mac too.

Done.

[Evan Martin, 2013-07-24 23:56:54]

https://codereview.chromium.org/17359002/diff/10001/test/cflags/gyptest-cflag...
File test/cflags/gyptest-cflags.py (right):

https://codereview.chromium.org/17359002/diff/10001/test/cflags/gyptest-cflag...
test/cflags/gyptest-cflags.py:26: # by the make generator.
If the below change is correct, then this comment is out of date.

[Paweł Hajdan Jr., 2013-07-25 21:41:59]

https://codereview.chromium.org/17359002/diff/10001/test/cflags/gyptest-cflag...
File test/cflags/gyptest-cflags.py (right):

https://codereview.chromium.org/17359002/diff/10001/test/cflags/gyptest-cflag...
test/cflags/gyptest-cflags.py:26: # by the make generator.
On 2013/07/24 23:56:55, Evan Martin wrote:
> If the below change is correct, then this comment is out of date.

Done.

[Evan Martin, 2013-07-26 03:30:58]

this lgtm, but i don't really work on gyp

[Evan Martin, 2013-07-26 03:31:00]

this lgtm, but i don't really work on gyp

[scottmg, 2013-07-26 03:42:20]

it doesn't look like this will work on mac or win. or am i misreading?

Nico mentioned upthread that he felt that it ought to work on mac, if it works
on 'else'. i don't think it's too important to have it work on windows, though
it might be nice.

at the very least, the test looks like it'll fail on mac and win bots.

[Paweł Hajdan Jr., 2013-07-26 18:19:23]

On 2013/07/26 03:42:20, scottmg wrote:
> it doesn't look like this will work on mac or win. or am i misreading?

At least it's not introducing a regression. Is there a way to test that?

> Nico mentioned upthread that he felt that it ought to work on mac, if it works
> on 'else'. i don't think it's too important to have it work on windows, though
> it might be nice.
> 
> at the very least, the test looks like it'll fail on mac and win bots.

What are your suggested next steps?

[scottmg, 2013-07-26 18:24:04]

On 2013/07/26 18:19:23, Paweł Hajdan Jr. wrote:
> On 2013/07/26 03:42:20, scottmg wrote:
> > it doesn't look like this will work on mac or win. or am i misreading?
> 
> At least it's not introducing a regression. Is there a way to test that?
> 
> > Nico mentioned upthread that he felt that it ought to work on mac, if it
works
> > on 'else'. i don't think it's too important to have it work on windows,
though
> > it might be nice.
> > 
> > at the very least, the test looks like it'll fail on mac and win bots.
> 
> What are your suggested next steps?

I would run try jobs and fix the test if it turns red.

[Paweł Hajdan Jr., 2013-07-26 23:15:20]

On 2013/07/26 18:24:04, scottmg wrote:
> I would run try jobs and fix the test if it turns red.

The tryjobs failed with apply_issue errors. Are trybots for gyp supported
somehow?

[Ryan Sleevi, 2013-07-26 23:18:36]

On 2013/07/26 23:15:20, Paweł Hajdan Jr. wrote:
> On 2013/07/26 18:24:04, scottmg wrote:
> > I would run try jobs and fix the test if it turns red.
> 
> The tryjobs failed with apply_issue errors. Are trybots for gyp supported
> somehow?

trybots for GYP are supported

http://dev.chromium.org/developers/testing/try-server-usage#TOC-I-m-modifying...

alternatively,
http://dev.chromium.org/developers/testing/try-server-usage#TOC-Submit-a-try-...

that is, using --root

[Paweł Hajdan Jr., 2013-07-30 01:22:22]

Thanks, Ryan! I got it to work with "~/depot_tools/gcl try flags --no_search
--email ..."

PTAL

[scottmg, 2013-08-01 18:02:40]

lgtm, with a bug filed and noted in test for lack of support on non-linux.

[Paweł Hajdan Jr., 2013-08-01 18:28:10]

On 2013/08/01 18:02:40, scottmg wrote:
> lgtm, with a bug filed and noted in test for lack of support on non-linux.

Filed the bug. Nico, what do you think? Could you land this?

[Paweł Hajdan Jr., 2013-08-06 18:37:43]

Lei, could you commit this gyp change?

Nico is on vacation.

[Lei Zhang, 2013-08-07 00:32:43]

On 2013/08/06 18:37:43, Paweł Hajdan Jr. wrote:
> Lei, could you commit this gyp change?

I also don't really work on gyp anymore.

So instead of using the CFLAGS from gyp run time, we'll now use a combination of
flags from gyp and ninja run time?

[scottmg, 2013-08-08 22:48:47]

Committed patchset #7 manually as r1691.

[Nico, 2013-08-12 15:59:39]

I missed that this isn't implemented on OS X :-/ What was the issue?

[Paweł Hajdan Jr., 2013-08-12 19:15:02]

On 2013/08/12 15:59:39, Nico (vacation Aug 2 - Aug 22) wrote:
> I missed that this isn't implemented on OS X :-/ What was the issue?

It seems that the test is relying on some gcc-specific behavior to test whether
-O2 was passed to the build.