Compile OpenSSL for NaCl

Compile OpenSSL for NaCl

The new openssl_nacl target compiles OpenSSL for NaCl. It will be used
for chromoting client.

BUG=276739
R=agl@chromium.org, wtc@chromium.org

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

[agl, 2014-01-09 15:52:06]

LGTM, assuming that wtc is aware and happy of this new SSL usage in the
codebase.

[Sergey Ulanov, 2014-01-09 21:52:17]

On 2014/01/09 15:52:06, agl wrote:
> LGTM, assuming that wtc is aware and happy of this new SSL usage in the
> codebase.

Thanks. FYI: it should be possible to switch to NSS in future if necessary.
OpenSSL is just much easier to compile for NaCl than NSS and it's also smaller.

[wtc, 2014-01-09 23:47:50]

Review comments on patch set 1:

Sergey: does this mean Chrome will use both NSS and OpenSSL? That would be code
bloat.

[Sergey Ulanov, 2014-01-09 23:55:36]

On Thu, Jan 9, 2014 at 3:47 PM, <wtc@chromium.org> wrote:

> Review comments on patch set 1:
>
> Sergey: does this mean Chrome will use both NSS and OpenSSL? That would be
> code
> bloat.
>

No this doesn't affect chrome itself. I plan to use the new target to build
chromoting client NaCl plugin which will be shipped in the Chromoting
webapp, not as part of chrome chrome. Moving that plugin to NaCl out of
chrome will actually reduce chrome size.



>
> https://codereview.chromium.org/130373002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wtc, 2014-01-10 00:31:03]

Patch set 1 LGTM.

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp
File openssl_nacl.gyp (right):

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode6
openssl_nacl.gyp:6: 'werror': '',

Just curious: what is 'werror'?

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode27
openssl_nacl.gyp:27: 'openssl.gypi'

In third_party/openssl/openssl.gyp, this line is a (relative) pathname:

        '../../third_party/openssl/openssl.gypi'

Should we change that to the simple filename that you are using here?

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode40
openssl_nacl.gyp:40: 'SSIZE_MAX=INT_MAX',

Just curious: why do you need the following?
    'NO_SYS_UN_H',
    'NO_SYSLOG',
    'TERMIOS',
    'SSIZE_MAX=INT_MAX',

They are not in third_party/openssl/openssl.gyp.

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode62
openssl_nacl.gyp:62: '!pnacl_compile_flags': [

I am not familiar with GYP syntax, but I have never seen a '!' before a variable
name. Should this be 'pnacl_compile_flags!' ?

[Sergey Ulanov, 2014-01-10 01:35:29]

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp
File openssl_nacl.gyp (right):

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode6
openssl_nacl.gyp:6: 'werror': '',
On 2014/01/10 00:31:03, wtc wrote:
> 
> Just curious: what is 'werror'?

It's necessary for untrusted.gypi to remove -werror compiler flags which makes
the compiler treat warnings as errors. Anyway, I updated this defines to make
sure we compile without warnings (there was warning about redefined
_XOPEN_SOURCE), so this is no longer needed. removed it.

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode27
openssl_nacl.gyp:27: 'openssl.gypi'
On 2014/01/10 00:31:03, wtc wrote:
> 
> In third_party/openssl/openssl.gyp, this line is a (relative) pathname:
> 
>         '../../third_party/openssl/openssl.gypi'
> 
> Should we change that to the simple filename that you are using here?

Done.

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode40
openssl_nacl.gyp:40: 'SSIZE_MAX=INT_MAX',
On 2014/01/10 00:31:03, wtc wrote:
> 
> Just curious: why do you need the following?
>     'NO_SYS_UN_H',
>     'NO_SYSLOG',
>     'TERMIOS',
>     'SSIZE_MAX=INT_MAX',
> 
> They are not in third_party/openssl/openssl.gyp.

NaCl toolchain doesn't have some headers that are available to native builds.
E.g. there is no syslog.h (that's why I need NO_SYSLOG). Similarly there is no
TERMIO, but it does have TERMIOS. SSIZE_MAX=INT_MAX is needed because NaCl
headers don't define SSIZE_MAX.

https://codereview.chromium.org/130373002/diff/1/openssl_nacl.gyp#newcode62
openssl_nacl.gyp:62: '!pnacl_compile_flags': [
On 2014/01/10 00:31:03, wtc wrote:
> 
> I am not familiar with GYP syntax, but I have never seen a '!' before a
variable
> name. Should this be 'pnacl_compile_flags!' ?

Yes that was a mistake. I actually meant to write 'pnacl_compile_flags!' here. 
Anyway, the correct thing to do here is to add -Wno-unused-variable in
pnacl_compile_flags . That way it compiles without any warnings.

[Sergey Ulanov, 2014-01-10 01:50:57]

Committed patchset #2 manually as r244037.

[wtc, 2014-01-10 02:06:27]

Patch set 2 LGTM.

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp
File openssl_nacl.gyp (right):

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp#newcode43
openssl_nacl.gyp:43: 'TERMIO',

Why didn't you add the '_XOPEN_SOURCE=600' line here?

[Sergey Ulanov, 2014-01-10 02:10:04]

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp
File openssl_nacl.gyp (right):

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp#newcode43
openssl_nacl.gyp:43: 'TERMIO',
On 2014/01/10 02:06:27, wtc wrote:
> 
> Why didn't you add the '_XOPEN_SOURCE=600' line here?

Because untrusted.gypi adds it in "variables->defines", so it would work if I
added it here :-(

[wtc, 2014-01-10 03:25:42]

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp
File openssl_nacl.gyp (right):

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp#newcode43
openssl_nacl.gyp:43: 'TERMIO',

On 2014/01/10 02:10:04, Sergey Ulanov wrote:
>
> Because untrusted.gypi adds it in "variables->defines", so it would work if I
> added it here :-(

I see. Thanks for the explanation.

I wonder if we just need the macro's name '_XOPEN_SOURCE' when trying to
undefine it.

[Sergey Ulanov, 2014-01-10 05:19:46]

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp
File openssl_nacl.gyp (right):

https://codereview.chromium.org/130373002/diff/100001/openssl_nacl.gyp#newcode43
openssl_nacl.gyp:43: 'TERMIO',
On 2014/01/10 03:25:43, wtc wrote:
> 
> On 2014/01/10 02:10:04, Sergey Ulanov wrote:
> >
> > Because untrusted.gypi adds it in "variables->defines", so it would work if
I
> > added it here :-(
> 
> I see. Thanks for the explanation.
> 
> I wonder if we just need the macro's name '_XOPEN_SOURCE' when trying to
> undefine it.

I don't think that would work. gyp processes defines list just as any other
list, and it doesn't understand the semantics of the list elements (
https://code.google.com/p/gyp/wiki/InputFormatReference#Exclusion_Lists_(!) ).
Thought it should be possible to exclude it using patterns ('defines/'), that
would allow to avoid specifying define value.