Only call CopyStringsFile if convert_to_binary is False.

Only call CopyStringsFile if convert_to_binary is False.

When convert_to_binary is True, there is no point converting the .strings
file encoding to UTF-16 as the conversion will be performed by "plutil".
This fix compilation failure when the input files are already in "binary1"
property list format.

BUG=625578
R=justincohen@chromium.org

Committed: https://chromium.googlesource.com/external/gyp/+/bac4680ec9a5c55ab692490b6732999648ecf1e9

[sdefresne, 2016-07-05 13:00:17]

sdefresne@chromium.org changed reviewers:
+ justincohen@chromium.org

[sdefresne, 2016-07-05 13:00:18]

Please take a look.

[sdefresne, 2016-07-05 13:50:20]

Description was changed from

==========
Only call CopyStringsFile if convert_to_binary is False.

When convert_to_binary is True, there is no point converting the .strings
file encoding to UTF-16 as the conversion will be performed by "plutil".

BUG=625578
==========

to

==========
Only call CopyStringsFile if convert_to_binary is False.

When convert_to_binary is True, there is no point converting the .strings
file encoding to UTF-16 as the conversion will be performed by "plutil".
This fix compilation failure when the input files are already in "binary1"
property list format.

BUG=625578
==========

[justincohen, 2016-07-05 15:36:26]

How is the file copied when convert to binary is true?

[sdefresne, 2016-07-05 16:52:14]

https://codereview.chromium.org/2117353002/diff/1/pylib/gyp/mac_tool.py
File pylib/gyp/mac_tool.py (right):

https://codereview.chromium.org/2117353002/diff/1/pylib/gyp/mac_tool.py#newco...
pylib/gyp/mac_tool.py:71: shutil.copy(source, dest)
The file is copied here (i.e. in the else clause).

[justincohen, 2016-07-06 09:18:06]

https://codereview.chromium.org/2117353002/diff/1/pylib/gyp/mac_tool.py
File pylib/gyp/mac_tool.py (right):

https://codereview.chromium.org/2117353002/diff/1/pylib/gyp/mac_tool.py#newco...
pylib/gyp/mac_tool.py:71: shutil.copy(source, dest)
Will this create a lot of churn? I wonder why we delete before copy here and not
on line 67?

On 2016/07/05 16:52:14, sdefresne wrote:
> The file is copied here (i.e. in the else clause).

[sdefresne, 2016-07-06 09:28:06]

https://codereview.chromium.org/2117353002/diff/1/pylib/gyp/mac_tool.py
File pylib/gyp/mac_tool.py (right):

https://codereview.chromium.org/2117353002/diff/1/pylib/gyp/mac_tool.py#newco...
pylib/gyp/mac_tool.py:71: shutil.copy(source, dest)
On 2016/07/06 09:18:05, justincohen wrote:
> Will this create a lot of churn? I wonder why we delete before copy here and
not
> on line 67?
> 
> On 2016/07/05 16:52:14, sdefresne wrote:
> > The file is copied here (i.e. in the else clause).
> 

I don't know why we do not delete first for the other target. I do not think it
would create too much additional work as the majority of the files copied in
application bundle are not special and already fall in that last "else".

For ".strings" file, it will remove work, as the current code does read the file
as a plist, re-read the file as a plain data, convert from UTF-8 to UTF-8 and
save as dest, then invoke "plutil" to convert it to binary1 property list file.

After my change, it will remove the file if it exists, copy it using shutil.copy
without doing any modification and then use "plutil" to convert it to binary1
property list. This is mostly used for the InfoPlist.strings and
Localizable.strings file generated by generate_localizable_strings step in
Chromium build.

I can probably remove the copy and directly use "plutil" to do the copy (since
it supports an explicit output parameter), but this will be a bigger change to
this code.

The goal of this CL is to allow supporting .strings file already in binary1
format in which case the plutil call will be a noop (as plutil is smart enough
to avoid doing anything then). It is also required for compatibility with gn (as
we want to generate .strings in binary1 format directly and just not convert
them when copying to the application bundle).

[justincohen, 2016-07-06 14:59:09]

LGTM

[sdefresne, 2016-07-06 15:11:23]

Description was changed from

==========
Only call CopyStringsFile if convert_to_binary is False.

When convert_to_binary is True, there is no point converting the .strings
file encoding to UTF-16 as the conversion will be performed by "plutil".
This fix compilation failure when the input files are already in "binary1"
property list format.

BUG=625578
==========

to

==========
Only call CopyStringsFile if convert_to_binary is False.

When convert_to_binary is True, there is no point converting the .strings
file encoding to UTF-16 as the conversion will be performed by "plutil".
This fix compilation failure when the input files are already in "binary1"
property list format.

BUG=625578
R=justincohen@chromium.org

Committed:
https://chromium.googlesource.com/external/gyp/+/bac4680ec9a5c55ab692490b6732...
==========

[sdefresne, 2016-07-06 15:11:25]

Committed patchset #1 (id:1) manually as
bac4680ec9a5c55ab692490b6732999648ecf1e9 (presubmit successful).