Build src/crypto for PNaCl

Build src/crypto for PNaCl

The new crypto_nacl target builds crypto for PNaCl.

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

[Sergey Ulanov, 2014-03-05 21:22:06]

rsleevi: ping

[Ryan Sleevi, 2014-03-05 21:43:50]

It mostly looks good, I'm trying to understand the overall approach though.

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp
File crypto/crypto_nacl.gyp (right):

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:12: 'use_openssl': 1,
This really should be global in some other .gypi. I wouldn't want to see this
code duplicated in net/, for example, as it's the wrong layer.

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:20: 'target_name': 'crypto_nacl',
Is it going to be necessary to spin up separate _nacl targets for each of the
first party libraries?

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:46: ['exclude', 'symmetric_key_nss\.cc$'],
Why are you sources/ this, but sources! the rest?

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:48: 'sources!': [
This seems like it's going to be fairly brittle.

[Sergey Ulanov, 2014-03-06 19:37:27]

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp
File crypto/crypto_nacl.gyp (right):

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:12: 'use_openssl': 1,
On 2014/03/05 21:43:50, Ryan Sleevi wrote:
> This really should be global in some other .gypi. I wouldn't want to see this
> code duplicated in net/, for example, as it's the wrong layer.

Updated common.gyp to define USE_OPENSSL when nacl_untrusted_build==1

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:20: 'target_name': 'crypto_nacl',
On 2014/03/05 21:43:50, Ryan Sleevi wrote:
> Is it going to be necessary to spin up separate _nacl targets for each of the
> first party libraries?

Yes, for each library used in remoting. gyp currently doesn't support building
NaCl and native variants of the same target, so we need a separate target.

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:46: ['exclude', 'symmetric_key_nss\.cc$'],
On 2014/03/05 21:43:50, Ryan Sleevi wrote:
> Why are you sources/ this, but sources! the rest?

removed sources!

https://codereview.chromium.org/185233012/diff/40001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:48: 'sources!': [
On 2014/03/05 21:43:50, Ryan Sleevi wrote:
> This seems like it's going to be fairly brittle.

Replaced with regex-based exclusions.

[Ryan Sleevi, 2014-03-06 21:48:02]

https://codereview.chromium.org/185233012/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/185233012/diff/80001/build/common.gypi#newcod...
build/common.gypi:2503: 'defines': ['USE_OPENSSL=1'],
You will need to update DEPS for this - deps/third_party/openssl is only checked
out for Android/Unix.

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto.gypi#newcode9
crypto/crypto.gypi:9: 'hmac_win64_related_sources': [
This needs to be in a nested variables section. GYP does not "guarantee" that
variables are scoped like this (even though in practice it's implemented as
such)

'variables': {
  'variables': {
    'hmac_win64_related_sources': [
    ],
  },
  'crypto_sources': [
  ],
}

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto_nacl.gyp
File crypto/crypto_nacl.gyp (right):

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:19: 'nlib_target': 'libcrypto_nacl.a',
Note: we call this crcrypto to avoid collisions with 'libcrypto' - not sure if
you'll have the same concerns.

[Sergey Ulanov, 2014-03-11 01:07:50]

https://codereview.chromium.org/185233012/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/185233012/diff/80001/build/common.gypi#newcod...
build/common.gypi:2503: 'defines': ['USE_OPENSSL=1'],
On 2014/03/06 21:48:02, Ryan Sleevi wrote:
> You will need to update DEPS for this - deps/third_party/openssl is only
checked
> out for Android/Unix.

Done.

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto.gypi#newcode9
crypto/crypto.gypi:9: 'hmac_win64_related_sources': [
Thanks for catching this! I also had to add the following in the outer
'variables'.
 hmac_win64_related_sources': '<(hmac_win64_related_sources)',

On 2014/03/06 21:48:02, Ryan Sleevi wrote:
> This needs to be in a nested variables section. GYP does not "guarantee" that
> variables are scoped like this (even though in practice it's implemented as
> such)
> 
> 'variables': {
>   'variables': {
>     'hmac_win64_related_sources': [
>     ],
>   },
>   'crypto_sources': [
>   ],
> }

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto_nacl.gyp
File crypto/crypto_nacl.gyp (right):

https://codereview.chromium.org/185233012/diff/80001/crypto/crypto_nacl.gyp#n...
crypto/crypto_nacl.gyp:19: 'nlib_target': 'libcrypto_nacl.a',
On 2014/03/06 21:48:02, Ryan Sleevi wrote:
> Note: we call this crcrypto to avoid collisions with 'libcrypto' - not sure if
> you'll have the same concerns.

For NaCl OpenSSL is compiled as libopenssl_nacl.a, so this shouldn't be a
problem:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

[Ryan Sleevi, 2014-03-11 02:36:57]

lgtm

https://codereview.chromium.org/185233012/diff/120001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/120001/crypto/crypto.gypi#newc...
crypto/crypto.gypi:22: 'hmac_win64_related_sources':
'<(hmac_win64_related_sources)',
You don't need to do this to 'promote' it - this style is only used when
allowing overridable defaults

[Sergey Ulanov, 2014-03-11 18:08:21]

https://codereview.chromium.org/185233012/diff/120001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/120001/crypto/crypto.gypi#newc...
crypto/crypto.gypi:22: 'hmac_win64_related_sources':
'<(hmac_win64_related_sources)',
On 2014/03/11 02:36:58, Ryan Sleevi wrote:
> You don't need to do this to 'promote' it - this style is only used when
> allowing overridable defaults

Done.

[Sergey Ulanov, 2014-03-11 18:08:25]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-03-11 18:13:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/185233012/140001

[commit-bot: I haz the power, 2014-03-11 19:38:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 19:38:49]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Sergey Ulanov, 2014-03-11 20:28:39]

https://codereview.chromium.org/185233012/diff/120001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/120001/crypto/crypto.gypi#newc...
crypto/crypto.gypi:22: 'hmac_win64_related_sources':
'<(hmac_win64_related_sources)',
On 2014/03/11 18:08:21, Sergey Ulanov wrote:
> On 2014/03/11 02:36:58, Ryan Sleevi wrote:
> > You don't need to do this to 'promote' it - this style is only used when
> > allowing overridable defaults
> 
> Done.

Actually it doesn't compile without this line:
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/279768...

[Sergey Ulanov, 2014-03-11 20:28:43]

The CQ bit was checked by sergeyu@chromium.org

[Ryan Sleevi, 2014-03-11 20:52:55]

Blergh. I'll have to dig into GYP to figure out why this isn't working, since
the way it's written isn't how it's supposed to work. I suspect it's some
interaction with .gypi and how that's resolved.

Original LGTM, mod nit re: <@

https://codereview.chromium.org/185233012/diff/150001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/150001/crypto/crypto.gypi#newc...
crypto/crypto.gypi:22: 'hmac_win64_related_sources':
'<(hmac_win64_related_sources)',
This should be an <@(), not <(), to preserve the list nature of the list.

[Sergey Ulanov, 2014-03-11 21:06:12]

The CQ bit was unchecked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-03-11 21:25:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/185233012/150001

[commit-bot: I haz the power, 2014-03-11 21:29:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-11 21:29:52]

Internal error: failed to checkout. Please try again.

[Sergey Ulanov, 2014-03-12 00:56:43]

https://codereview.chromium.org/185233012/diff/150001/crypto/crypto.gypi
File crypto/crypto.gypi (right):

https://codereview.chromium.org/185233012/diff/150001/crypto/crypto.gypi#newc...
crypto/crypto.gypi:22: 'hmac_win64_related_sources':
'<(hmac_win64_related_sources)',
On 2014/03/11 20:52:55, Ryan Sleevi wrote:
> This should be an <@(), not <(), to preserve the list nature of the list.

With <@() I get the following error:
  ValueError: Variable expansion in this context permits str and int only, found
list for hmac_win64_related_sources

Changed it to the following:
 'hmac_win64_related_sources': [ '<@(hmac_win64_related_sources)' ],

[Sergey Ulanov, 2014-03-12 00:56:48]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-03-12 01:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/185233012/170001

[commit-bot: I haz the power, 2014-03-12 09:34:12]

Change committed as 256485