Setup basic Runtime with properties and platform.

Setup basic Runtime with properties and platform.

Create the recipe engine Runtime class, which contains runtime-global
recipe engine state. Currently, that consists of the initial property
set and the operating platform.

Integrate Runtime into "run_recipe".

Standardize on platform semantics, exposing it to the "platform" recipe
module via the new PlatformClient recipe engine client.

BUG=chromium:628770
TEST=unit

[dnj, 2016-10-13 01:06:29]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org, martiniss@chromium.org

[dnj, 2016-10-13 01:06:29]

PTAL. This is an initial effort to isolate global state.

My intention is to add a LogDog instance to "Runtime" when appropriate.

[dnj, 2016-10-13 01:08:23]

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py
File recipe_engine/run.py (left):

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#oldcod...
recipe_engine/run.py:203: # NOTE(iannucci): 'root' was a terribly bad idea and
has been replaced by
Talked with iannucci@, probably don't need this.

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#oldcod...
recipe_engine/run.py:209: # TODO(iannucci): A much better way to do this would
be to dynamically
Transplanted into "recipes.py", closer to other properties setup.

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/unittests/run...
File recipe_engine/unittests/run_test.py (left):

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/unittests/run...
recipe_engine/unittests/run_test.py:146: api._engine = mock.Mock()
This was actually vestigial from before "_engine" was replaced with explicit
clients.

[iannucci, 2016-10-15 00:39:45]

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#newcod...
recipe_engine/run.py:213: properties_to_print.pop('use_mirror', None)
I would ditch this

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#newcod...
recipe_engine/run.py:318: # TODO(iannucci): come up with a more structured way
to advertise/set mode
I think this isn't used any more. you can delete this whole section. I think the
proto arguments completely supercede this.

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api.py
File recipe_modules/platform/api.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api...
recipe_modules/platform/api.py:30: return self.name == 'linux'
these should probably pass-through too?

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api...
recipe_modules/platform/api.py:37: self._test_data.get('name', 'linux'))
Hm... I think you need to move this up a level; if someone starts importing the
platform client directly, and they mock the os with api.platform, they'll get
conflicting values in their test. This probably implies having the ability to
specify test clients...

https://codereview.chromium.org/2415793003/diff/1/unittests/doc_test.py
File unittests/doc_test.py (right):

https://codereview.chromium.org/2415793003/diff/1/unittests/doc_test.py#newco...
unittests/doc_test.py:19: 'doc']
debugging?

[dnj, 2016-10-15 00:59:27]

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#newcod...
recipe_engine/run.py:213: properties_to_print.pop('use_mirror', None)
On 2016/10/15 00:39:45, iannucci wrote:
> I would ditch this

altogether, or this specific change?

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#newcod...
recipe_engine/run.py:318: # TODO(iannucci): come up with a more structured way
to advertise/set mode
On 2016/10/15 00:39:45, iannucci wrote:
> I think this isn't used any more. you can delete this whole section. I think
the
> proto arguments completely supercede this.

Done.

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api.py
File recipe_modules/platform/api.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api...
recipe_modules/platform/api.py:30: return self.name == 'linux'
On 2016/10/15 00:39:45, iannucci wrote:
> these should probably pass-through too?

Well, my thought is that at this point the strings are standardized, so I don't
mind adding module-use functionality here, since recipe engine doesn't need
these checks.

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api...
recipe_modules/platform/api.py:37: self._test_data.get('name', 'linux'))
On 2016/10/15 00:39:45, iannucci wrote:
> Hm... I think you need to move this up a level; if someone starts importing
the
> platform client directly, and they mock the os with api.platform, they'll get
> conflicting values in their test. This probably implies having the ability to
> specify test clients...

Yeah. I think that would be a good manifestation of test clients.

ATM though since those don't exist, I think this is fine? Someone importing
clients directly means they need to know how those clients function.

https://codereview.chromium.org/2415793003/diff/1/unittests/doc_test.py
File unittests/doc_test.py (right):

https://codereview.chromium.org/2415793003/diff/1/unittests/doc_test.py#newco...
unittests/doc_test.py:19: 'doc']
On 2016/10/15 00:39:45, iannucci wrote:
> debugging? 

yarp

[iannucci, 2016-10-15 01:04:09]

lgtm

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#newcod...
recipe_engine/run.py:213: properties_to_print.pop('use_mirror', None)
On 2016/10/15 00:59:27, dnj wrote:
> On 2016/10/15 00:39:45, iannucci wrote:
> > I would ditch this
> 
> altogether, or this specific change?

Well you're moving the other property stuff in this change... I would actually
feel a bit more comfortable if the deletion of the property-massaging code was a
separate CL (before or after this one)

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api.py
File recipe_modules/platform/api.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_modules/platform/api...
recipe_modules/platform/api.py:37: self._test_data.get('name', 'linux'))
On 2016/10/15 00:59:27, dnj wrote:
> On 2016/10/15 00:39:45, iannucci wrote:
> > Hm... I think you need to move this up a level; if someone starts importing
> the
> > platform client directly, and they mock the os with api.platform, they'll
get
> > conflicting values in their test. This probably implies having the ability
to
> > specify test clients...
> 
> Yeah. I think that would be a good manifestation of test clients.
> 
> ATM though since those don't exist, I think this is fine? Someone importing
> clients directly means they need to know how those clients function.

fair enough, but I wouldn't leave this gap open too long. Otherwise we'll be
scratching our heads at this in 6 months....

[dnj, 2016-10-15 15:42:05]

PTAL, made some more changes that seemed reasonable in this context.

Namely, the Runtime stores and (by default) returns the frozen properties. It is
capable of returning non-frozen properties via deepcopy on request.

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/2415793003/diff/1/recipe_engine/run.py#newcod...
recipe_engine/run.py:213: properties_to_print.pop('use_mirror', None)
On 2016/10/15 01:04:09, iannucci wrote:
> On 2016/10/15 00:59:27, dnj wrote:
> > On 2016/10/15 00:39:45, iannucci wrote:
> > > I would ditch this
> > 
> > altogether, or this specific change?
> 
> Well you're moving the other property stuff in this change... I would actually
> feel a bit more comfortable if the deletion of the property-massaging code was
a
> separate CL (before or after this one)

Done.

[dnj, 2016-10-17 21:17:35]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-10-17 21:17:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2415793003/#ps40001
(title: "Split out, more immutables, better utilization.")

[commit-bot: I haz the power, 2016-10-17 21:17:45]

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-10-17 21:24:14]

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

[commit-bot: I haz the power, 2016-10-17 21:24:15]

Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31ed539482d23610)

[iannucci, 2016-10-19 16:41:03]

not lgtm, as I'm not comfortable with the addition of mutable properties without
additional explanation. Maybe you want to expose a `write_property(key, value)`
for adding write-only property support (like we have in the recipes currently
via annotations?

https://codereview.chromium.org/2415793003/diff/40001/recipe_engine/recipe_ap...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/2415793003/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:102: def mutable_properties(self):
this seems really dangerous to me; we've never exposed a mutable properties
dictionary to user code before. We didn't do this specifically to prevent people
from using it as a loosely coupled global variable mechanism (which they used
properties for previous to recipes).

Why do we need this?

[dnj, 2016-10-19 16:44:05]

https://codereview.chromium.org/2415793003/diff/40001/recipe_engine/recipe_ap...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/2415793003/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:102: def mutable_properties(self):
On 2016/10/19 16:41:03, iannucci wrote:
> this seems really dangerous to me; we've never exposed a mutable properties
> dictionary to user code before. We didn't do this specifically to prevent
people
> from using it as a loosely coupled global variable mechanism (which they used
> properties for previous to recipes).
> 
> Why do we need this?

The mutable properties function returns a deepcopy, so this should be 100% safe.

[iannucci, 2016-10-19 16:47:50]

nvm, lgtm. I just got confused by the terminology. It's a mutable copy, not a
direct line to the mutable properties state. Do you need to thaw the whole
thing, or are you copying an already-thawed version? thaw on a frozen dictionary
will essentially copy it too.

[iannucci, 2016-10-19 16:48:53]

On 2016/10/19 16:47:50, iannucci wrote:
> nvm, lgtm. I just got confused by the terminology. It's a mutable copy, not a
> direct line to the mutable properties state. Do you need to thaw the whole
> thing, or are you copying an already-thawed version? thaw on a frozen
dictionary
> will essentially copy it too.

Looks like copying an already-thawed version. I would probably recommend just
returning the thaw of the frozen one.

[dnj, 2016-10-19 21:31:48]

On 2016/10/19 16:48:53, iannucci wrote:
> On 2016/10/19 16:47:50, iannucci wrote:
> > nvm, lgtm. I just got confused by the terminology. It's a mutable copy, not
a
> > direct line to the mutable properties state. Do you need to thaw the whole
> > thing, or are you copying an already-thawed version? thaw on a frozen
> dictionary
> > will essentially copy it too.
> 
> Looks like copying an already-thawed version. I would probably recommend just
> returning the thaw of the frozen one.

I am copying the already-thawed version b/c freeze/thaw converts lists to
tuples, so any recipes that made assumptions about the list-ness of the
properties that it got from "api.properties" would be broken.

[martiniss, 2017-01-10 19:13:50]

is this going to land?