Adds the OCHamcrest library to the iOS build.

Adds the OCHamcrest library to the iOS build.

Adapted from a patch by baxley@chromium.org.

BUG=580730
TEST=None

[rohitrao (ping after 24h), 2016-02-19 21:37:52]

rohitrao@chromium.org changed reviewers:
+ baxley@chromium.org

[baxley, 2016-02-20 00:49:59]

On 2016/02/19 21:37:52, rohitrao wrote:

LGTM, thanks for doing the gyp thing.

Here's a bug for this task: 580730

Here's the original CL, which was referenced and approved by security and
open-source. This was also approved by chrome-eng-reivew as part of pulling in
EarlGrey:
https://codereview.chromium.org/1626583003/

[baxley, 2016-02-20 01:11:51]

https://codereview.chromium.org/1711953003/diff/20001/ios/chrome/tools/build/...
File ios/chrome/tools/build/ios_generate_forwarding_headers.py (right):

https://codereview.chromium.org/1711953003/diff/20001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:28: def
generate_headers(inputs, dest_dir):
Would it make any sense to rm the dest_dir? I know we don't need to re-create
the file if it is already there, but I'm just wondering if there's a case where
stuff moves around or is removed, and we may want to clean up?

I guess it's not that big of a deal, it shouldn't change too much, or be that
many files.

https://codereview.chromium.org/1711953003/diff/20001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:47: return
list_outputs(args.filenames, args.dest_dir)
Does this list get printed?

[rohitrao (ping after 24h), 2016-02-20 01:18:51]

rohitrao@chromium.org changed reviewers:
+ justincohen@google.com

[rohitrao (ping after 24h), 2016-02-20 01:18:52]

Over to Justin for a committer review.

https://codereview.chromium.org/1711953003/diff/20001/ios/chrome/tools/build/...
File ios/chrome/tools/build/ios_generate_forwarding_headers.py (right):

https://codereview.chromium.org/1711953003/diff/20001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:28: def
generate_headers(inputs, dest_dir):
On 2016/02/20 01:11:51, baxley wrote:
> Would it make any sense to rm the dest_dir? I know we don't need to re-create
> the file if it is already there, but I'm just wondering if there's a case
where
> stuff moves around or is removed, and we may want to clean up?
> 
> I guess it's not that big of a deal, it shouldn't change too much, or be that
> many files.

I can't arbitrarily remove dest_dir because then we will regenerate all of the
files every time, making no-op builds impossible.

But it's a good point that headers might move.  We should probably be reading
the contents of each file and making sure they are correct.  I think that can go
in a future CL.

https://codereview.chromium.org/1711953003/diff/20001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:47: return
list_outputs(args.filenames, args.dest_dir)
On 2016/02/20 01:11:51, baxley wrote:
> Does this list get printed?

No, it never gets printed anywhere =)  It actually doesn't matter because
pymod_do_main wants it to be returned as a string, but I'll have the script
print the results if you run the script directly.

[baxley, 2016-02-20 01:24:31]

Sorry, one last comment...

https://codereview.chromium.org/1711953003/diff/20001/ios/third_party/ochamcr...
File ios/third_party/ochamcrest/OWNERS (right):

https://codereview.chromium.org/1711953003/diff/20001/ios/third_party/ochamcr...
ios/third_party/ochamcrest/OWNERS:3: rohitrao@google.com
change these to @chromium.org
and remove me since I'm not a committer?

[rohitrao (ping after 24h), 2016-02-20 01:28:35]

https://codereview.chromium.org/1711953003/diff/20001/ios/third_party/ochamcr...
File ios/third_party/ochamcrest/OWNERS (right):

https://codereview.chromium.org/1711953003/diff/20001/ios/third_party/ochamcr...
ios/third_party/ochamcrest/OWNERS:3: rohitrao@google.com
On 2016/02/20 01:24:31, baxley wrote:
> change these to @chromium.org
> and remove me since I'm not a committer?

Good call.

[rohitrao (ping after 24h), 2016-02-20 01:28:44]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[rohitrao (ping after 24h), 2016-02-20 01:30:09]

Description was changed from

==========
Adds the OCHamcrest library to the iOS build.

BUG=None
TEST=None
==========

to

==========
Adds the OCHamcrest library to the iOS build.

Adapted from a patch by baxley@chromium.org.

BUG=580730
TEST=None
==========

[commit-bot: I haz the power, 2016-02-20 01:34:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711953003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711953003/40001

[commit-bot: I haz the power, 2016-02-20 02:51:27]

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

[commit-bot: I haz the power, 2016-02-20 02:51:28]

Dry run: This issue passed the CQ dry run.

[baxley, 2016-02-21 06:33:47]

https://codereview.chromium.org/1711953003/diff/40001/ios/ios.gyp
File ios/ios.gyp (right):

https://codereview.chromium.org/1711953003/diff/40001/ios/ios.gyp#newcode25
ios/ios.gyp:25: 'third_party/ochamcrest/ochamcrest.gyp:*',
Do we need this dependency here? Or will EarlGrey simply depend on it and we'll
just have to make sure whatever depends on EarlGrey (web_shell, web_shell_tests)
is here?

[sdefresne, 2016-02-22 09:09:49]

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

[sdefresne, 2016-02-22 09:09:49]

https://codereview.chromium.org/1711953003/diff/40001/ios/chrome/tools/build/...
File ios/chrome/tools/build/ios_generate_forwarding_headers.py (right):

https://codereview.chromium.org/1711953003/diff/40001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:30: forwarding_header
= get_output_filename(filename, dest_dir)
I would change the code to the following. It will overwrite the file if the
content change, and do nothing otherwise, thus allowing no-op builds:

    content = COPYRIGHT + '#import "{0}"\n'.format(filename)
    forwarding_header = get_output_filename(filename, dest_dir)
    if os.path.isfile(forwarding_header):
      with open(forwarding_header, 'rb') as header:
        old_content = header.read()
      if old_content == content:
        return
    with open(forwarding_header, 'wb') as header:
      header.write(content)

https://codereview.chromium.org/1711953003/diff/40001/ios/ios.gyp
File ios/ios.gyp (right):

https://codereview.chromium.org/1711953003/diff/40001/ios/ios.gyp#newcode25
ios/ios.gyp:25: 'third_party/ochamcrest/ochamcrest.gyp:*',
On 2016/02/21 at 06:33:47, baxley wrote:
> Do we need this dependency here? Or will EarlGrey simply depend on it and
we'll just have to make sure whatever depends on EarlGrey (web_shell,
web_shell_tests) is here?

We want it here until EG depends on it otherwise it is not going to be build and
could break.

[rohitrao (ping after 24h), 2016-02-22 14:49:09]

The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run

[rohitrao (ping after 24h), 2016-02-22 14:49:38]

sdefresne, can you take another look?  Are you comfortable giving the committer
LGTM on this CL?

https://codereview.chromium.org/1711953003/diff/40001/ios/chrome/tools/build/...
File ios/chrome/tools/build/ios_generate_forwarding_headers.py (right):

https://codereview.chromium.org/1711953003/diff/40001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:30: forwarding_header
= get_output_filename(filename, dest_dir)
On 2016/02/22 09:09:49, sdefresne wrote:
> I would change the code to the following. It will overwrite the file if the
> content change, and do nothing otherwise, thus allowing no-op builds:
> 
>     content = COPYRIGHT + '#import "{0}"\n'.format(filename)
>     forwarding_header = get_output_filename(filename, dest_dir)
>     if os.path.isfile(forwarding_header):
>       with open(forwarding_header, 'rb') as header:
>         old_content = header.read()
>       if old_content == content:
>         return
>     with open(forwarding_header, 'wb') as header:
>       header.write(content)

Done.

[commit-bot: I haz the power, 2016-02-22 14:49:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711953003/60001

[sdefresne, 2016-02-22 15:43:35]

Can you explain why we want to have forwarding-headers?

All other libraries that we use just configure the include_dirs property,
optionally add it to dependent_settings. I don't understand why ochamrest needs
to be treated differently there. Is this because of our downstream usage of
Google libraries that assume ochamrest is at a certain path (does not look like
so since we put the forwarding-header in almost the same structure as the
original headers)?

[sdefresne, 2016-02-22 15:45:43]

On 2016/02/22 at 15:43:35, sdefresne wrote:
> Can you explain why we want to have forwarding-headers?
> 
> All other libraries that we use just configure the include_dirs property,
optionally add it to dependent_settings. I don't understand why ochamrest needs
to be treated differently there. Is this because of our downstream usage of
Google libraries that assume ochamrest is at a certain path (does not look like
so since we put the forwarding-header in almost the same structure as the
original headers)?

Another data point for my question: this is not something we can do with gn
(using pymod_do_main) and there we'll have to list manually all inputs and
outputs. I would really like to avoid listing each file twice unless it was
absolutely unavoidable.

[sdefresne, 2016-02-22 16:36:49]

OK, I think I understand why we need to process the headers. As discuss outside
of the codereview tool, OCHamcrest files requires to be included as a framework.
I think building a static framework should not be too hard, but as a first step,
I would suggest copying the files instead of creating forwarding headers.

Two reasons for that:
1. copying files is easy to express with gn
2. copying files is semantically closer to what will happen in creating a
framework

I've suggested edits that would result in copying the files, and as a followup
I'll see how we can build a proper framework (to build a static framework, we
would instead just list the files into mac_bundle_resources property if gyp were
to support non-executable/non-loadable module frameworks which is not the case).

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
File ios/chrome/tools/build/ios_generate_forwarding_headers.py (right):

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:1: #!/usr/bin/env
python
Rename to copy_files.py

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:7: import datetime
Remove.

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:9: import sys
import shutil

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:11: COPYRIGHT = """//
Copyright {0} The Chromium Authors. All rights reserved.
Remove.

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:16: def
filter_header_files(inputs):
Remove.

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:22: def
list_inputs(possible_inputs):
Remove.

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:25: def
list_outputs(possible_inputs, dest_dir):
s/possible_inputs/inputs/

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:26: outputs = []
outputs = [ get_output_filename(filename, dest_dir) for filename in inputs ]

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:31: def
generate_headers(possible_inputs, dest_dir):
Remove and replace with:

def copy_files(inputs, dest_dir):
  for filename in inputs:
    shutil.copy(filename, dest_dir)

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:45:
parser.add_argument('-i', '--list-inputs', action='store_true',
Remove '-i' argument.

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:55: if
args.list_inputs:
Remove.

https://codereview.chromium.org/1711953003/diff/80001/ios/chrome/tools/build/...
ios/chrome/tools/build/ios_generate_forwarding_headers.py:66:
generate_headers(args.filenames, args.dest_dir)
copy_files(args.filenames, args.dest_dir)

https://codereview.chromium.org/1711953003/diff/80001/ios/third_party/ochamcr...
File ios/third_party/ochamcrest/ochamcrest.gyp (right):

https://codereview.chromium.org/1711953003/diff/80001/ios/third_party/ochamcr...
ios/third_party/ochamcrest/ochamcrest.gyp:8: 'ochamcrest_sources': [
split into 'ochamrest_sources' & 'ochamrest_headers', i.e.:

'ochamcrest_headers': [
  'src/Source/Core/HCAssertThat.h',
  'src/Source/Core/HCBaseDescription.h',
  ...
],
'ochamcrest_sources': [
  '<@(ochamrest_headers)',
  'src/Source/Core/HCAssertThat.m',
  'src/Source/Core/HCBaseDescription.m',
],

https://codereview.chromium.org/1711953003/diff/80001/ios/third_party/ochamcr...
ios/third_party/ochamcrest/ochamcrest.gyp:188: 'target_name':
'ochamcrest_forwarding_headers',
I don't think we need the script, but instead we can just copy the files. I
would do the following (note this is assuming that we do not yet have a script
to copy files to a directory):

'actions': [
  {
    'action_name': 'copy headers',
    'variables': {
      'script_path': '../../chrome/tools/build/copy_files.py',
    },
    'inputs': [ '<(script_path)', '<@(ochamcrest_headers)' ]
    'outputs': [ '<!@pymod_do_main(copy_files -o --dest-dir <(output_dir)
<(ochamrest_headers))' ],
    'action': [
      'python',
      '<(script_path)',
      '--dest-dir',
      '<(output_dir)'
      '<@(ochamcrest_headers)',
    ],
  },

Then the equivalent gn code will be (BTW, I recommend also renaming the gyp
target to have the same name):

copy("ochamcrest_copy_headers") {
  inputs = ochamcrest_sources
  outputs = [ "${output_dir}/{{source_file_part}}" ]
}

https://codereview.chromium.org/1711953003/diff/80001/ios/third_party/ochamcr...
ios/third_party/ochamcrest/ochamcrest.gyp:189: 'type': 'none',
You need "'hard_dependency': 1" here otherwise build will be flaky.