Refactor generate engine manifest to accept blacklist as an argument.

Refactor generate engine manifest to accept blacklist as an argument.

The current manifest generation script assumes a static blacklist, which means it can not be generalized to also support manifest generation for chromium unittests to be used for environment testing deployment.

Updating the script to accept a blacklist as an argument generalizes it so it can be used to generate manifests for both the engine and environment unittests.

BUG=608479
Committed: https://crrev.com/384cd04b36e53b48e237e601e804ee65d65e2924
Cr-Commit-Position: refs/heads/master@{#394576}

[Jess, 2016-05-02 21:38:31]

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

[Jess, 2016-05-02 21:38:31]

My thought is the refactor generate-engine-manifest to work for both the engine
and the environmental unittests by accepting target, whitelist, and blacklist
parameters.

This cr only covers moving the currently hardcoded blacklist to be an argument
and updating documentation, but I wanted to do a quick concept validation and
behavior review before proceeding with the refactor.

[Sriram, 2016-05-02 23:16:51]

On 2016/05/02 21:38:31, Jess wrote:
> My thought is the refactor generate-engine-manifest to work for both the
engine
> and the environmental unittests by accepting target, whitelist, and blacklist
> parameters.
> 
> This cr only covers moving the currently hardcoded blacklist to be an argument
> and updating documentation, but I wanted to do a quick concept validation and
> behavior review before proceeding with the refactor.

LGTM.

As for concept - my understanding is that there is no whitelist as everything
that
is part of the output directory when the target is built (except that blacklist)
will be included.

[nyquist, 2016-05-03 00:24:57]

On 2016/05/02 23:16:51, Sriram wrote:
> On 2016/05/02 21:38:31, Jess wrote:
> > My thought is the refactor generate-engine-manifest to work for both the
> engine
> > and the environmental unittests by accepting target, whitelist, and
blacklist
> > parameters.
> > 
> > This cr only covers moving the currently hardcoded blacklist to be an
argument
> > and updating documentation, but I wanted to do a quick concept validation
and
> > behavior review before proceeding with the refactor.
> 
> LGTM.
> 
> As for concept - my understanding is that there is no whitelist as everything
> that
> is part of the output directory when the target is built (except that
blacklist)
> will be included.

I'm not sure I understand this comment. When the target is built, a lot of
things are in the out/ directory.
How we find the dependencies is by running 'gn desc out/foo blimp_engine_app
runtime_deps //blimp/engine'. This does not include everything in the
out-directory.

[maniscalco, 2016-05-03 21:57:54]

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
File blimp/engine/engine-manifest.txt (right):

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
blimp/engine/engine-manifest.txt:20: Mojo
Mojo is repeated several times... something seems odd here.  Looks like the
generation of the manifest isn't working properly anymore (this is probably why
some of the trybots are failing).  Might be best to not regen the manifest as
part of this change.  Or maybe add these new items to the blacklist?

[nyquist, 2016-05-03 22:44:59]

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

[nyquist, 2016-05-03 22:45:00]

Also; could you expand the CL description to be more helpful?

For example have one paragraph for each of these points:
1. How did this work before?
2. What was the issue with that?
3. What does this CL do to remedy that?

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
File blimp/engine/engine-manifest.txt (right):

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
blimp/engine/engine-manifest.txt:20: Mojo
On 2016/05/03 21:57:54, maniscalco wrote:
> Mojo is repeated several times... something seems odd here.  Looks like the
> generation of the manifest isn't working properly anymore (this is probably
why
> some of the trybots are failing).  Might be best to not regen the manifest as
> part of this change.  Or maybe add these new items to the blacklist?

I don't think our bundle needs these. Could you just add */manifest.json to the
blacklist? The same with "Mojo".

There is some wildcard-support in the blacklist, but just verify that none of
these new dependencies sneak in.

[nyquist, 2016-05-05 22:31:49]

https://codereview.chromium.org/1937423002/diff/1/blimp/tools/generate-engine...
File blimp/tools/generate-engine-manifest.py (right):

https://codereview.chromium.org/1937423002/diff/1/blimp/tools/generate-engine...
blimp/tools/generate-engine-manifest.py:60: with open(args.blacklist, 'r') as
blacklist_file:
os.sys.path[0] is the script path. Does removing that always imply a path
relative to the script? I would have thought it would at that point be relative
to the current working directory?
I guess that in fact might be what we want going forward though.

[Jess, 2016-05-09 22:47:05]

Description was changed from

==========
First pass at refactoring generate engine manifest to accept blacklist as an
argument.

BUG=608479
==========

to

==========
Refactor generate engine manifest to accept blacklist as an argument.

The current manifest generation script assumes a static blacklist, which means
it can not be generalized to also support manifest generation for chromium
unittests to be used for environment testing deployment.

Updating the script to accept a blacklist as an argument generalizes it so it
can be used to generate manifests for both the engine and environment unittests.

BUG=608479
==========

[Jess, 2016-05-09 22:47:06]

jessicag@chromium.org changed required reviewers:
+ nyquist@chromium.org

[Jess, 2016-05-09 23:42:55]

Looks like the sanity check resulted in some important discussion.

I've cleaned this up in the suggested ways and updated the description to
something more appropriate for the CQ.

Please review this as change as a check in now, rather than a sanity check
share.

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
File blimp/engine/engine-manifest.txt (right):

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
blimp/engine/engine-manifest.txt:20: Mojo
On 2016/05/03 21:57:54, maniscalco wrote:
> Mojo is repeated several times... something seems odd here.  Looks like the
> generation of the manifest isn't working properly anymore (this is probably
why
> some of the trybots are failing).  Might be best to not regen the manifest as
> part of this change.  Or maybe add these new items to the blacklist?

Agree this behavior is odd which is why I included these changes. From comments
it appears the blacklist should be updated. That change and resulting engine
manifest are reflected in patch set 2.

https://codereview.chromium.org/1937423002/diff/1/blimp/engine/engine-manifes...
blimp/engine/engine-manifest.txt:20: Mojo
On 2016/05/03 22:44:59, nyquist wrote:
> On 2016/05/03 21:57:54, maniscalco wrote:
> > Mojo is repeated several times... something seems odd here.  Looks like the
> > generation of the manifest isn't working properly anymore (this is probably
> why
> > some of the trybots are failing).  Might be best to not regen the manifest
as
> > part of this change.  Or maybe add these new items to the blacklist?
> 
> I don't think our bundle needs these. Could you just add */manifest.json to
the
> blacklist? The same with "Mojo".
> 
> There is some wildcard-support in the blacklist, but just verify that none of
> these new dependencies sneak in.

Acknowledged.

https://codereview.chromium.org/1937423002/diff/1/blimp/tools/generate-engine...
File blimp/tools/generate-engine-manifest.py (right):

https://codereview.chromium.org/1937423002/diff/1/blimp/tools/generate-engine...
blimp/tools/generate-engine-manifest.py:60: with open(args.blacklist, 'r') as
blacklist_file:
On 2016/05/05 22:31:49, nyquist wrote:
> os.sys.path[0] is the script path. Does removing that always imply a path
> relative to the script? I would have thought it would at that point be
relative
> to the current working directory?
> I guess that in fact might be what we want going forward though.

My thought was to ensure that path behavior was consistent between --output and
--blacklist. Right now they are both relative to current working directory.

[maniscalco, 2016-05-10 17:24:47]

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
File blimp/tools/generate-manifest.py (right):

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
blimp/tools/generate-manifest.py:6: '''Generates a list of Blimp target runtime
dependencies.
Naming nit: generate-manifest.py feels a little too generic.  WDTY about
renaming this script to generate-target-manifest.py?  Other ideas?

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
blimp/tools/generate-manifest.py:28: help=('build target of engine'),
The help message references engine.  Does it need to be updated as part of this
change?

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
blimp/tools/generate-manifest.py:49: '# Runtime dependencies for the Blimp
Engine',
Another engine reference.  Maybe the comment should say which target instead of
"the Blimp Engine" ?

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/manifest-bl...
File blimp/tools/manifest-blacklist.txt (right):

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/manifest-bl...
blimp/tools/manifest-blacklist.txt:1: # Listing of filename patterns to be
excluded from the Blimp Docker manifest.
Do you anticipate us having multiple different blacklists?  One for engine, one
for blimp_unittests, etc. ?  Should we rename this file to
engine-manifest-blacklist.txt?

[Jess, 2016-05-18 00:20:00]

jessicag@chromium.org changed reviewers:
+ dpranke@chromium.org

[Jess, 2016-05-18 00:20:02]

Merging in with new BUILD.gn work. PTAL.

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
File blimp/tools/generate-manifest.py (right):

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
blimp/tools/generate-manifest.py:6: '''Generates a list of Blimp target runtime
dependencies.
On 2016/05/10 17:24:47, maniscalco wrote:
> Naming nit: generate-manifest.py feels a little too generic.  WDTY about
> renaming this script to generate-target-manifest.py?  Other ideas?

Done.

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
blimp/tools/generate-manifest.py:28: help=('build target of engine'),
On 2016/05/10 17:24:47, maniscalco wrote:
> The help message references engine.  Does it need to be updated as part of
this
> change?

Obviated by dpranke's updates. Now the runtime deps are directly passed.

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/generate-ma...
blimp/tools/generate-manifest.py:49: '# Runtime dependencies for the Blimp
Engine',
On 2016/05/10 17:24:47, maniscalco wrote:
> Another engine reference.  Maybe the comment should say which target instead
of
> "the Blimp Engine" ?

Replaced with runtime deps file that is now passed.

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/manifest-bl...
File blimp/tools/manifest-blacklist.txt (right):

https://codereview.chromium.org/1937423002/diff/20001/blimp/tools/manifest-bl...
blimp/tools/manifest-blacklist.txt:1: # Listing of filename patterns to be
excluded from the Blimp Docker manifest.
On 2016/05/10 17:24:47, maniscalco wrote:
> Do you anticipate us having multiple different blacklists?  One for engine,
one
> for blimp_unittests, etc. ?  Should we rename this file to
> engine-manifest-blacklist.txt?

Done.

[Dirk Pranke, 2016-05-18 01:28:04]

mostly lgtm.

https://codereview.chromium.org/1937423002/diff/40001/blimp/tools/engine-mani...
File blimp/tools/engine-manifest-blacklist.txt (right):

https://codereview.chromium.org/1937423002/diff/40001/blimp/tools/engine-mani...
blimp/tools/engine-manifest-blacklist.txt:15: Mojo
This last line is odd. I think you're seeing a bunch of "Mojo Application/*"
stuff, and it probably makes sense to blacklist that, but shouldn't you need a
wildcard here or something?

[maniscalco, 2016-05-18 16:14:12]

LGTM % two doc comments

https://codereview.chromium.org/1937423002/diff/40001/blimp/docs/container.md
File blimp/docs/container.md (right):

https://codereview.chromium.org/1937423002/diff/40001/blimp/docs/container.md...
blimp/docs/container.md:49: --output blimp/engine/engine-manifest.txt \
This example needs to be updated, right?  Looks like the new args are
runtime-deps-file, output, and blacklist.

https://codereview.chromium.org/1937423002/diff/40001/blimp/docs/container.md...
blimp/docs/container.md:53: You can compare the output at
`blimp/engine/engine-manifest.txt` with the
We probably don't want to recommend that people direct --output to their source
tree as they may later accidentally check in the manually generated file.  How
about specifying a /tmp path in the --output example?

[Jess, 2016-05-18 17:36:44]

PTAL

https://codereview.chromium.org/1937423002/diff/40001/blimp/docs/container.md
File blimp/docs/container.md (right):

https://codereview.chromium.org/1937423002/diff/40001/blimp/docs/container.md...
blimp/docs/container.md:49: --output blimp/engine/engine-manifest.txt \
On 2016/05/18 16:14:12, maniscalco wrote:
> This example needs to be updated, right?  Looks like the new args are
> runtime-deps-file, output, and blacklist.

Done.

https://codereview.chromium.org/1937423002/diff/40001/blimp/docs/container.md...
blimp/docs/container.md:53: You can compare the output at
`blimp/engine/engine-manifest.txt` with the
On 2016/05/18 16:14:12, maniscalco wrote:
> We probably don't want to recommend that people direct --output to their
source
> tree as they may later accidentally check in the manually generated file.  How
> about specifying a /tmp path in the --output example?

Ended up dropping it in out-linux/Debug since we know that directory exists. I
didn't want to add more directory creation/cleanup here, but I am open to other
options for output.

https://codereview.chromium.org/1937423002/diff/40001/blimp/tools/engine-mani...
File blimp/tools/engine-manifest-blacklist.txt (right):

https://codereview.chromium.org/1937423002/diff/40001/blimp/tools/engine-mani...
blimp/tools/engine-manifest-blacklist.txt:15: Mojo
On 2016/05/18 01:28:03, Dirk Pranke wrote:
> This last line is odd. I think you're seeing a bunch of "Mojo Application/*"
> stuff, and it probably makes sense to blacklist that, but shouldn't you need a
> wildcard here or something?

This no longer appears to be needed.  I think there was a bug in earlier version
of the script where the whitespace in Mojo Application path was resulting in a
split where just "Mojo" was appearing in the manifest.  This is no longer
occuring.

[nyquist, 2016-05-18 21:36:52]

https://codereview.chromium.org/1937423002/diff/80001/blimp/tools/manifest-bl...
File blimp/tools/manifest-blacklist.txt (left):

https://codereview.chromium.org/1937423002/diff/80001/blimp/tools/manifest-bl...
blimp/tools/manifest-blacklist.txt:1: # Listing of filename patterns to be
excluded from the Blimp Docker manifest.
I wonder; this file shows up as "M" for modified in rietveld, insteads of "D".
Is it just the code review tool that is messing with me, or do you need to in
fact git rm the file?

[nyquist, 2016-05-18 22:16:10]

discussed offline. lgtm

[Jess, 2016-05-18 22:16:22]

https://codereview.chromium.org/1937423002/diff/80001/blimp/tools/manifest-bl...
File blimp/tools/manifest-blacklist.txt (left):

https://codereview.chromium.org/1937423002/diff/80001/blimp/tools/manifest-bl...
blimp/tools/manifest-blacklist.txt:1: # Listing of filename patterns to be
excluded from the Blimp Docker manifest.
On 2016/05/18 21:36:52, nyquist wrote:
> I wonder; this file shows up as "M" for modified in rietveld, insteads of "D".
> Is it just the code review tool that is messing with me, or do you need to in
> fact git rm the file?

My guess would be this is tied to the fact I committed edits to this file before
using git mv to rename it.

[Jess, 2016-05-18 22:16:36]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-05-18 22:16:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
sriramsr@chromium.org, maniscalco@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1937423002/#ps80001
(title: "Add missed output location update in doc.")

[commit-bot: I haz the power, 2016-05-18 22:17:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1937423002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1937423002/80001

[commit-bot: I haz the power, 2016-05-18 22:26:18]

Description was changed from

==========
Refactor generate engine manifest to accept blacklist as an argument.

The current manifest generation script assumes a static blacklist, which means
it can not be generalized to also support manifest generation for chromium
unittests to be used for environment testing deployment.

Updating the script to accept a blacklist as an argument generalizes it so it
can be used to generate manifests for both the engine and environment unittests.

BUG=608479
==========

to

==========
Refactor generate engine manifest to accept blacklist as an argument.

The current manifest generation script assumes a static blacklist, which means
it can not be generalized to also support manifest generation for chromium
unittests to be used for environment testing deployment.

Updating the script to accept a blacklist as an argument generalizes it so it
can be used to generate manifests for both the engine and environment unittests.

BUG=608479
==========

[commit-bot: I haz the power, 2016-05-18 22:26:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-18 22:28:06]

Description was changed from

==========
Refactor generate engine manifest to accept blacklist as an argument.

The current manifest generation script assumes a static blacklist, which means
it can not be generalized to also support manifest generation for chromium
unittests to be used for environment testing deployment.

Updating the script to accept a blacklist as an argument generalizes it so it
can be used to generate manifests for both the engine and environment unittests.

BUG=608479
==========

to

==========
Refactor generate engine manifest to accept blacklist as an argument.

The current manifest generation script assumes a static blacklist, which means
it can not be generalized to also support manifest generation for chromium
unittests to be used for environment testing deployment.

Updating the script to accept a blacklist as an argument generalizes it so it
can be used to generate manifests for both the engine and environment unittests.

BUG=608479

Committed: https://crrev.com/384cd04b36e53b48e237e601e804ee65d65e2924
Cr-Commit-Position: refs/heads/master@{#394576}
==========

[commit-bot: I haz the power, 2016-05-18 22:28:08]

Patchset 5 (id:??) landed as
https://crrev.com/384cd04b36e53b48e237e601e804ee65d65e2924
Cr-Commit-Position: refs/heads/master@{#394576}