Move nacl_defines to build.common.gypi.

Move nacl_defines to build/nacl_defines.gypi.

Remove the definition of nacl_defines from chrome.gyp. This will allow including these defines in other gyp files which are not in the chrome layer.

This is part of an effort to componentize NaCl code.

BUG=244791
R=jam@chromium.org, mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214344

[yael.aharon1, 2013-07-27 11:29:47]

Adding nacl_defines to buid/common.gypi did not work, so I created a new gypi
file for nacl_defines. Mark, can you please review?

[Mark Seaborn, 2013-07-28 00:41:07]

https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi
File build/nacl_defines.gypi (right):

https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi#n...
build/nacl_defines.gypi:5: {
Should this file go in components/nacl, to keep the NaCl-related stuff together?

https://codereview.chromium.org/20550003/diff/35001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/20550003/diff/35001/chrome/chrome.gyp#newcode116
chrome/chrome.gyp:116: 'defines': [
In the removed code, there's nothing about the field 'defines', only
'nacl_defines'.  How come you're adding a 'defines' field here?

[yael.aharon1, 2013-07-28 13:20:20]

On 2013/07/28 00:41:07, Mark Seaborn wrote:
> https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi
> File build/nacl_defines.gypi (right):
> 
>
https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi#n...
> build/nacl_defines.gypi:5: {
> Should this file go in components/nacl, to keep the NaCl-related stuff
together?
This will be needed in components_tests.gypi and later on in nacl_host.gypi.
I thought it would be better to have it in one place and include it where it is
needed.
WDYT?

> 
> https://codereview.chromium.org/20550003/diff/35001/chrome/chrome.gyp
> File chrome/chrome.gyp (right):
> 
>
https://codereview.chromium.org/20550003/diff/35001/chrome/chrome.gyp#newcode116
> chrome/chrome.gyp:116: 'defines': [
> In the removed code, there's nothing about the field 'defines', only
> 'nacl_defines'.  How come you're adding a 'defines' field here?
I thought it was needed just like in other gyp files. I updated the CL to not
have this extra, let's see if the try bots are ok with the change.

[Mark Seaborn, 2013-07-29 15:59:21]

On 28 July 2013 06:20, <yael.aharon@chromium.org> wrote:

> On 2013/07/28 00:41:07, Mark Seaborn wrote:
>
>> https://codereview.chromium.**org/20550003/diff/35001/build/**
>>
nacl_defines.gypi<https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi>
>> File build/nacl_defines.gypi (right):
>>
>
>
> https://codereview.chromium.**org/20550003/diff/35001/build/**
>
nacl_defines.gypi#newcode5<https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi#newcode5>
>
>> build/nacl_defines.gypi:5: {
>> Should this file go in components/nacl, to keep the NaCl-related stuff
>> together?
>>
> This will be needed in components_tests.gypi and later on in
> nacl_host.gypi.
> I thought it would be better to have it in one place and include it where
> it is
> needed.
> WDYT?


I wasn't suggesting duplicating the file.  What I meant was, is there a
reason not to put this file under components/nacl/ (rather than under
build/) and have components_tests.gypi and nacl_host.gpi include it?

Cheers,
Mark

[yael.aharon1, 2013-07-29 16:11:55]

On 2013/07/29 15:59:21, Mark Seaborn wrote:
> On 28 July 2013 06:20, <mailto:yael.aharon@chromium.org> wrote:
> 
> > On 2013/07/28 00:41:07, Mark Seaborn wrote:
> >
> >> https://codereview.chromium.**org/20550003/diff/35001/build/**
> >>
>
nacl_defines.gypi<https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi>
> >> File build/nacl_defines.gypi (right):
> >>
> >
> >
> > https://codereview.chromium.**org/20550003/diff/35001/build/**
> >
>
nacl_defines.gypi#newcode5<https://codereview.chromium.org/20550003/diff/35001/build/nacl_defines.gypi#newcode5>
> >
> >> build/nacl_defines.gypi:5: {
> >> Should this file go in components/nacl, to keep the NaCl-related stuff
> >> together?
> >>
> > This will be needed in components_tests.gypi and later on in
> > nacl_host.gypi.
> > I thought it would be better to have it in one place and include it where
> > it is
> > needed.
> > WDYT?
> 
> 
> I wasn't suggesting duplicating the file.  What I meant was, is there a
> reason not to put this file under components/nacl/ (rather than under
> build/) and have components_tests.gypi and nacl_host.gpi include it?
> 
> Cheers,
> Mark

I updated the CL to move it under components/. Like Joi said before, adding gypi
files in sub-components can cause issues, and probably not desired.

[Mark Seaborn, 2013-07-29 18:30:28]

On 29 July 2013 09:11, <yael.aharon@chromium.org> wrote:

> On 2013/07/29 15:59:21, Mark Seaborn wrote:
>
>> I wasn't suggesting duplicating the file.  What I meant was, is there a
>> reason not to put this file under components/nacl/ (rather than under
>> build/) and have components_tests.gypi and nacl_host.gpi include it?
>>
>
> I updated the CL to move it under components/. Like Joi said before,
> adding gypi
> files in sub-components can cause issues, and probably not desired.
>

The reason for that is that source files referenced by a .gypi file are
interpreted relative to the .gyp file that includes it, not relative to the
.gypi file.  This Gyp problem results in the convention that if a .gypi
file references source files, it should only be included from .gyp files in
the same directory as the .gypi file.

So, this Gyp problem doesn't apply to your new nacl_defines.gypi file,
because it doesn't reference any source files.  If it did reference any
source files, then you wouldn't be able to put nacl_defines.gypi in build/
(or directly under components/) and include it from chrome/.  So putting it
in components/nacl/ should be OK too.

Mark

[yael.aharon1, 2013-07-29 18:58:33]

On 2013/07/29 18:30:28, Mark Seaborn wrote:
> On 29 July 2013 09:11, <mailto:yael.aharon@chromium.org> wrote:
> 
> > On 2013/07/29 15:59:21, Mark Seaborn wrote:
> >
> >> I wasn't suggesting duplicating the file.  What I meant was, is there a
> >> reason not to put this file under components/nacl/ (rather than under
> >> build/) and have components_tests.gypi and nacl_host.gpi include it?
> >>
> >
> > I updated the CL to move it under components/. Like Joi said before,
> > adding gypi
> > files in sub-components can cause issues, and probably not desired.
> >
> 
> The reason for that is that source files referenced by a .gypi file are
> interpreted relative to the .gyp file that includes it, not relative to the
> .gypi file.  This Gyp problem results in the convention that if a .gypi
> file references source files, it should only be included from .gyp files in
> the same directory as the .gypi file.
> 
> So, this Gyp problem doesn't apply to your new nacl_defines.gypi file,
> because it doesn't reference any source files.  If it did reference any
> source files, then you wouldn't be able to put nacl_defines.gypi in build/
> (or directly under components/) and include it from chrome/.  So putting it
> in components/nacl/ should be OK too.
> 
> Mark

I see.
Moved the file to components/nacl.

[Mark Seaborn, 2013-07-29 19:41:12]

LGTM

https://codereview.chromium.org/20550003/diff/66001/components/nacl/nacl_defi...
File components/nacl/nacl_defines.gypi (right):

https://codereview.chromium.org/20550003/diff/66001/components/nacl/nacl_defi...
components/nacl/nacl_defines.gypi:14: },],
Nit: write as "}]," rather than "},]," -- the latter looks odd.  Same below.

[yael.aharon1, 2013-07-29 19:48:59]

jam@ can you please review the changes to chrome.gyp ?
thanks

[jam, 2013-07-29 23:01:37]

On 2013/07/29 19:48:59, yael.aharon1 wrote:
> jam@ can you please review the changes to chrome.gyp ?
> thanks

lgtm

[commit-bot: I haz the power, 2013-07-30 03:14:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/20550003/75001

[yael.aharon, 2013-07-30 14:15:40]

Committed patchset #5 manually as r214344 (presubmit successful).

[commit-bot: I haz the power, 2013-07-30 14:36:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/20550003/75001

[commit-bot: I haz the power, 2013-07-30 14:36:10]

Failed to apply patch for chrome/chrome.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome.gyp
  Hunk #1 FAILED at 44.
  Hunk #2 FAILED at 59.
  Hunk #3 FAILED at 87.
  Hunk #4 FAILED at 108.
  Hunk #5 FAILED at 154.
  5 out of 5 hunks FAILED -- saving rejects to file chrome/chrome.gyp.rej

Patch:       chrome/chrome.gyp
Index: chrome/chrome.gyp
diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp
index
b065db8f547132b191b4c7e487e9cf55c948596e..42922e947c0ae43d9fe18d9c46b49546d7923678
100644
--- a/chrome/chrome.gyp
+++ b/chrome/chrome.gyp
@@ -44,11 +44,6 @@
         ],
       }],
       ['OS=="win"', {
-        'nacl_defines': [
-          'NACL_WINDOWS=1',
-          'NACL_LINUX=0',
-          'NACL_OSX=0',
-        ],
         'platform_locale_settings_grd':
             'app/resources/locale_settings_win.grd',
       },],
@@ -59,11 +54,6 @@
         ],
       }],
       ['OS=="linux"', {
-        'nacl_defines': [
-          'NACL_WINDOWS=0',
-          'NACL_LINUX=1',
-          'NACL_OSX=0',
-        ],
         'conditions': [
           ['chromeos==1', {
             'conditions': [
@@ -87,11 +77,6 @@
       },],
       ['OS=="mac"', {
         'tweak_info_plist_path': '../build/mac/tweak_info_plist.py',
-        'nacl_defines': [
-          'NACL_WINDOWS=0',
-          'NACL_LINUX=0',
-          'NACL_OSX=1',
-        ],
         'platform_locale_settings_grd':
             'app/resources/locale_settings_mac.grd',
         'conditions': [
@@ -108,36 +93,6 @@
           }],  # branding
         ],  # conditions
       }],  # OS=="mac"
-      # TODO(mcgrathr): This duplicates native_client/build/common.gypi;
-      # we should figure out a way to unify the settings.
-      ['target_arch=="ia32"', {
-        'nacl_defines': [
-          'NACL_TARGET_SUBARCH=32',
-          'NACL_TARGET_ARCH=x86',
-          'NACL_BUILD_SUBARCH=32',
-          'NACL_BUILD_ARCH=x86',
-        ],
-      }],
-      ['target_arch=="x64"', {
-        'nacl_defines': [
-          'NACL_TARGET_SUBARCH=64',
-          'NACL_TARGET_ARCH=x86',
-          'NACL_BUILD_SUBARCH=64',
-          'NACL_BUILD_ARCH=x86',
-        ],
-      }],
-      ['target_arch=="arm"', {
-        'nacl_defines': [
-          'NACL_BUILD_ARCH=arm',
-          'NACL_BUILD_SUBARCH=32',
-          'NACL_TARGET_ARCH=arm',
-          'NACL_TARGET_SUBARCH=32',
-        ],
-      }],
-      ['target_arch=="mipsel"', {
-        'nacl_defines': [
-        ],
-      }],
     ],  # conditions
   },  # variables
   'includes': [
@@ -154,6 +109,7 @@
     'chrome_installer_util.gypi',
     'chrome_tests_unit.gypi',
     'version.gypi',
+    '../components/nacl/nacl_defines.gypi',
   ],
   'conditions': [
     ['OS!="ios"', {