Re-organize Skia recipes

Re-organize Skia recipes

Break Skia recipe module into:
  - skia_vars: defines and stores variables (eg. paths)
  - skia_step: utilities for running Skia steps
  - skia_flavor: flavor-specific stuff
  - skia: top-level setup, checkout, test/perf steps, etc etc

This establishes a saner dependency structure for the recipes; skia_vars
is at the bottom level, skia_step depends on it, skia_flavor depends on
both of them, skia depends on all of the above, and the recipes
themselves may depend on any or all of them.

Next steps:
  - Merge buildbot_spec into skia_vars
  - Move test_steps and perf_steps from skia recipe_module into
    swarm_test and swarm_perf recipes
  - Cleaner checkout_steps process

BUG=skia:5578
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198173002

Committed: https://skia.googlesource.com/skia/+/1436a09e1fc3be4655af960d4ffb31066bfe4cdd

[borenet2, 2016-08-01 17:29:01]

Description was changed from

==========
Re-organize Skia recipes

Break Skia recipe module into:
  - skia_vars: defines and stores variables (eg. paths)
  - skia_step: utilities for running Skia steps
  - skia_flavor: flavor-specific stuff
  - skia: top-level setup, checkout, test/perf steps, etc etc

This establishes a saner dependency structure for the recipes; skia_vars
is at the bottom level, skia_step depends on it, skia_flavor depends on
both of them, skia depends on all of the above, and the recipes
themselves may depend on any or all of them.

Next steps:
  - Merge buildbot_spec into skia_vars
  - Move test_steps and perf_steps from skia recipe_module into
    swarm_test and swarm_perf recipes
  - Cleaner checkout_steps process

BUG=skia:5578
==========

to

==========
Re-organize Skia recipes

Break Skia recipe module into:
  - skia_vars: defines and stores variables (eg. paths)
  - skia_step: utilities for running Skia steps
  - skia_flavor: flavor-specific stuff
  - skia: top-level setup, checkout, test/perf steps, etc etc

This establishes a saner dependency structure for the recipes; skia_vars
is at the bottom level, skia_step depends on it, skia_flavor depends on
both of them, skia depends on all of the above, and the recipes
themselves may depend on any or all of them.

Next steps:
  - Merge buildbot_spec into skia_vars
  - Move test_steps and perf_steps from skia recipe_module into
    swarm_test and swarm_perf recipes
  - Cleaner checkout_steps process

BUG=skia:5578
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198173002
==========

[borenet, 2016-08-01 17:38:51]

borenet@google.com changed reviewers:
+ borenet@google.com, rmistry@google.com

[borenet, 2016-08-01 17:38:52]

This is a big CL, but note that there are only a couple of trivial simulation
diffs:
- Removed legacy SKP and SkImage test paths, which are unused
- Removed legacy Win Vulkan SDK support, which isn't needed with the recipes
checked into Skia.

[rmistry, 2016-08-02 15:01:50]

High level comments without looking into code yet:

1.
In the CL's description you said:
"""
Break Skia recipe module into:
  - skia_vars: defines and stores variables (eg. paths)
  - skia_step: utilities for running Skia steps
  - skia_flavor: flavor-specific stuff
  - skia: top-level setup, checkout, test/perf steps, etc etc

This establishes a saner dependency structure for the recipes; skia_vars
is at the bottom level, skia_step depends on it, skia_flavor depends on
both of them, skia depends on all of the above, and the recipes
themselves may depend on any or all of them.
"""
It would be useful to reword some of that and put it in a README.md in
infra/bots/recipe_modules/ or add to the README.md in infra/bots/

2.
What do you think about doing:
skia_vars -> vars
skia_step -> step
skia_flavor -> flavor

3.
I know in c-i-t tools repo it had to be recipe_modules/skia/... but now since it
is in our repo could it be something like recipe_modules/core (or a better name)
and recipe_modules/swarming (instead of skia_swarming)
If that is somehow possible then it would look cleaner:
recipe_modules/core/...
recipe_modules/core/vars/...
recipe_modules/core/step/...
recipe_modules/core/flavor/...
recipe_modules/swarming/...

[borenet, 2016-08-02 15:30:23]

On 2016/08/02 15:01:50, rmistry wrote:
> High level comments without looking into code yet:
> 
> 1.
> In the CL's description you said:
> """
> Break Skia recipe module into:
>   - skia_vars: defines and stores variables (eg. paths)
>   - skia_step: utilities for running Skia steps
>   - skia_flavor: flavor-specific stuff
>   - skia: top-level setup, checkout, test/perf steps, etc etc
> 
> This establishes a saner dependency structure for the recipes; skia_vars
> is at the bottom level, skia_step depends on it, skia_flavor depends on
> both of them, skia depends on all of the above, and the recipes
> themselves may depend on any or all of them.
> """
> It would be useful to reword some of that and put it in a README.md in
> infra/bots/recipe_modules/ or add to the README.md in infra/bots/
> 

SGTM, whatever organization we land on, I'll update the README accordingly.

> 2.
> What do you think about doing:
> skia_vars -> vars
> skia_step -> step
> skia_flavor -> flavor
> 

Unfortunately "step" would collide with the built-in module.  Maybe "run"?  The
others would be okay though.

> 3.
> I know in c-i-t tools repo it had to be recipe_modules/skia/... but now since
it
> is in our repo could it be something like recipe_modules/core (or a better
name)
> and recipe_modules/swarming (instead of skia_swarming)
> If that is somehow possible then it would look cleaner:
> recipe_modules/core/...
> recipe_modules/core/vars/...
> recipe_modules/core/step/...
> recipe_modules/core/flavor/...
> recipe_modules/swarming/...

Yeah, I can give that a try.

[rmistry, 2016-08-02 15:33:30]

On 2016/08/02 15:30:23, borenet wrote:
> On 2016/08/02 15:01:50, rmistry wrote:
> > High level comments without looking into code yet:
> > 
> > 1.
> > In the CL's description you said:
> > """
> > Break Skia recipe module into:
> >   - skia_vars: defines and stores variables (eg. paths)
> >   - skia_step: utilities for running Skia steps
> >   - skia_flavor: flavor-specific stuff
> >   - skia: top-level setup, checkout, test/perf steps, etc etc
> > 
> > This establishes a saner dependency structure for the recipes; skia_vars
> > is at the bottom level, skia_step depends on it, skia_flavor depends on
> > both of them, skia depends on all of the above, and the recipes
> > themselves may depend on any or all of them.
> > """
> > It would be useful to reword some of that and put it in a README.md in
> > infra/bots/recipe_modules/ or add to the README.md in infra/bots/
> > 
> 
> SGTM, whatever organization we land on, I'll update the README accordingly.
> 
> > 2.
> > What do you think about doing:
> > skia_vars -> vars
> > skia_step -> step
> > skia_flavor -> flavor
> > 
> 
> Unfortunately "step" would collide with the built-in module.  Maybe "run"? 
The
> others would be okay though.

"run" SGTM

> 
> > 3.
> > I know in c-i-t tools repo it had to be recipe_modules/skia/... but now
since
> it
> > is in our repo could it be something like recipe_modules/core (or a better
> name)
> > and recipe_modules/swarming (instead of skia_swarming)
> > If that is somehow possible then it would look cleaner:
> > recipe_modules/core/...
> > recipe_modules/core/vars/...
> > recipe_modules/core/step/...
> > recipe_modules/core/flavor/...
> > recipe_modules/swarming/...
> 
> Yeah, I can give that a try.

[borenet, 2016-08-02 17:39:12]

On 2016/08/02 15:33:30, rmistry wrote:
> On 2016/08/02 15:30:23, borenet wrote:
> > On 2016/08/02 15:01:50, rmistry wrote:
> > > High level comments without looking into code yet:
> > > 
> > > 1.
> > > In the CL's description you said:
> > > """
> > > Break Skia recipe module into:
> > >   - skia_vars: defines and stores variables (eg. paths)
> > >   - skia_step: utilities for running Skia steps
> > >   - skia_flavor: flavor-specific stuff
> > >   - skia: top-level setup, checkout, test/perf steps, etc etc
> > > 
> > > This establishes a saner dependency structure for the recipes; skia_vars
> > > is at the bottom level, skia_step depends on it, skia_flavor depends on
> > > both of them, skia depends on all of the above, and the recipes
> > > themselves may depend on any or all of them.
> > > """
> > > It would be useful to reword some of that and put it in a README.md in
> > > infra/bots/recipe_modules/ or add to the README.md in infra/bots/
> > > 
> > 
> > SGTM, whatever organization we land on, I'll update the README accordingly.
> > 
> > > 2.
> > > What do you think about doing:
> > > skia_vars -> vars
> > > skia_step -> step
> > > skia_flavor -> flavor
> > > 
> > 
> > Unfortunately "step" would collide with the built-in module.  Maybe "run"? 
> The
> > others would be okay though.
> 
> "run" SGTM
> 
> > 
> > > 3.
> > > I know in c-i-t tools repo it had to be recipe_modules/skia/... but now
> since
> > it
> > > is in our repo could it be something like recipe_modules/core (or a better
> > name)
> > > and recipe_modules/swarming (instead of skia_swarming)
> > > If that is somehow possible then it would look cleaner:
> > > recipe_modules/core/...
> > > recipe_modules/core/vars/...
> > > recipe_modules/core/step/...
> > > recipe_modules/core/flavor/...
> > > recipe_modules/swarming/...
> > 
> > Yeah, I can give that a try.

Okay, did #1 and #2. I wasn't able to get #3 to work.

[rmistry, 2016-08-02 20:17:32]

On 2016/08/02 17:39:12, borenet wrote:
> On 2016/08/02 15:33:30, rmistry wrote:
> > On 2016/08/02 15:30:23, borenet wrote:
> > > On 2016/08/02 15:01:50, rmistry wrote:
> > > > High level comments without looking into code yet:
> > > > 
> > > > 1.
> > > > In the CL's description you said:
> > > > """
> > > > Break Skia recipe module into:
> > > >   - skia_vars: defines and stores variables (eg. paths)
> > > >   - skia_step: utilities for running Skia steps
> > > >   - skia_flavor: flavor-specific stuff
> > > >   - skia: top-level setup, checkout, test/perf steps, etc etc
> > > > 
> > > > This establishes a saner dependency structure for the recipes; skia_vars
> > > > is at the bottom level, skia_step depends on it, skia_flavor depends on
> > > > both of them, skia depends on all of the above, and the recipes
> > > > themselves may depend on any or all of them.
> > > > """
> > > > It would be useful to reword some of that and put it in a README.md in
> > > > infra/bots/recipe_modules/ or add to the README.md in infra/bots/
> > > > 
> > > 
> > > SGTM, whatever organization we land on, I'll update the README
accordingly.
> > > 
> > > > 2.
> > > > What do you think about doing:
> > > > skia_vars -> vars
> > > > skia_step -> step
> > > > skia_flavor -> flavor
> > > > 
> > > 
> > > Unfortunately "step" would collide with the built-in module.  Maybe "run"?

> > The
> > > others would be okay though.
> > 
> > "run" SGTM
> > 
> > > 
> > > > 3.
> > > > I know in c-i-t tools repo it had to be recipe_modules/skia/... but now
> > since
> > > it
> > > > is in our repo could it be something like recipe_modules/core (or a
better
> > > name)
> > > > and recipe_modules/swarming (instead of skia_swarming)
> > > > If that is somehow possible then it would look cleaner:
> > > > recipe_modules/core/...
> > > > recipe_modules/core/vars/...
> > > > recipe_modules/core/step/...
> > > > recipe_modules/core/flavor/...
> > > > recipe_modules/swarming/...
> > > 
> > > Yeah, I can give that a try.
> 
> Okay, did #1 and #2. I wasn't able to get #3 to work.

Out of curiosity, what was the problem? I thought it would be the same as
changing 'skia' in DEPS list in recipes to 'core'.

[borenet, 2016-08-03 11:18:47]

On 2016/08/02 20:17:32, rmistry wrote:
> On 2016/08/02 17:39:12, borenet wrote:
> > On 2016/08/02 15:33:30, rmistry wrote:
> > > On 2016/08/02 15:30:23, borenet wrote:
> > > > On 2016/08/02 15:01:50, rmistry wrote:
> > > > > High level comments without looking into code yet:
> > > > > 
> > > > > 1.
> > > > > In the CL's description you said:
> > > > > """
> > > > > Break Skia recipe module into:
> > > > >   - skia_vars: defines and stores variables (eg. paths)
> > > > >   - skia_step: utilities for running Skia steps
> > > > >   - skia_flavor: flavor-specific stuff
> > > > >   - skia: top-level setup, checkout, test/perf steps, etc etc
> > > > > 
> > > > > This establishes a saner dependency structure for the recipes;
skia_vars
> > > > > is at the bottom level, skia_step depends on it, skia_flavor depends
on
> > > > > both of them, skia depends on all of the above, and the recipes
> > > > > themselves may depend on any or all of them.
> > > > > """
> > > > > It would be useful to reword some of that and put it in a README.md in
> > > > > infra/bots/recipe_modules/ or add to the README.md in infra/bots/
> > > > > 
> > > > 
> > > > SGTM, whatever organization we land on, I'll update the README
> accordingly.
> > > > 
> > > > > 2.
> > > > > What do you think about doing:
> > > > > skia_vars -> vars
> > > > > skia_step -> step
> > > > > skia_flavor -> flavor
> > > > > 
> > > > 
> > > > Unfortunately "step" would collide with the built-in module.  Maybe
"run"?
> 
> > > The
> > > > others would be okay though.
> > > 
> > > "run" SGTM
> > > 
> > > > 
> > > > > 3.
> > > > > I know in c-i-t tools repo it had to be recipe_modules/skia/... but
now
> > > since
> > > > it
> > > > > is in our repo could it be something like recipe_modules/core (or a
> better
> > > > name)
> > > > > and recipe_modules/swarming (instead of skia_swarming)
> > > > > If that is somehow possible then it would look cleaner:
> > > > > recipe_modules/core/...
> > > > > recipe_modules/core/vars/...
> > > > > recipe_modules/core/step/...
> > > > > recipe_modules/core/flavor/...
> > > > > recipe_modules/swarming/...
> > > > 
> > > > Yeah, I can give that a try.
> > 
> > Okay, did #1 and #2. I wasn't able to get #3 to work.
> 
> Out of curiosity, what was the problem? I thought it would be the same as
> changing 'skia' in DEPS list in recipes to 'core'.

I don't think nested modules are supported.  Eg. "core/flavor" didn't work
because "core" isn't in recipe_deps.

[rmistry, 2016-08-03 14:15:16]

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/android_flavor.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/android_flavor.py:24: self.m = m
Nit: self._m = m (since it is not used outside this class).

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/api.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/api.py:77: def setup(self):
Could you check if using constructors in api.py instead of setup works? Eg:
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/swarming/...

This will help automatically instantiate api object's variables atleast to their
default values and make these recipe modules usable by other recipes like
ct_skps and others like it in the future (we have run into this before). I am
not sure how it behaves with the modules in __init__.py, if they do not work
then they will need to be in setup.

If it works then we can try using constructors in other api.py as well, some
might need a setup function to handle special cases but hopefully majority can
have just constructors.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/gn_flavor.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/gn_flavor.py:52: cwd=self.m.vars.skia_dir)
Nit: I think the alignment here and and above needs to be fixed.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/run/api.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/run/api.py:32: def setup(self):
Looks like a constructor should work here.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/skia/__init__.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/skia/__init__.py:13: 'flavor',
Nit: sort flavor

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_compile.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_compile.py:88: #path_config='kitchen',
Commented out?

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_trigger.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_trigger.py:26: 'run',
Nit: sort

[rmistry, 2016-08-03 14:16:30]

On 2016/08/03 11:18:47, borenet wrote:
> On 2016/08/02 20:17:32, rmistry wrote:
> > On 2016/08/02 17:39:12, borenet wrote:
> > > On 2016/08/02 15:33:30, rmistry wrote:
> > > > On 2016/08/02 15:30:23, borenet wrote:
> > > > > On 2016/08/02 15:01:50, rmistry wrote:
> > > > > > High level comments without looking into code yet:
> > > > > > 
> > > > > > 1.
> > > > > > In the CL's description you said:
> > > > > > """
> > > > > > Break Skia recipe module into:
> > > > > >   - skia_vars: defines and stores variables (eg. paths)
> > > > > >   - skia_step: utilities for running Skia steps
> > > > > >   - skia_flavor: flavor-specific stuff
> > > > > >   - skia: top-level setup, checkout, test/perf steps, etc etc
> > > > > > 
> > > > > > This establishes a saner dependency structure for the recipes;
> skia_vars
> > > > > > is at the bottom level, skia_step depends on it, skia_flavor depends
> on
> > > > > > both of them, skia depends on all of the above, and the recipes
> > > > > > themselves may depend on any or all of them.
> > > > > > """
> > > > > > It would be useful to reword some of that and put it in a README.md
in
> > > > > > infra/bots/recipe_modules/ or add to the README.md in infra/bots/
> > > > > > 
> > > > > 
> > > > > SGTM, whatever organization we land on, I'll update the README
> > accordingly.
> > > > > 
> > > > > > 2.
> > > > > > What do you think about doing:
> > > > > > skia_vars -> vars
> > > > > > skia_step -> step
> > > > > > skia_flavor -> flavor
> > > > > > 
> > > > > 
> > > > > Unfortunately "step" would collide with the built-in module.  Maybe
> "run"?
> > 
> > > > The
> > > > > others would be okay though.
> > > > 
> > > > "run" SGTM
> > > > 
> > > > > 
> > > > > > 3.
> > > > > > I know in c-i-t tools repo it had to be recipe_modules/skia/... but
> now
> > > > since
> > > > > it
> > > > > > is in our repo could it be something like recipe_modules/core (or a
> > better
> > > > > name)
> > > > > > and recipe_modules/swarming (instead of skia_swarming)
> > > > > > If that is somehow possible then it would look cleaner:
> > > > > > recipe_modules/core/...
> > > > > > recipe_modules/core/vars/...
> > > > > > recipe_modules/core/step/...
> > > > > > recipe_modules/core/flavor/...
> > > > > > recipe_modules/swarming/...
> > > > > 
> > > > > Yeah, I can give that a try.
> > > 
> > > Okay, did #1 and #2. I wasn't able to get #3 to work.
> > 
> > Out of curiosity, what was the problem? I thought it would be the same as
> > changing 'skia' in DEPS list in recipes to 'core'.
> 
> I don't think nested modules are supported.  Eg. "core/flavor" didn't work
> because "core" isn't in recipe_deps.

Oh I see, I thought you meant that renaming to core did not work. What do you
think about renaming skia -> core since that is our core recipe module and seems
redundant to have it named skia in the skia repo.
So it would be:
recipe_modules/core
recipe_modules/vars
recipe_modules/step
recipe_modules/flavor
recipe_modules/swarming

[borenet, 2016-08-03 14:30:01]

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/android_flavor.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/android_flavor.py:24: self.m = m
On 2016/08/03 14:15:15, rmistry wrote:
> Nit: self._m = m (since it is not used outside this class).

self.m is the convention used throughout all recipe modules for accessing other
modules.  In this case, I think self._m would just be more confusing and add
complication.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/api.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/api.py:77: def setup(self):
On 2016/08/03 14:15:15, rmistry wrote:
> Could you check if using constructors in api.py instead of setup works? Eg:
>
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/swarming/...
> 
> This will help automatically instantiate api object's variables atleast to
their
> default values and make these recipe modules usable by other recipes like
> ct_skps and others like it in the future (we have run into this before). I am
> not sure how it behaves with the modules in __init__.py, if they do not work
> then they will need to be in setup.
> 
> If it works then we can try using constructors in other api.py as well, some
> might need a setup function to handle special cases but hopefully majority can
> have just constructors.

That doesn't work.  The dependencies have not yet been resolved when __init__ is
called, so __init__ can't use self.m.*

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/gn_flavor.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/gn_flavor.py:52: cwd=self.m.vars.skia_dir)
On 2016/08/03 14:15:16, rmistry wrote:
> Nit: I think the alignment here and and above needs to be fixed.

Done.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/run/api.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/run/api.py:32: def setup(self):
On 2016/08/03 14:15:16, rmistry wrote:
> Looks like a constructor should work here.

Done.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/skia/__init__.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/skia/__init__.py:13: 'flavor',
On 2016/08/03 14:15:16, rmistry wrote:
> Nit: sort flavor

Done.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_compile.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_compile.py:88: #path_config='kitchen',
On 2016/08/03 14:15:16, rmistry wrote:
> Commented out?

Fixed.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_trigger.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_trigger.py:26: 'run',
On 2016/08/03 14:15:16, rmistry wrote:
> Nit: sort

Done.

[rmistry, 2016-08-03 14:37:37]

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/android_flavor.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/android_flavor.py:24: self.m = m
On 2016/08/03 14:30:00, borenet wrote:
> On 2016/08/03 14:15:15, rmistry wrote:
> > Nit: self._m = m (since it is not used outside this class).
> 
> self.m is the convention used throughout all recipe modules for accessing
other
> modules.  In this case, I think self._m would just be more confusing and add
> complication.

I noticed that in other recipe_modules but did not think it applied here because
this is an internal wrapper.

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
File infra/bots/recipe_modules/flavor/api.py (right):

https://codereview.chromium.org/2198173002/diff/40001/infra/bots/recipe_modul...
infra/bots/recipe_modules/flavor/api.py:77: def setup(self):
On 2016/08/03 14:30:00, borenet wrote:
> On 2016/08/03 14:15:15, rmistry wrote:
> > Could you check if using constructors in api.py instead of setup works? Eg:
> >
>
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/swarming/...
> > 
> > This will help automatically instantiate api object's variables atleast to
> their
> > default values and make these recipe modules usable by other recipes like
> > ct_skps and others like it in the future (we have run into this before). I
am
> > not sure how it behaves with the modules in __init__.py, if they do not work
> > then they will need to be in setup.
> > 
> > If it works then we can try using constructors in other api.py as well, some
> > might need a setup function to handle special cases but hopefully majority
can
> > have just constructors.
> 
> That doesn't work.  The dependencies have not yet been resolved when __init__
is
> called, so __init__ can't use self.m.*

Yea that sounds familiar, I tried it out a long time ago and did not remember
why it did not work.

[rmistry, 2016-08-03 14:38:42]

I think https://codereview.chromium.org/2198173002/#msg11 is the only comment
left.

[borenet, 2016-08-03 14:48:21]

On 2016/08/03 14:38:42, rmistry wrote:
> I think https://codereview.chromium.org/2198173002/#msg11 is the only comment
> left.

Sorry, I missed that before.  Renamed "skia" -> "core" which caused some
simulation test diffs where "skia" resources (eg. upload_dm_results) became
"core" resources.

[rmistry, 2016-08-03 14:51:17]

lgtm

[borenet, 2016-08-03 14:51:59]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2016-08-03 14:52:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 14:54:11]

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

[commit-bot: I haz the power, 2016-08-03 14:54:13]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[borenet, 2016-08-03 15:00:39]

The CQ bit was checked by borenet@google.com

[borenet, 2016-08-03 15:00:40]

The patchset sent to the CQ was uploaded after l-g-t-m from rmistry@google.com
Link to the patchset: https://codereview.chromium.org/2198173002/#ps120001
(title: "Fix missing dependency")

[commit-bot: I haz the power, 2016-08-03 15:00:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 15:23:13]

Description was changed from

==========
Re-organize Skia recipes

Break Skia recipe module into:
  - skia_vars: defines and stores variables (eg. paths)
  - skia_step: utilities for running Skia steps
  - skia_flavor: flavor-specific stuff
  - skia: top-level setup, checkout, test/perf steps, etc etc

This establishes a saner dependency structure for the recipes; skia_vars
is at the bottom level, skia_step depends on it, skia_flavor depends on
both of them, skia depends on all of the above, and the recipes
themselves may depend on any or all of them.

Next steps:
  - Merge buildbot_spec into skia_vars
  - Move test_steps and perf_steps from skia recipe_module into
    swarm_test and swarm_perf recipes
  - Cleaner checkout_steps process

BUG=skia:5578
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198173002
==========

to

==========
Re-organize Skia recipes

Break Skia recipe module into:
  - skia_vars: defines and stores variables (eg. paths)
  - skia_step: utilities for running Skia steps
  - skia_flavor: flavor-specific stuff
  - skia: top-level setup, checkout, test/perf steps, etc etc

This establishes a saner dependency structure for the recipes; skia_vars
is at the bottom level, skia_step depends on it, skia_flavor depends on
both of them, skia depends on all of the above, and the recipes
themselves may depend on any or all of them.

Next steps:
  - Merge buildbot_spec into skia_vars
  - Move test_steps and perf_steps from skia recipe_module into
    swarm_test and swarm_perf recipes
  - Cleaner checkout_steps process

BUG=skia:5578
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2198173002

Committed:
https://skia.googlesource.com/skia/+/1436a09e1fc3be4655af960d4ffb31066bfe4cdd
==========

[commit-bot: I haz the power, 2016-08-03 15:23:14]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/1436a09e1fc3be4655af960d4ffb31066bfe4cdd