|
|
Descriptiongyp: chromium.gyp_env behaves like .gyp/include.gypi
Environment values overwrite values in chromium.gyp_env, while values in
.gyp/include.gypi are merged to environment values. The difference is
confusing.
Moreover, .gyp/include.gypi behavior is more convenient because some developers
build multiple targets (e.g. chromium, chromeos, android chrome) so they cannot
use chromium.gyp_env.
Committed: https://crrev.com/ce4b0d178d5ecc3b085011cd0abe146c3ba6d85b
Cr-Commit-Position: refs/heads/master@{#319232}
Patch Set 1 #Patch Set 2 : preserve overriding order #
Total comments: 4
Patch Set 3 : apply it to only GYP_DEFINES #
Total comments: 5
Patch Set 4 : ', not " #Messages
Total messages: 21 (4 generated)
dongseong.hwang@intel.com changed reviewers: + thakis@chromium.org
On 2015/02/16 16:56:13, dshwang wrote: > mailto:dongseong.hwang@intel.com changed reviewers: > + mailto:thakis@chromium.org I wanna change chromium.gyp_env's behavior like .gyp/include.gypi WDYT? The change looks backward compatible also.
Should the values from the .gyp_env file be prepended, so that on conflicting settings, the env vars win? In general, I think this seems like a good change, but it's also changing something relatively subtle that has been the same for a long time. Right now, one can't use both .gyp_env file and env vars, and after this change it's possible but still a mostly bad idea unless one knows what they're doing – and in that case, one can make the current behavior work too. So I'm not sure if we should do it.
On 2015/02/17 17:16:53, Nico wrote: > Should the values from the .gyp_env file be prepended, so that on conflicting > settings, the env vars win? .gyp_env overrides env vars like how include.gypi behaves. it's because GetGypVars() in gyp_chromium makes later value override early value. > In general, I think this seems like a good change, but it's also changing > something relatively subtle that has been the same for a long time. Right now, > one can't use both .gyp_env file and env vars, and after this change it's > possible but still a mostly bad idea unless one knows what they're doing – and > in that case, one can make the current behavior work too. So I'm not sure if we > should do it. That's good point. IMO, it's fine. Currently gyp_chromium ignore env vars if there is .gyp_env. After this CL, gyp_chromium overrides env vars by .gyp_env. The result behavior is similar. e.g. let's env vars has component=shared_library and .gyp_env has component=static_library. gyp_chromium takes component=static_library no matter this CL.
On 2015/02/17 19:31:45, dshwang wrote: > On 2015/02/17 17:16:53, Nico wrote: > > Should the values from the .gyp_env file be prepended, so that on conflicting > > settings, the env vars win? > > .gyp_env overrides env vars like how include.gypi behaves. it's because > GetGypVars() in gyp_chromium makes later value override early value. After this CL, if there are env vars, .gyp_env, include.gypi .gyp_env overrides env vars and then include.gypi overrides both.
Current behavior is that .gyp_env is ignored if there is any evn vars although massage is "INFO: Environment value for "GYP_DEFINES" overrides value in chromium.gyp_env" This CL makes evn vars overrides chromium.gyp_env like the message. if there are env vars, .gyp_env, include.gypi, env vars overrides .gyp_env ,and then include.gypi overrides both.
Hi Nico, could you review again?
thakis@chromium.org changed reviewers: + scottmg@chromium.org
+scottmg for a second opinion
Meh. These files are a bug anyway. https://codereview.chromium.org/931643002/diff/20001/build/gyp_helper.py File build/gyp_helper.py (right): https://codereview.chromium.org/931643002/diff/20001/build/gyp_helper.py#newc... build/gyp_helper.py:45: os.environ[var] = file_val + ' ' + os.environ[var]; merging with a space seems wrong? e.g. GYP_GENERATORS is , separated. https://codereview.chromium.org/931643002/diff/20001/build/gyp_helper.py#newc... build/gyp_helper.py:46: print 'INFO: Environment value for "%s" overrides value in %s' % ( This print seems incorrect now.
thx for reviewing. I addressed the concerns https://codereview.chromium.org/931643002/diff/20001/build/gyp_helper.py File build/gyp_helper.py (right): https://codereview.chromium.org/931643002/diff/20001/build/gyp_helper.py#newc... build/gyp_helper.py:45: os.environ[var] = file_val + ' ' + os.environ[var]; On 2015/02/25 19:58:20, scottmg wrote: > merging with a space seems wrong? e.g. GYP_GENERATORS is , separated. I only applied this change to GYP_DEFINES in next patch https://codereview.chromium.org/931643002/diff/20001/build/gyp_helper.py#newc... build/gyp_helper.py:46: print 'INFO: Environment value for "%s" overrides value in %s' % ( On 2015/02/25 19:58:20, scottmg wrote: > This print seems incorrect now. yes, only GYP_DEFINES is overriden. other env values are replaced. now the log is as follows: INFO: Environment value for "GYP_DEFINES" overrides value in /home/dshwang/chromium/chromium.gyp_env INFO: Environment value for "GYP_GENERATORS" replaces value in /home/dshwang/chromium/chromium.gyp_env For your information, the log is as follow when ~/.gyp/include.gypi overrides env values. Using overrides found in ~/.gyp/include.gypi
would you have a chance to look at it again?
lgtm with those I guess unless Nico would prefer not. https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py File build/gyp_helper.py (right): https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py#newc... build/gyp_helper.py:45: behavior = "replaces"; please use ', not " https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py#newc... build/gyp_helper.py:48: behavior = "overrides" 'overrides' doesn't seem correct. 'prepended to'? Also, ', not ".
https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py File build/gyp_helper.py (right): https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py#newc... build/gyp_helper.py:45: behavior = "replaces"; On 2015/03/04 18:10:37, scottmg wrote: > please use ', not " yes, I'll update. https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py#newc... build/gyp_helper.py:48: behavior = "overrides" On 2015/03/04 18:10:37, scottmg wrote: > 'overrides' doesn't seem correct. 'prepended to'? Also, ', not ". The behavior is 'prepended to', but if there is duplicated values, earlier one is taken. So this behavior can be called as 'overrides' In addition, ~/.gyp/include.gypi uses 'overrides' term so I wanted to match the terminology. `Using overrides found in ~/.gyp/include.gypi` WDYT?
https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py File build/gyp_helper.py (right): https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py#newc... build/gyp_helper.py:48: behavior = "overrides" On 2015/03/04 18:28:07, dshwang wrote: > On 2015/03/04 18:10:37, scottmg wrote: > > 'overrides' doesn't seem correct. 'prepended to'? Also, ', not ". > > The behavior is 'prepended to', but if there is duplicated values, earlier one > is taken. So this behavior can be called as 'overrides' > In addition, ~/.gyp/include.gypi uses 'overrides' term so I wanted to match the > terminology. > `Using overrides found in ~/.gyp/include.gypi` > WDYT? Sigh, OK, fine as-is then. `git rm build/gyp_helper.py` ftw.
On 2015/03/04 18:33:44, scottmg wrote: > https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py > File build/gyp_helper.py (right): > > https://codereview.chromium.org/931643002/diff/40001/build/gyp_helper.py#newc... > build/gyp_helper.py:48: behavior = "overrides" > On 2015/03/04 18:28:07, dshwang wrote: > > On 2015/03/04 18:10:37, scottmg wrote: > > > 'overrides' doesn't seem correct. 'prepended to'? Also, ', not ". > > > > The behavior is 'prepended to', but if there is duplicated values, earlier one > > is taken. So this behavior can be called as 'overrides' > > In addition, ~/.gyp/include.gypi uses 'overrides' term so I wanted to match > the > > terminology. > > `Using overrides found in ~/.gyp/include.gypi` > > WDYT? > > Sigh, OK, fine as-is then. > > `git rm build/gyp_helper.py` ftw. Thanks for reviewing. That's good idea but someone/bots might use chromium.gyp_env
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/931643002/#ps60001 (title: "', not "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/931643002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ce4b0d178d5ecc3b085011cd0abe146c3ba6d85b Cr-Commit-Position: refs/heads/master@{#319232} |