Support for bundling Chromium unittests into a tarball.

Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000, 608487, 608483
Committed: https://crrev.com/c25b6361957f68f9524ff09f2adcfb58d6cae500
Cr-Commit-Position: refs/heads/master@{#398324}

[Jess, 2016-06-02 19:05:49]

jessicag@chromium.org changed reviewers:
+ maniscalco@chromium.org, sriramsr@chromium.org, wez@chromium.org

[maniscalco, 2016-06-02 19:15:56]

Nice.  Here's my feedback.

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile
File blimp/Dockerfile (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile#newcode6
blimp/Dockerfile:6: RUN apt-get update && \
Let's talk about the Dockerfile OOB.  It would be great if we could find a way
to factor this into a base file or image that's shared by both the engine and
test builds.

https://codereview.chromium.org/2028353003/diff/20001/blimp/tools/bundle.py
File blimp/tools/bundle.py (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/tools/bundle.py#n...
blimp/tools/bundle.py:43: help=('startup script to add to the bundle'),
Consider mentioning that this script is optional in the help text.

[Sriram, 2016-06-02 19:55:05]

Looks good except the following.

https://codereview.chromium.org/2028353003/diff/20001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/BUILD.gn#newcode160
blimp/BUILD.gn:160: _blimp_engine_env_tests_manifest,
Should this be _rebased_blimp_engine_env_tests_manifest? It is not clear as when
one would use rebased version vs not.

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile
File blimp/Dockerfile (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile#newcode6
blimp/Dockerfile:6: RUN apt-get update && \
On 2016/06/02 19:15:55, maniscalco wrote:
> Let's talk about the Dockerfile OOB.  It would be great if we could find a way
> to factor this into a base file or image that's shared by both the engine and
> test builds.

+1

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile#newcode13
blimp/Dockerfile:13: RUN mkdir /out/test
could use "mkdir -p" to create all parents as well.

Also any reason to have /out/test instead of say /tests?

[Jess, 2016-06-02 23:02:15]

https://codereview.chromium.org/2028353003/diff/20001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/BUILD.gn#newcode160
blimp/BUILD.gn:160: _blimp_engine_env_tests_manifest,
On 2016/06/02 19:55:05, Sriram wrote:
> Should this be _rebased_blimp_engine_env_tests_manifest? It is not clear as
when
> one would use rebased version vs not.

Nope. That will fail to build.

The issue is that rebasing is being used to set arguments to both the root and
gen dir. The naming convention I copied from engine bundling tracks if a rebase
occurred rather than which root directory a file is part of. That code should
probably be refactored too, but it might be best to tackle that in a stand alone
codereview.

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/languag...

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile
File blimp/Dockerfile (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile#newcode6
blimp/Dockerfile:6: RUN apt-get update && \
On 2016/06/02 19:55:05, Sriram wrote:
> On 2016/06/02 19:15:55, maniscalco wrote:
> > Let's talk about the Dockerfile OOB.  It would be great if we could find a
way
> > to factor this into a base file or image that's shared by both the engine
and
> > test builds.
> 
> +1

I intended this Dockerfile as a stub that would need further work. I've hacked
it down to just comments to make that clearer.

Created http://crbug/616945 to track the broader Dockerfile structuring problem.

https://codereview.chromium.org/2028353003/diff/20001/blimp/Dockerfile#newcode13
blimp/Dockerfile:13: RUN mkdir /out/test
On 2016/06/02 19:55:05, Sriram wrote:
> could use "mkdir -p" to create all parents as well.
> 
> Also any reason to have /out/test instead of say /tests? 

Done.

Mostly because I structured this based on how I could get tests running locally.
 My test binaries were looking for data files 2 directory layers under
themselves.

https://codereview.chromium.org/2028353003/diff/20001/blimp/tools/bundle.py
File blimp/tools/bundle.py (right):

https://codereview.chromium.org/2028353003/diff/20001/blimp/tools/bundle.py#n...
blimp/tools/bundle.py:43: help=('startup script to add to the bundle'),
On 2016/06/02 19:15:55, maniscalco wrote:
> Consider mentioning that this script is optional in the help text.

Done.

[Sriram, 2016-06-02 23:32:16]

lgtm

[maniscalco, 2016-06-03 14:52:17]

LGTM with one nit.

https://codereview.chromium.org/2028353003/diff/80001/blimp/Dockerfile
File blimp/Dockerfile (right):

https://codereview.chromium.org/2028353003/diff/80001/blimp/Dockerfile#newcode2
blimp/Dockerfile:2: # TODO(jessicag): Fill in.
nit: it's common to annotate TODOs with the crbug link.  E.g.

# TODO(jessicag): Fill in (crbug.com/616945).

[Jess, 2016-06-03 17:03:18]

That clears all comments.

Wez, please feel free to delegate OWNER approval if needed. I started with you
since you were familiar with the previous manifest step.

https://codereview.chromium.org/2028353003/diff/80001/blimp/Dockerfile
File blimp/Dockerfile (right):

https://codereview.chromium.org/2028353003/diff/80001/blimp/Dockerfile#newcode2
blimp/Dockerfile:2: # TODO(jessicag): Fill in.
On 2016/06/03 14:52:17, maniscalco wrote:
> nit: it's common to annotate TODOs with the crbug link.  E.g.
> 
> # TODO(jessicag): Fill in (crbug.com/616945).

Done.

[Jess, 2016-06-06 18:57:45]

Description was changed from

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000
==========

to

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487
==========

[Jess, 2016-06-06 18:59:30]

Description was changed from

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487
==========

to

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487,608483
==========

[Wez, 2016-06-06 23:52:15]

https://codereview.chromium.org/2028353003/diff/100001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2028353003/diff/100001/blimp/BUILD.gn#newcode153
blimp/BUILD.gn:153: # and everything in the manifest has been built.
nit: ... everything listed in the ...

https://codereview.chromium.org/2028353003/diff/100001/blimp/BUILD.gn#newcode166
blimp/BUILD.gn:166: # Manually specify the actual arguments to the script.
nit: This comment seems superfluous - isn't this how all action()s are
structured?

https://codereview.chromium.org/2028353003/diff/100001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/2028353003/diff/100001/blimp/engine/BUILD.gn#...
blimp/engine/BUILD.gn:534: script = "//blimp/tools/bundle.py"
nit: This name makes it less obvious whether "bundle" is a verb or noun here -
i.e. whether the script bundles something, or is a helper providing APIs that
work on a "bundle".  Could we call it create-bundle.py or bundle-for-docker.py,
for example?

https://codereview.chromium.org/2028353003/diff/100001/blimp/tools/bundle.py
File blimp/tools/bundle.py (right):

https://codereview.chromium.org/2028353003/diff/100001/blimp/tools/bundle.py#...
blimp/tools/bundle.py:47: help=('manifest for bundle'),
Both the old and new versions of this comment don't actually say what the
manifest is for - can we change this to say "file listing the files to include
in the bundle"?

[Jess, 2016-06-07 00:28:38]

https://codereview.chromium.org/2028353003/diff/100001/blimp/BUILD.gn
File blimp/BUILD.gn (right):

https://codereview.chromium.org/2028353003/diff/100001/blimp/BUILD.gn#newcode153
blimp/BUILD.gn:153: # and everything in the manifest has been built.
On 2016/06/06 23:52:15, Wez wrote:
> nit: ... everything listed in the ...

Done.

https://codereview.chromium.org/2028353003/diff/100001/blimp/BUILD.gn#newcode166
blimp/BUILD.gn:166: # Manually specify the actual arguments to the script.
On 2016/06/06 23:52:15, Wez wrote:
> nit: This comment seems superfluous - isn't this how all action()s are
> structured?

Removed.

https://codereview.chromium.org/2028353003/diff/100001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/2028353003/diff/100001/blimp/engine/BUILD.gn#...
blimp/engine/BUILD.gn:534: script = "//blimp/tools/bundle.py"
On 2016/06/06 23:52:15, Wez wrote:
> nit: This name makes it less obvious whether "bundle" is a verb or noun here -
> i.e. whether the script bundles something, or is a helper providing APIs that
> work on a "bundle".  Could we call it create-bundle.py or
bundle-for-docker.py,
> for example?

Updated to use create-bundle.py

https://codereview.chromium.org/2028353003/diff/100001/blimp/tools/bundle.py
File blimp/tools/bundle.py (right):

https://codereview.chromium.org/2028353003/diff/100001/blimp/tools/bundle.py#...
blimp/tools/bundle.py:47: help=('manifest for bundle'),
On 2016/06/06 23:52:15, Wez wrote:
> Both the old and new versions of this comment don't actually say what the
> manifest is for - can we change this to say "file listing the files to include
> in the bundle"?

Done.

[Wez, 2016-06-07 01:05:54]

lgtm

[Jess, 2016-06-07 16:53:45]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-06-07 16:53:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
sriramsr@chromium.org, maniscalco@chromium.org
Link to the patchset: https://codereview.chromium.org/2028353003/#ps120001
(title: "Updates based on review feedback.")

[commit-bot: I haz the power, 2016-06-07 16:54:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028353003/120001

[commit-bot: I haz the power, 2016-06-07 17:39:07]

Description was changed from

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487,608483
==========

to

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487,608483
==========

[commit-bot: I haz the power, 2016-06-07 17:39:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-07 17:40:34]

Description was changed from

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487,608483
==========

to

==========
Support for bundling Chromium unittests into a tarball.

Without this change only the engine binary can be bundled in this way.

This change allows all the required files to successfully run in a
docker container can be bundled together in a tarball and uploaded to a
new location. This makes the process of exporting tests this way easily
repeatable and automatable.

This builds on the manifest generation work of
https://codereview.chromium.org/1997493002/

BUG=613000,608487,608483

Committed: https://crrev.com/c25b6361957f68f9524ff09f2adcfb58d6cae500
Cr-Commit-Position: refs/heads/master@{#398324}
==========

[commit-bot: I haz the power, 2016-06-07 17:40:35]

Patchset 7 (id:??) landed as
https://crrev.com/c25b6361957f68f9524ff09f2adcfb58d6cae500
Cr-Commit-Position: refs/heads/master@{#398324}