Add big endian support

Add big endian support

Add a script that generates an assembly file from a .dat file. This is
needed for generating big endian assembly file after using icupkg to
convert little endian icudtl.dat to big endian icudtb.dat. Also the
icu.gyp file is modified so big endian architectures use appropriate
files.

BUG=v8:4828

[miran.karic, 2016-05-10 14:19:36]

miran.karic@imgtec.com changed reviewers:
+ gergely.kis@imgtec.com, ivica.bogosavljevic@imgtec.com, jshin@chromium.org

[jungshik at Google, 2016-05-12 08:52:52]

Thank you for the CL. 

In the CL description, 'BUG=None' should be 'BUG=v8:4828'. 

In gyp file, could you make the assembly file generated from the corresponding
dat file by running your script? 

BTW, how about changing the script name to something like make_data_assembly.py
?

https://codereview.chromium.org/1967523002/diff/1/scripts/gen_datS.py
File scripts/gen_datS.py (right):

https://codereview.chromium.org/1967523002/diff/1/scripts/gen_datS.py#newcode16
scripts/gen_datS.py:16: output.write(".globl icudt54_dat\n")
It'd be better to take a major version number as an input parameter (54, 56, 57,
58 .... ) or read off the header portion from ${ICU_SRC}/linux/icudtl_dat.S 

Given that this  script can be used for icudtl_dat.S (instead of checking that
in as we do now) as well as icudtb_dat.S, passing the major version number in
the command line should be fine.

https://codereview.chromium.org/1967523002/diff/1/scripts/gen_datS.py#newcode39
scripts/gen_datS.py:39: for i in range(len(split)):
Can't you just combine this for-loop with the previous one?

[jungshik at Google, 2016-05-12 17:27:15]

https://codereview.chromium.org/1967523002/diff/1/scripts/gen_datS.py
File scripts/gen_datS.py (right):

https://codereview.chromium.org/1967523002/diff/1/scripts/gen_datS.py#newcode16
scripts/gen_datS.py:16: output.write(".globl icudt54_dat\n")
On 2016/05/12 08:52:52, jshin (jungshik at google) wrote:
> It'd be better to take a major version number as an input parameter (54, 56,
57,
> 58 .... ) or read off the header portion from ${ICU_SRC}/linux/icudtl_dat.S 
> 
> Given that this  script can be used for icudtl_dat.S (instead of checking that
> in as we do now) as well as icudtb_dat.S, passing the major version number in
> the command line should be fine. 

Or, a bit hacky approach of reading the major version number from
source/common/unicode/uvernum.h or README.chromium or from icudt[bl].dat   

strings common/icudtl.dat  | grep icudt | head -1 | sed
's/icudt\([5-9][0-9]\)[bl].*$/\1/'

Perhaps, the last is the most self-contained because this script is taking
icudt[bl].dat as an input.

[jungshik at Google, 2016-05-13 22:17:44]

On 2016/05/12 08:52:52, jshin (jungshik at google) wrote:
> Thank you for the CL. 
> 
> In the CL description, 'BUG=None' should be 'BUG=v8:4828'. 
> 
> In gyp file, could you make the assembly file generated from the corresponding
> dat file by running your script? 

FYI: 

http://dev.chromium.org/developers/generated-files (doc) and
https://goo.gl/t4TRR3 (example)


BTW, you don't have to address all my comments. I'm also fine with checking in
your CLs (with two issues addressed:
1. '54' => '56' for the hardcoded major version in the script
2. the script name change). I can build on top of your CL. 

Of course, if you can address all my comments, that's great.

[miran.karic, 2016-05-16 10:45:38]

Description was changed from

==========
Add big endian support

Add a script that generates an assembly file from a .dat file. This is
needed for generating big endian assembly file after using icupkg to
convert little endian icudtl.dat to big endian icudtb.dat. Also the
icu.gyp file is modified so big endian architectures use appropriate
files.

BUG=None
==========

to

==========
Add big endian support

Add a script that generates an assembly file from a .dat file. This is
needed for generating big endian assembly file after using icupkg to
convert little endian icudtl.dat to big endian icudtb.dat. Also the
icu.gyp file is modified so big endian architectures use appropriate
files.

BUG=v8:4828
==========

[miran.karic, 2016-05-16 11:01:31]

On 2016/05/13 22:17:44, jshin (jungshik at google) wrote:
> On 2016/05/12 08:52:52, jshin (jungshik at google) wrote:
> > Thank you for the CL. 
> > 
> > In the CL description, 'BUG=None' should be 'BUG=v8:4828'. 
> > 
> > In gyp file, could you make the assembly file generated from the
corresponding
> > dat file by running your script? 
> 
> FYI: 
> 
> http://dev.chromium.org/developers/generated-files (doc) and
> https://goo.gl/t4TRR3 (example)
> 
> 
> BTW, you don't have to address all my comments. I'm also fine with checking in
> your CLs (with two issues addressed:
> 1. '54' => '56' for the hardcoded major version in the script
> 2. the script name change). I can build on top of your CL. 
> 
> Of course, if you can address all my comments, that's great.

I addressed your comments, except for generating the assembly file in the gyp
file. I will work on addressing this as well. Thank you for the links.

[jungshik at Google, 2016-05-17 00:32:01]

Thank you for the update. Below are my comments.

https://codereview.chromium.org/1967523002/diff/40001/scripts/make_data_assem...
File scripts/make_data_assembly.py (right):

https://codereview.chromium.org/1967523002/diff/40001/scripts/make_data_assem...
scripts/make_data_assembly.py:7: print "Provide input file name as argument to
use the script!"
Instead of a series of nested 'if - else', please exit as soon as you find
something is wrong. 

That is, instead of A, do B. 

A. 
if <cond>
   print ....
else
   if <cond>
      print
   else 
       ....

B. 

if <cond>
  print-error-message or usage
  sys.exit(..)

keep-on-doing ....

https://codereview.chromium.org/1967523002/diff/40001/scripts/make_data_assem...
scripts/make_data_assembly.py:32: output.write("icudt" + version_number +
"_dat:\n")
How about just using a multiline string with a single output.write?

output.write(".globl icudt%s_dat\n"
             "\t.section .note.GNU....."
             ......
             "icudt%s_dat:\n"
             ......    " % (version, version, version, version)

[miran.karic, 2016-05-17 15:38:43]

On 2016/05/17 00:32:01, jshin (jungshik at google) wrote:
> Thank you for the update. Below are my comments.
> 
>
https://codereview.chromium.org/1967523002/diff/40001/scripts/make_data_assem...
> File scripts/make_data_assembly.py (right):
> 
>
https://codereview.chromium.org/1967523002/diff/40001/scripts/make_data_assem...
> scripts/make_data_assembly.py:7: print "Provide input file name as argument to
> use the script!"
> Instead of a series of nested 'if - else', please exit as soon as you find
> something is wrong. 
> 
> That is, instead of A, do B. 
> 
> A. 
> if <cond>
>    print ....
> else
>    if <cond>
>       print
>    else 
>        ....
> 
> B. 
> 
> if <cond>
>   print-error-message or usage
>   sys.exit(..)
> 
> keep-on-doing ....
> 
>
https://codereview.chromium.org/1967523002/diff/40001/scripts/make_data_assem...
> scripts/make_data_assembly.py:32: output.write("icudt" + version_number +
> "_dat:\n")
> How about just using a multiline string with a single output.write?
> 
> output.write(".globl icudt%s_dat\n"
>              "\t.section .note.GNU....."
>              ......
>              "icudt%s_dat:\n"
>              ......    " % (version, version, version, version)

Addressed these comments.

[jungshik at Google, 2016-05-17 17:51:59]

Thank you for the update. A few more comments below. Sorry that I didn't catch
them in the previous round.

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
File scripts/make_data_assembly.py (right):

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:7: sys.exit("Provide input file name as argument
to use the script!");
nit:  Chromium python style is to use 2-space indentation instead of 8.
Oh. you're using <tab>.  Please, replace all the tabs with 2 spaces. 

( http://dev.chromium.org/chromium-os/python-style-guidelines )

And, perhaps, this is clearer? 

sys.exit("Usage: %s icu_data_file" % sys.argv[0]);

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:12: sys.exit("Not a .dat file!");
sys.exit("%s is not a ICU .dat file." % input_file);

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:19: sys.exit("Cannot find version number in the
input file!");
sys.exit("Cannot find a version number in %s." % input_file);

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:25: "\t.section .note.GNU-stack,\"\",%progbits\n"
You can align this and other lines in the string with ".globl. icudt", can't
you?

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:34: split =
[binascii.hexlify(input[i:i+4]).upper().lstrip('0') for i in range(0,
len(input), 4)]
nit: each line has to be 80 chars or fewer. 

split = [binascii.hexlify(input[i:i+4]).upper().lstrip('0')
         for i in range(0, len(input), 4)]

And, to make this work for both little and big endians, you need to 
determine  earlier the endianness (based on the input file name suffix, 'l.dat'
vs 'b.dat', maybe) Then, depending on the endianness, 

input[i:i+4] has to be either input[i:i+4][::1] (big endian) or
input[i:i+4][::-1] (little endian)

[jungshik at Google, 2016-05-17 18:09:52]

A couple of more nits.

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
File scripts/make_data_assembly.py (right):

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:1: #!/usr/bin/env python
Add these 3 lines after the 1st line. 

# Copyright 2016 The Chromium Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

style guide is here: (search for Python) 
https://sites.google.com/a/chromium.org/dev/developers/coding-style

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:6: if len(sys.argv)<2:
nit: Please, use spaces around a binary operator (here and elsewhere). 

if len(sys.argv) < 2: 

( see
https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-state...
)

[jungshik at Google, 2016-05-17 20:33:13]

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
File scripts/make_data_assembly.py (right):

https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
scripts/make_data_assembly.py:14: output_file = input_file[0:n] + "_dat.S"
For use with gyp (automatic generation during a build), this script needs to
accept an explicit output file path. For a local test run, you can keep this
automatic derivation of the output file, but if sys.argv[2] is given, that has
to be used for the output.

[jungshik at Google, 2016-05-17 20:44:21]

I'm adding you to AUTHORS file in the top level directory.

https://codereview.chromium.org/1967523002/

Could you double-check your name and email address? Thanks

[miran.karic, 2016-05-18 12:16:55]

On 2016/05/17 20:33:13, jshin (jungshik at google) wrote:
>
https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
> File scripts/make_data_assembly.py (right):
> 
>
https://codereview.chromium.org/1967523002/diff/60001/scripts/make_data_assem...
> scripts/make_data_assembly.py:14: output_file = input_file[0:n] + "_dat.S"
> For use with gyp (automatic generation during a build), this script needs to
> accept an explicit output file path. For a local test run, you can keep this
> automatic derivation of the output file, but if sys.argv[2] is given, that has
> to be used for the output.

Addressed new comments. Also changed first line in the script which I believe is
now according to the specification. Also added the optional parameter to usage
message. Still working on the gyp file. If I understand correctly I should edit
icu.gyp file to call the script to generate the assembly file for BE as well as
LE, as needed. Both data files, icudtl.dat and icudtb.dat will be checked in
inside common folder?

[jungshik at Google, 2016-05-18 20:51:24]

Thank you for the update. Almost there. 

As for gyp file update, gyp is in a tricky situation atm. v8 cannot be built
with GN (yet) while gyp is being deprecated for Chromium (the transition is not
yet over, though). 

Because v8 is still only built with gyp (and your main motivation is v8), I
think changing gyp is still useful now.

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
File scripts/make_data_assembly.py (right):

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:2: # Copyright 2016 The Chromium Authors. All
rights reserved.
Sorry that I forgot to mention. Please, insert a new line between line 1 and
line 2 (Copyright...).

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:23: if n == -1:
nit: if input_file.find("l.dat") == -1:

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:25: if n == -1:
same here

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:28: step = 1;
You don't need 'else' here because of sys.exit() above.

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:34: if n == -1:
again, just |if input_data.find("icudt") == -1:| would be better.(imho) :-)

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:51: for i in range(0, len(input_data), 4)]
nit: You don't need '\' at the end. 
Please, line up 'for i in' with |binascii.hex....|.

[miran.karic, 2016-05-19 11:46:33]

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
File scripts/make_data_assembly.py (right):

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:2: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> Sorry that I forgot to mention. Please, insert a new line between line 1 and
> line 2 (Copyright...). 

Done.

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:23: if n == -1:
On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> nit: if input_file.find("l.dat") == -1:

Done.

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:25: if n == -1:
On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> same here

Done.

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:28: step = 1;
On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> You don't need 'else' here because of sys.exit() above. 

sys.exit() exits only if there is no 'l' or 'b' before ".dat" in the file name,
still need else to initialize step value for BE and other else for LE. Also
removed semicolons here.

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:34: if n == -1:
On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> again, just |if input_data.find("icudt") == -1:| would be better.(imho) :-) 

Here I use n below to extract version number

https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
scripts/make_data_assembly.py:51: for i in range(0, len(input_data), 4)]
On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> nit: You don't need '\' at the end. 
> Please, line up 'for i in' with |binascii.hex....|. 

Done.

[miran.karic, 2016-05-20 14:36:01]

On 2016/05/19 11:46:33, miran.karic wrote:
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> File scripts/make_data_assembly.py (right):
> 
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> scripts/make_data_assembly.py:2: # Copyright 2016 The Chromium Authors. All
> rights reserved.
> On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> > Sorry that I forgot to mention. Please, insert a new line between line 1 and
> > line 2 (Copyright...). 
> 
> Done.
> 
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> scripts/make_data_assembly.py:23: if n == -1:
> On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> > nit: if input_file.find("l.dat") == -1:
> 
> Done.
> 
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> scripts/make_data_assembly.py:25: if n == -1:
> On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> > same here
> 
> Done.
> 
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> scripts/make_data_assembly.py:28: step = 1;
> On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> > You don't need 'else' here because of sys.exit() above. 
> 
> sys.exit() exits only if there is no 'l' or 'b' before ".dat" in the file
name,
> still need else to initialize step value for BE and other else for LE. Also
> removed semicolons here.
> 
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> scripts/make_data_assembly.py:34: if n == -1:
> On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> > again, just |if input_data.find("icudt") == -1:| would be better.(imho) :-) 
> 
> Here I use n below to extract version number
> 
>
https://codereview.chromium.org/1967523002/diff/80001/scripts/make_data_assem...
> scripts/make_data_assembly.py:51: for i in range(0, len(input_data), 4)]
> On 2016/05/18 20:51:24, jshin (jungshik at google) wrote:
> > nit: You don't need '\' at the end. 
> > Please, line up 'for i in' with |binascii.hex....|. 
> 
> Done.

I changed the gyp file to generate icu assembly files. Currently it generates
both LE and BE files regardless of the target. I still have a few nits I plan to
work on but with these changes v8 compilation is successful and
internationalization tests pass on BE board.

[jungshik at Google, 2016-05-25 08:22:57]

jshin@chromium.org changed reviewers:
+ mark@chromium.org

[jungshik at Google, 2016-05-25 08:22:58]

Thank you for the update. 

Mark, can you take a look? 

Background: To add Intl API support (enable V8_I18N_*) on big endian arch,
icudtb.dat is going to be added. While adding it, we want to get rid of icu data
assembly source files because they can be generated. 

GN does not work for v8, yet. So, gyp file has to be adjusted for v8. 

I'm planning to change BUILD.gn for chromium.

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode113
icu.gyp:113: 'common/icudtb.dat',
FYI: icudtl_dat.S for Mac is slightly different from that for Linux.  For
Android, it has to be generated out of android/icudtl.dat. 

However, I'd not ask you to take care of them in this CL because this CL is for
making v8 support Intl API on big endian. 

OTOH, either  icudtl_dat.S or icudtb_dat.S (but not both) should be generated.

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode125
icu.gyp:125: 'process_outputs_as_sources': 1,
I didn't know about this. This is a neat trick, but perhaps, it's cleaner to put
the output in a build directory.

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode149
icu.gyp:149: 'make_icu_assembly',
Assembly source files should not be generated when icu_use_data_file_flag=1 or
on os==win even though it's  'harmless' except that it slows down things (a few
seconds).

[miran.karic, 2016-05-25 09:24:10]

On 2016/05/25 08:22:58, jshin (slow- 23-26) wrote:
> Thank you for the update. 
> 
> Mark, can you take a look? 
> 
> Background: To add Intl API support (enable V8_I18N_*) on big endian arch,
> icudtb.dat is going to be added. While adding it, we want to get rid of icu
data
> assembly source files because they can be generated. 
> 
> GN does not work for v8, yet. So, gyp file has to be adjusted for v8. 
> 
> I'm planning to change BUILD.gn for chromium.
> 
> https://codereview.chromium.org/1967523002/diff/120001/icu.gyp
> File icu.gyp (right):
> 
> https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode113
> icu.gyp:113: 'common/icudtb.dat',
> FYI: icudtl_dat.S for Mac is slightly different from that for Linux.  For
> Android, it has to be generated out of android/icudtl.dat. 
> 
> However, I'd not ask you to take care of them in this CL because this CL is
for
> making v8 support Intl API on big endian. 
> 
> OTOH, either  icudtl_dat.S or icudtb_dat.S (but not both) should be generated.
> 
> https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode125
> icu.gyp:125: 'process_outputs_as_sources': 1,
> I didn't know about this. This is a neat trick, but perhaps, it's cleaner to
put
> the output in a build directory.
> 
> https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode149
> icu.gyp:149: 'make_icu_assembly',
> Assembly source files should not be generated when icu_use_data_file_flag=1 or
> on os==win even though it's  'harmless' except that it slows down things (a
few
> seconds).

Thank you for the comments. I will look into that next week as I will be out of
office until then.

[miran.karic, 2016-05-31 16:44:10]

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode113
icu.gyp:113: 'common/icudtb.dat',
On 2016/05/25 08:22:58, jungshik at google wrote:
> FYI: icudtl_dat.S for Mac is slightly different from that for Linux.  For
> Android, it has to be generated out of android/icudtl.dat. 
> 
> However, I'd not ask you to take care of them in this CL because this CL is
for
> making v8 support Intl API on big endian. 
> 
> OTOH, either  icudtl_dat.S or icudtb_dat.S (but not both) should be generated.
 

Done.

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode125
icu.gyp:125: 'process_outputs_as_sources': 1,
On 2016/05/25 08:22:58, jungshik at google wrote:
> I didn't know about this. This is a neat trick, but perhaps, it's cleaner to
put
> the output in a build directory. 

I actually meant to remove this. Is this what you had in mind?

https://codereview.chromium.org/1967523002/diff/120001/icu.gyp#newcode149
icu.gyp:149: 'make_icu_assembly',
On 2016/05/25 08:22:58, jungshik at google wrote:
> Assembly source files should not be generated when icu_use_data_file_flag=1 or
> on os==win even though it's  'harmless' except that it slows down things (a
few
> seconds). 
> 

Done.

[miran.karic, 2016-05-31 16:52:08]

Uploaded a new patch set.

[jungshik at Google, 2016-06-02 18:45:42]

Thank you for the update.

https://codereview.chromium.org/1967523002/diff/140001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1967523002/diff/140001/icu.gyp#newcode139
icu.gyp:139: '<@(_data_assembly_outputs)',
"..data.._outputs" should not be in sources, should it?

https://codereview.chromium.org/1967523002/diff/140001/icu.gyp#newcode192
icu.gyp:192: 'data_assembly#target',
for now (it'll be short-lived), this dependency exclusion also has to be done
when OS == "mac" or OS == "ios".

OS == "android" does not matter because Android build (chromium) does not use
gyp any more.

[jungshik at Google, 2016-06-02 18:58:42]

A bit more.

https://codereview.chromium.org/1967523002/diff/140001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1967523002/diff/140001/icu.gyp#newcode121
icu.gyp:121: '<(SHARED_INTERMEDIATE_DIR)/icudtb_dat.S',
Change the output path to
<(SHARED_INTERMEDIATE_DIR)/third_party/icu/icudtb_dat.S
and all of its references have to be updated.

https://codereview.chromium.org/1967523002/diff/140001/icu.gyp#newcode133
icu.gyp:133: '<(SHARED_INTERMEDIATE_DIR)/icudtl_dat.S',
same here.

[miran.karic, 2016-06-03 10:31:03]

On 2016/06/02 18:58:42, jungshik at google wrote:
> A bit more.
> 
> https://codereview.chromium.org/1967523002/diff/140001/icu.gyp
> File icu.gyp (right):
> 
> https://codereview.chromium.org/1967523002/diff/140001/icu.gyp#newcode121
> icu.gyp:121: '<(SHARED_INTERMEDIATE_DIR)/icudtb_dat.S',
> Change the output path to
> <(SHARED_INTERMEDIATE_DIR)/third_party/icu/icudtb_dat.S
> and all of its references have to be updated.
> 
> https://codereview.chromium.org/1967523002/diff/140001/icu.gyp#newcode133
> icu.gyp:133: '<(SHARED_INTERMEDIATE_DIR)/icudtl_dat.S',
> same here.

[miran.karic, 2016-06-03 10:32:23]

All done, PTAL

[miran.karic, 2016-06-14 08:40:37]

Reminder, PTAL.

[miran.karic, 2016-07-19 07:10:33]

Reminder, PTAL.

[jungshik at Google, 2016-07-19 20:18:06]

On 2016/07/19 07:10:33, miran.karic wrote:
> Reminder, PTAL.

Terribly sorry. I didn't forget this. I'll take a look, soon.

[jungshik at Google, 2016-07-19 23:01:30]

https://codereview.chromium.org/1967523002/diff/160001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1967523002/diff/160001/icu.gyp#newcode420
icu.gyp:420: ],
This gave me an error when running v8/gypfiles/gyp_v8. I don't think you need
the above 3 lines. 

Even after removing the above lines, I'm still getting a cryptic error from gyp.
I'm trying to figure it out.

[jungshik at Google, 2016-07-20 00:08:40]

jshin@chromium.org changed reviewers:
+ jochen@chromium.org, machenbach@google.com

[jungshik at Google, 2016-07-20 00:08:40]

With two changes I suggested, ninja file generation works and I was able to
build d8 (with icu_use_data_file set to either 0 or 1) on LE.  

The default is now set to 1 (see https://codereview.chromium.org/18846002 ) in
gypfiles/standalone.gypi. 

That means that icu_dat_copy has to copy icudtb.dat on a BE target. Can you make
that change? 

The above CL went in on June 9. Sorry for a new request.

https://codereview.chromium.org/1967523002/diff/160001/icu.gyp
File icu.gyp (right):

https://codereview.chromium.org/1967523002/diff/160001/icu.gyp#newcode214
icu.gyp:214: 'sources/': [['exclude', 'icudtl_dat', 'icudtb_dat']],
this should read

'sources/': [['exclude', 'icudt[bl]_dat']], 

'/' requires a regex.

[jungshik at Google, 2016-07-20 00:08:45]

With two changes I suggested, ninja file generation works and I was able to
build d8 (with icu_use_data_file set to either 0 or 1) on LE.  

The default is now set to 1 (see https://codereview.chromium.org/18846002 ) in
gypfiles/standalone.gypi. 

That means that icu_dat_copy has to copy icudtb.dat on a BE target. Can you make
that change? 

The above CL went in on June 9. Sorry for a new request.

[jungshik at Google, 2016-07-20 00:15:25]

jochen@, machenbach@: can you take a look? 

I didn't ask Miran to change Mac/Android section in gyp although it's doable by
a slight modification of the python script in the CL. 

For Android, there's no point of changing gyp because Android has completely
migrated to GN build.
 
For Mac, gyp build is still used but it may go away soon. 

My plan: 
1. I'll add icudtb.dat to common for BE
2. Land this CL after review. 
3. Modify the python script to handle Mac and Android  
6. Modify GN build

[jochen (gone - plz use gerrit), 2016-07-20 11:08:38]

note that V8 is also almost done switching to gn...

[Michael Achenbach, 2016-07-20 11:36:55]

If it's not too difficult, I suggest to support gyp on all platforms as well. V8
is switching to gn, but continues gyp support a little while longer. If the CL
lands before deprecation, it might break our v8-deps autoroller.

[jungshik at Google, 2016-07-20 16:42:30]

On 2016/07/20 00:15:25, jungshik at google wrote:
> jochen@, machenbach@: can you take a look? 
> 
> I didn't ask Miran to change Mac/Android section in gyp although it's doable
by
> a slight modification of the python script in the CL. 
> 
> For Android, there's no point of changing gyp because Android has completely
> migrated to GN build.
>  
> For Mac, gyp build is still used but it may go away soon. 
> 
> My plan: 
> 1. I'll add icudtb.dat to common for BE
> 2. Land this CL after review. 
> 3. Modify the python script to handle Mac and Android  
> 6. Modify GN build

I sent this comment out before finishing.  '6' should be replaced by

4. Modify GN build to support all platforms (Android, Win, Linux, Mac) with
build-time generation of assembly
5. Remove assembly source files from the source tree (because they're generated

[jungshik at Google, 2016-07-20 18:08:47]

On 2016/07/20 11:36:55, Michael Achenbach (slow) wrote:
> If it's not too difficult, I suggest to support gyp on all platforms as well.
V8
> is switching to gn, but continues gyp support a little while longer. If the CL
> lands before deprecation, it might break our v8-deps autoroller.

I just realized that I need to land this CL with '-C "Miran K...<...>" because
he can't land it himself. 
So, I'll take care of that. 

A question: Do I need to worry about Android?

[jungshik at Google, 2016-07-20 19:34:59]

On 2016/07/20 18:08:47, jungshik at google wrote:
> On 2016/07/20 11:36:55, Michael Achenbach (slow) wrote:
> > If it's not too difficult, I suggest to support gyp on all platforms as
well.
> V8
> > is switching to gn, but continues gyp support a little while longer. If the
CL
> > lands before deprecation, it might break our v8-deps autoroller.
> 
> I just realized that I need to land this CL with '-C "Miran K...<...>" because
> he can't land it himself. 
> So, I'll take care of that. 

https://codereview.chromium.org/2162393003 is a CL based on this with a couple
of fixes I suggested. 

I'll land that CL and follow it up by a couple of CLs to do what I outlined
above (Mac support, removing assembly source, GN support). 

> A question: Do I need to worry about Android?

In my follow-up CL, I'll just take care of Android, too (although Android does
not use gyp any more).

[Michael Achenbach, 2016-07-21 07:40:33]

On 2016/07/20 19:34:59, jungshik at google wrote:
> On 2016/07/20 18:08:47, jungshik at google wrote:
> > On 2016/07/20 11:36:55, Michael Achenbach (slow) wrote:
> > > If it's not too difficult, I suggest to support gyp on all platforms as
> well.
> > V8
> > > is switching to gn, but continues gyp support a little while longer. If
the
> CL
> > > lands before deprecation, it might break our v8-deps autoroller.
> > 
> > I just realized that I need to land this CL with '-C "Miran K...<...>"
because
> > he can't land it himself. 
> > So, I'll take care of that. 
> 
> https://codereview.chromium.org/2162393003 is a CL based on this with a couple
> of fixes I suggested. 
> 
> I'll land that CL and follow it up by a couple of CLs to do what I outlined
> above (Mac support, removing assembly source, GN support). 
> 
> > A question: Do I need to worry about Android?
> 
> In my follow-up CL, I'll just take care of Android, too (although Android does
> not use gyp any more).

Thanks! We'll switch also Android to gn eventually but we're not there yet. For
Android we won't need to keep gyp support.

[miran.karic, 2016-07-21 11:51:27]

On 2016/07/19 20:18:06, jungshik at google wrote:
> On 2016/07/19 07:10:33, miran.karic wrote:
> > Reminder, PTAL.
> 
> Terribly sorry. I didn't forget this. I'll take a look, soon.

No problem and thank you for the review.

> The default is now set to 1 (see https://codereview.chromium.org/18846002 ) in
> gypfiles/standalone.gypi. 
> 
> That means that icu_dat_copy has to copy icudtb.dat on a BE target. Can you
make
> that change? 
> 
> The above CL went in on June 9. Sorry for a new request.

I don't really understand what do you mean by "icu_dat_copy has to copy
icudtb.dat on a BE target", can you explain? Also could you check that link to
the CL you provided is the correct one?

[jungshik at Google, 2016-07-21 20:59:47]

On 2016/07/21 11:51:27, miran.karic wrote:
> On 2016/07/19 20:18:06, jungshik at google wrote:
> > On 2016/07/19 07:10:33, miran.karic wrote:
> > > Reminder, PTAL.
> > 
> > Terribly sorry. I didn't forget this. I'll take a look, soon.
> 
> No problem and thank you for the review.
> 
> > The default is now set to 1 (see https://codereview.chromium.org/18846002 )
in
> > gypfiles/standalone.gypi. 
> > 
> > That means that icu_dat_copy has to copy icudtb.dat on a BE target. Can you
> make
> > that change? 
> > 
> > The above CL went in on June 9. Sorry for a new request.
> 
> I don't really understand what do you mean by "icu_dat_copy has to copy
> icudtb.dat on a BE target", can you explain? Also could you check that link to
> the CL you provided is the correct one?

You meant my CL (= your CL + my review comments addressed)?  It's at 
https://codereview.chromium.org/2162393003 (the url given before was correct
afaict :-) ) and I'm gonna land it (credited to you). 

As for icu_data_copy target, v8's default has changed to
'icu_use_data_file_flag=1', which means there's no need for assembly source (in
the default settings). 'icudatb.dat' has to be copied, though. That I took care
of my CL mentioned above.

[miran.karic, 2016-07-22 09:59:43]

On 2016/07/21 20:59:47, jungshik at google wrote:
> On 2016/07/21 11:51:27, miran.karic wrote:
> > On 2016/07/19 20:18:06, jungshik at google wrote:
> > > On 2016/07/19 07:10:33, miran.karic wrote:
> > > > Reminder, PTAL.
> > > 
> > > Terribly sorry. I didn't forget this. I'll take a look, soon.
> > 
> > No problem and thank you for the review.
> > 
> > > The default is now set to 1 (see https://codereview.chromium.org/18846002
)
> in
> > > gypfiles/standalone.gypi. 
> > > 
> > > That means that icu_dat_copy has to copy icudtb.dat on a BE target. Can
you
> > make
> > > that change? 
> > > 
> > > The above CL went in on June 9. Sorry for a new request.
> > 
> > I don't really understand what do you mean by "icu_dat_copy has to copy
> > icudtb.dat on a BE target", can you explain? Also could you check that link
to
> > the CL you provided is the correct one?
> 
> You meant my CL (= your CL + my review comments addressed)?  It's at 
> https://codereview.chromium.org/2162393003 (the url given before was correct
> afaict :-) ) and I'm gonna land it (credited to you). 
> 
> As for icu_data_copy target, v8's default has changed to
> 'icu_use_data_file_flag=1', which means there's no need for assembly source
(in
> the default settings). 'icudatb.dat' has to be copied, though. That I took
care
> of my CL mentioned above.

I see it all now. Awesome! :) Thanks a lot.

[jungshik at Google, 2016-08-18 07:43:41]

I'm closing this CL because it's moved to another CL  and updated and landed.

Thanks again for the CL !