Add support for QNX Neutrino.

Add support for QNX Neutrino.

This patch incorporates a change to source/common/putil.c
fetched from upstream.

BUG=NONE
TEST=icu is built properly on qnx. After DEPS roll, net_unittests:*IDN* and base_unittests:*String* all pass on qnx. 
TBR=darin


Patch by:
Eli Fidler <efidler@blackberry.com>;,
Jeff Rogers <jrogers@blackberry.com>;,
Cosmin Truta <ctruta@blackberry.com>;.

[jochen (gone - plz use gerrit), 2014-02-10 12:32:53]

deferring to Jungshik

[jungshik at Google, 2014-02-11 19:26:45]

Can you make a patch file for putil.h, ucnmv* and ptypes.h and add it to patches
directory? You don't need a patch for pqnx.h. 
Also, update README.chromium file referring to the patch and pqnx.h (being
added). 

Thanks.

https://codereview.chromium.org/141063008/diff/1/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/141063008/diff/1/icu.gyp#newcode128
icu.gyp:128: '(OS == "qnx" and (_toolset == "host" and host_os != "linux"))', {
Would you combine OS==android and OS==qnx as you did for mac/icudtl...S below?  
Well, it's not important and if the line gets too long, you can leave it alone. 

It's not yours, but I got curious about the difference between conditions for
mac/icu*S vs linux/icu*S.

https://codereview.chromium.org/141063008/diff/1/icu.gyp#newcode370
icu.gyp:370: '-licuuc',
Just curious. Are you gonna use system_icu on qnx?

[cosmin.truta, 2014-02-12 22:51:40]

On 2014/02/11 19:26:45, Jungshik Shin wrote:
> Can you make a patch file for putil.h, ucnmv* and ptypes.h and add it
> to patches directory? You don't need a patch for pqnx.h. 

Done. I was actually able to make a small patch for pqnx.h also.

> Also, update README.chromium file referring to the patch and pqnx.h
> (being added). 

Done.

> https://codereview.chromium.org/141063008/diff/1/icu.gyp#newcode128
> icu.gyp:128:
> '(OS == "qnx" and (_toolset == "host" and host_os != "linux"))', {
> Would you combine OS==android and OS==qnx as you did for mac/icudtl...S
> below?  

That wouldn't work, because the condition is different between Android
and QNX (_toolset!="host" for Android, _toolset=="host" for QNX).
For the target toolset, we use linux/icudt46l_dat.S, regardless whether
we build on Linux or Mac.

> It's not yours, but I got curious about the difference between conditions
> for mac/icu*S vs linux/icu*S.

From what I understand, they are logically the same:
'OS == "win" or "mac" or "ios" or...' is like
'OS != "linux and ...', with or's and and's swapped, etc.

They might have been formulated in a more similar manner, except that
now, with this new QNX port, I added an asymmetry: on the Linux host, the
Linux-specific .S file is always used; but on the Mac host, it's using
the Mac-specific .S file for host build and the Linux-specific .S file
for the target build.

> https://codereview.chromium.org/141063008/diff/1/icu.gyp#newcode370
> icu.gyp:370: '-licuuc',
> Just curious. Are you gonna use system_icu on qnx?

We can do both, because QNX ships with ICU. It's too early to make a
decision, though, and for now we keep both options working.

[jungshik at Google, 2014-02-13 00:04:37]

mostly looks good except for 2 points:

1. Do you need XOPEN patch for blackberry? I guess so (otherwise, you'd not have
made a patch), but it's not clear why you need it. If not, can you drop that
patch. 
2. Is 'OS==blackberry' is better than 'OS=qnx' because 'qnx' is not just for
blackberry?

On 2014/02/12 22:51:40, cosmin.truta wrote:
> On 2014/02/11 19:26:45, Jungshik Shin wrote:
> > Can you make a patch file for putil.h, ucnmv* and ptypes.h and add it
> > to patches directory? You don't need a patch for pqnx.h. 
> 
> Done. I was actually able to make a small patch for pqnx.h also.
> 
> > Also, update README.chromium file referring to the patch and pqnx.h
> > (being added). 
> 
> Done.
> 



> > https://codereview.chromium.org/141063008/diff/1/icu.gyp#newcode128
> > icu.gyp:128:
> > '(OS == "qnx" and (_toolset == "host" and host_os != "linux"))', {
> > Would you combine OS==android and OS==qnx as you did for mac/icudtl...S
> > below?  
> 
> That wouldn't work, because the condition is different between Android
> and QNX (_toolset!="host" for Android, _toolset=="host" for QNX).
> For the target toolset, we use linux/icudt46l_dat.S, regardless whether
> we build on Linux or Mac.

Oh, I miss 'and vs or' in qnx vs android. 
BTW, I wonder if 'blackberry' is better than 'qnx' because 'qnx' is not only for
blackberry even though it does not matter right now. 



> > It's not yours, but I got curious about the difference between conditions
> > for mac/icu*S vs linux/icu*S.
> 
> From what I understand, they are logically the same:
> 'OS == "win" or "mac" or "ios" or...' is like
> 'OS != "linux and ...', with or's and and's swapped, etc.

Yes, that's why I was wondering about. Two different 'implementations' for a
single logic :-) 
> 
> They might have been formulated in a more similar manner, except that
> now, with this new QNX port, I added an asymmetry: on the Linux host, the
> Linux-specific .S file is always used; but on the Mac host, it's using
> the Mac-specific .S file for host build and the Linux-specific .S file
> for the target build.

Ok. 
> 
> > https://codereview.chromium.org/141063008/diff/1/icu.gyp#newcode370
> > icu.gyp:370: '-licuuc',
> > Just curious. Are you gonna use system_icu on qnx?
> 
> We can do both, because QNX ships with ICU. It's too early to make a
> decision, though, and for now we keep both options working.

Ok.

[cosmin.truta, 2014-02-13 00:54:55]

> 1. Do you need XOPEN patch for blackberry? I guess so (otherwise,
> you'd not have made a patch), but it's not clear why you need it.
> If not, can you drop that patch. 

Yes, we need that, because QNX requires _XOPEN_SOURCE to either
be at least 500, or left undefined. Without it, the value of
_XOPEN_SOURCE defaults to 4, which breaks compilation.
I could have added an #elif defined(QNX) also, but I decided to
pick that change from upstream instead.

Moreover, I added the _XOPEN_SOURCE_EXTENDED part in Change Set 2,
although this one was not strictly necessary. I can remove it back
if you wish; or, conversely, I can prepare another separate patch
in which to integrate as much as possible from upstream changeset 30418.
(For example, the deletion of platform_xopen_source_extended.h,
the inlining of other _XOPEN_SOURCE_EXTENDED definitions from that
removed header, etc., are still not done. So I can cherry-pick that,
separately and cleanly, and land it here; and then create a QNX patch
on top of that cherry-pick, if you prefer it that way.)

> 2. Is 'OS==blackberry' is better than 'OS=qnx' because 'qnx' is not
> just for blackberry?

You are correct, there are products other than BlackBerry that run on
top of QNX, and we support those officially also. This port relies on
QNX API only, so it is meant to run on all QNX platforms and products.

We are using the name QNX uniformly across all of our ports of Chromium
sub-projects. We use the "BlackBerry" moniker only when we use BB-specific
API.

Here is the QNX API:
http://www.qnx.com/developers/docs/6.5.0/index.jsp
and here is the BlackBerry 10 Native API:
http://developer.blackberry.com/native/documentation/cascades/

[jungshik at Google, 2014-02-13 20:27:28]

Thank you for the reply/explanation. LGTM 

On 2014/02/13 00:54:55, cosmin.truta wrote:
> > 1. Do you need XOPEN patch for blackberry? I guess so (otherwise,
> > you'd not have made a patch), but it's not clear why you need it.
> > If not, can you drop that patch. 
> 
> Yes, we need that, because QNX requires _XOPEN_SOURCE to either
> be at least 500, or left undefined. Without it, the value of
> _XOPEN_SOURCE defaults to 4, which breaks compilation.
> I could have added an #elif defined(QNX) also, but I decided to
> pick that change from upstream instead.
 
> Moreover, I added the _XOPEN_SOURCE_EXTENDED part in Change Set 2,
> although this one was not strictly necessary. I can remove it back
> if you wish; or, conversely, I can prepare another separate patch
> in which to integrate as much as possible from upstream changeset 30418.
> (For example, the deletion of platform_xopen_source_extended.h,
> the inlining of other _XOPEN_SOURCE_EXTENDED definitions from that
> removed header, etc., are still not done. So I can cherry-pick that,
> separately and cleanly, and land it here; and then create a QNX patch
> on top of that cherry-pick, if you prefer it that way.)

As I wrote earlier, we're gonna upgrade to 52/53 soon so that adding more than
absolutely required would lead to more work for me :-) to curate which patch to
keep and which to drop when upgrading ICU to 52/53. So, it'd be nice of you to
keep your patch to the minimum necessary. 


 
> > 2. Is 'OS==blackberry' is better than 'OS=qnx' because 'qnx' is not
> > just for blackberry?
> 
> You are correct, there are products other than BlackBerry that run on
> top of QNX, and we support those officially also. This port relies on
> QNX API only, so it is meant to run on all QNX platforms and products.
> 
> We are using the name QNX uniformly across all of our ports of Chromium
> sub-projects. We use the "BlackBerry" moniker only when we use BB-specific
> API.
> 
> Here is the QNX API:
> http://www.qnx.com/developers/docs/6.5.0/index.jsp
> and here is the BlackBerry 10 Native API:
> http://developer.blackberry.com/native/documentation/cascades/

Ok. Then, just use 'qnx' as you did. thanks again.

[cosmin.truta, 2014-02-13 21:28:37]

> As I wrote earlier, we're gonna upgrade to 52/53 soon so that adding
> more than absolutely required would lead to more work for me :-)
> to curate which patch to keep and which to drop when upgrading ICU
> to 52/53. So, it'd be nice of you to keep your patch to the minimum
> necessary.

Understood.

I undid the change to _XOPEN_SOURCE_EXTENDED, keeping only _XOPEN_SOURCE.
Could you please re-review Patch Set 4; also, could you please land it
for me, since I am not a committer yet?
(BTW, Patch Set 3 failed to upload properly, so you may ignore that one.)

Thank you very much for accommodating our OS and platform :-)

[cosmin.truta, 2014-02-13 21:37:54]

On 2014/02/13 21:28:37, cosmin.truta wrote:
> (BTW, Patch Set 3 failed to upload properly, so you may ignore that one.)

Never mind, I noticed the Delete link at the right of the patch set, so
I deleted the faulty one. The former (good) Patch Set 4 is now Patch Set 3.

[jungshik at Google, 2014-02-15 00:26:21]

On 2014/02/13 21:37:54, cosmin.truta wrote:
> On 2014/02/13 21:28:37, cosmin.truta wrote:
> > (BTW, Patch Set 3 failed to upload properly, so you may ignore that one.)
> 
> Never mind, I noticed the Delete link at the right of the patch set, so
> I deleted the faulty one. The former (good) Patch Set 4 is now Patch Set 3.

Thanks. LGTM. Do you need me to add this CL to CQ or can you?

[cosmin.truta, 2014-02-15 00:29:40]

On 2014/02/15 00:26:21, Jungshik Shin wrote:
> Thanks. LGTM. Do you need me to add this CL to CQ or can you?

Could you please CQ it? I am a few days away from my committer status
-- just not quite there yet :D

[cosmin.truta, 2014-02-15 00:30:35]

Oh, and thank you :-)

[jungshik at Google, 2014-02-15 00:31:24]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2014-02-15 00:31:28]

The CQ bit was unchecked by jshin@chromium.org

[jungshik at Google, 2014-02-15 00:31:52]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2014-02-15 00:33:38]

On 2014/02/15 00:29:40, cosmin.truta wrote:
> On 2014/02/15 00:26:21, Jungshik Shin wrote:
> > Thanks. LGTM. Do you need me to add this CL to CQ or can you?
> 
> Could you please CQ it? I am a few days away from my committer status
> -- just not quite there yet :D

Added to the CQ. After it's landed, you need to roll the icu revision in
src/DEPS to this revision. For that, make a new CL (DEPS roll only) and ask me
for review.

[cosmin.truta, 2014-02-15 00:38:24]

On 2014/02/15 00:33:38, Jungshik Shin wrote:
> Added to the CQ. After it's landed, you need to roll the icu revision in
> src/DEPS to this revision. For that, make a new CL (DEPS roll only) and ask me
> for review.

Will do.

[cmp, 2014-02-15 18:45:36]

On 2014/02/15 00:38:24, cosmin.truta wrote:
> On 2014/02/15 00:33:38, Jungshik Shin wrote:
> > Added to the CQ. After it's landed, you need to roll the icu revision in
> > src/DEPS to this revision. For that, make a new CL (DEPS roll only) and ask
me
> > for review.
> 
> Will do.

Hi, there's no CQ for this base URL path.  You'll need to land this using that
repository's commit directions.

[jungshik at Google, 2014-02-15 19:10:36]

Thanks for the reminder. That is what I always have done for icu. I was
trying sth different this time :-)
On Feb 15, 2014 10:45 AM, <cmp@chromium.org> wrote:

> On 2014/02/15 00:38:24, cosmin.truta wrote:
>
>> On 2014/02/15 00:33:38, Jungshik Shin wrote:
>> > Added to the CQ. After it's landed, you need to roll the icu revision in
>> > src/DEPS to this revision. For that, make a new CL (DEPS roll only) and
>> ask
>>
> me
>
>> > for review.
>>
>
>  Will do.
>>
>
> Hi, there's no CQ for this base URL path.  You'll need to land this using
> that
> repository's commit directions.
>
> https://codereview.chromium.org/141063008/
>

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

[cmp, 2014-02-16 22:01:59]

The CQ bit was unchecked by cmp@chromium.org

[cosmin.truta, 2014-02-21 18:29:26]

Is there any action required on my part, other than updating DEPS
after landing this patch? If landing requires more time, that's fine;
I'm just writing this to be sure I am not inadvertently blocking the
process.

[jungshik at Google, 2014-03-17 22:01:19]

On 2014/02/21 18:29:26, cosmin.truta wrote:
> Is there any action required on my part, other than updating DEPS
> after landing this patch? If landing requires more time, that's fine;
> I'm just writing this to be sure I am not inadvertently blocking the
> process.

Terribly sorry for the delay. It's just landed (
https://src.chromium.org/viewvc/chrome?view=rev&revision=257503 ). I made a
slight change in README.chromium (a reference to the upstream change for
si_value) and icu.gyp (line breaking) and the CL committed is at
https://codereview.chromium.org/195973005/

[cosmin.truta, 2014-03-17 22:35:59]

> Terribly sorry for the delay. It's just landed [...]

You're very kind. No need to apologize at all. We have plenty of other stuff to
do here, so the delay didn't block us. On the contrary, we're very happy that
the necessary code is in, and that Chromium's ICU is now supported on our
platform :D

Sincerely,
Cosmin

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

[jungshik at Google, 2014-03-18 18:50:01]

On 2014/03/17 22:35:59, cosmin.truta wrote:
> > Terribly sorry for the delay. It's just landed [...]
> 
> You're very kind. No need to apologize at all. We have plenty of other stuff
to
> do here, so the delay didn't block us. On the contrary, we're very happy that
> the necessary code is in, and that Chromium's ICU is now supported on our
> platform :D

Good to hear that I didn't block you :-) 
I made a mistake in the previous CL (forget to add pqnx.h. oops). It's being
fixed now 
( https://codereview.chromium.org/203823003/ )