[context] Split "env" values and prefixes.

[context] Split "env" values and prefixes.

Currently, "context.env" coalesces the string and prefix values for an
enviornment. This results in the environment flattening prefixes, and
recipes that pull the environment and tweak it having lots of duplicate
prefix values.

Now, we split the "env" property into two properties, "env", which
returns just the environment strings map, and "env_prefixes", which
returns prefix tuples. "env_prefixes" will only be applied when a setep
renders the enviornment to a string, meaning that prefixes will not
accumulate during flattening.

Along these lines, remove improper context handling from "python" module.

BUG=None
TEST=expectations
R=iannucci@chromium.org

Review-Url: https://codereview.chromium.org/2933473002
Committed: https://github.com/luci/recipes-py/commit/d732be8c982f015796f0e620b7b46cff161b137f

[dnj, 2017-06-09 02:52:57]

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

[commit-bot: I haz the power, 2017-06-09 02:53:08]

Dry run: 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, 2017-06-09 02:56:15]

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

[commit-bot: I haz the power, 2017-06-09 02:56:16]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-06-09 04:47:45]

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

https://codereview.chromium.org/2933473002/diff/1/recipe_modules/context/api....
recipe_modules/context/api.py:218: def env_prefixes(self):
I was actually thinking of storing these as separate stacks in context, i.e.
_env_prefixes, and adjusting __call__ appropriately as well.

[dnj, 2017-06-09 04:50:33]

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

https://codereview.chromium.org/2933473002/diff/1/recipe_modules/context/api....
recipe_modules/context/api.py:218: def env_prefixes(self):
On 2017/06/09 04:47:45, iannucci wrote:
> I was actually thinking of storing these as separate stacks in context, i.e.
> _env_prefixes, and adjusting __call__ appropriately as well.

I think that this might be a better API - less things for user to care about. No
user (*) will ever set both via "env=" for the same environment variable, so
keeping the API simple seems advantageous. Are you sold on splitting them?

[dnj, 2017-06-10 16:37:33]

PTAL, updated w/ comments.

"env_prefixes" is now separate, list of Path/str.
It is output as a separate expectation, and not merged into PATH expectation.
When merged via "_merge_envs", we follow:
  - If no prefix, use previous substitution
  - If prefix is specified, resolve original var via previous substitution, and
append if not empty/None.

I also updated "step" to use env_prefixes for forced prefixes.

https://codereview.chromium.org/2933473002/diff/20001/recipe_modules/context/...
File recipe_modules/context/tests/env.py (right):

https://codereview.chromium.org/2933473002/diff/20001/recipe_modules/context/...
recipe_modules/context/tests/env.py:14: # NOTE: This is NOT good practice; we
need to import this to set our
I'm a bit unhappy about this, but it offers perfect environment control, which
is really valuable during this test. Another option is to use something that's
always defined (e.g., PATH, HOME/HOMEDIR, etc.), but:
1) I can't probe for it, or assert it in the test.
2) Changing PATH, notably clearing it, will break the "bash" command, so I think
that one's out.

I *think* this is OK.

[iannucci, 2017-06-12 19:58:40]

lgtm but needs some rebarsing

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

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:261: ('name', 'base_name', 'cmd', 'cwd',
'env_prefixes', 'env', 'pathsep',
pathsep is necessary for simulations I guess?

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:290: nothing will be appeneded:
<prefix0>:...:<prefixN>
"There is currently no way to remove prefix paths; once they're there, they're
there for good. If you think you need to remove paths from the prefix lists,
please talk to infra-dev@chromium.org."

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:353: pathsep=(pathsep or os.pathsep),
this should just be required... os.pathsep might accidentally cause a sadness I
think?

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:369: env_prefixes={k: list(v) for k, v in
self.env_prefixes.iteritems()},
map(str, v)?

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:431: return self[key]
oof, this is starting to get weird; this and __getitem__ don't agree

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:735: if val is _MISSING:
this might be clearer

  val = overrides.get(k, original.get(k, ''))

?

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
File recipe_modules/context/api.py (right):

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:117: def _augment(st, val):
_push

?

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
File recipe_modules/context/tests/env.py (right):

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
recipe_modules/context/tests/env.py:15: # environment expectations against the
runtime enviornment. Other recipes
you could pass in the expected environment as a property from run_test.py
instead

[dnj, 2017-06-13 19:31:21]

Updated, PTAL

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

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:261: ('name', 'base_name', 'cmd', 'cwd',
'env_prefixes', 'env', 'pathsep',
On 2017/06/12 19:58:39, iannucci wrote:
> pathsep is necessary for simulations I guess?

Yeah, for accuracy I think we should use whatever "pathsep" the path module
uses.

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:290: nothing will be appeneded:
<prefix0>:...:<prefixN>
On 2017/06/12 19:58:40, iannucci wrote:
> "There is currently no way to remove prefix paths; once they're there, they're
> there for good. If you think you need to remove paths from the prefix lists,
> please talk to mailto:infra-dev@chromium.org."

Done.

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:353: pathsep=(pathsep or os.pathsep),
On 2017/06/12 19:58:39, iannucci wrote:
> this should just be required... os.pathsep might accidentally cause a sadness
I
> think?

Yeah, but "StepConfig" is called in a lot of places internally to
"recipe_engine" for testing, nesting, phantom steps, etc., and having each site
supply a "pathsep" is weird. I think defaulting to "os.pathsep" is safe; the
"step" module will do the right thing here.

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:369: env_prefixes={k: list(v) for k, v in
self.env_prefixes.iteritems()},
On 2017/06/12 19:58:40, iannucci wrote:
> map(str, v)?

I don't think that'd work. We need this here b/c comparison against JSON will
project tuples into lists. I think this isn't a bad way to write the
translation.

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:431: return self[key]
On 2017/06/12 19:58:40, iannucci wrote:
> oof, this is starting to get weird; this and __getitem__ don't agree

How don't they? This returns __getitem__.

https://codereview.chromium.org/2933473002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:735: if val is _MISSING:
On 2017/06/12 19:58:40, iannucci wrote:
> this might be clearer
> 
>   val = overrides.get(k, original.get(k, ''))
> 
> ?

Can't do that b/c we need to apply subst only if it's in "overrides". I
reorganized a bit though, I think it's more readable.

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
File recipe_modules/context/api.py (right):

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:117: def _augment(st, val):
On 2017/06/12 19:58:40, iannucci wrote:
> _push
> 
> ?

Done.

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
File recipe_modules/context/tests/env.py (right):

https://codereview.chromium.org/2933473002/diff/40001/recipe_modules/context/...
recipe_modules/context/tests/env.py:15: # environment expectations against the
runtime enviornment. Other recipes
On 2017/06/12 19:58:40, iannucci wrote:
> you could pass in the expected environment as a property from run_test.py
> instead

Good idea. Done, and documented.

[iannucci, 2017-06-13 20:28:12]

lgtm

https://codereview.chromium.org/2933473002/diff/80001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_engine/step_runn...
recipe_engine/step_runner.py:724: if not path_tuple:
s/path_tuple/paths

https://codereview.chromium.org/2933473002/diff/80001/recipe_engine/step_runn...
recipe_engine/step_runner.py:728: # If the same key is defined in "overrides",
we need to interact with it.
incorporate?

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
File recipe_modules/context/api.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/api.py:103: "/path/to/my/stuff:%(PATH)s"
nuke this shite

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/api.py:109: TODO(iannucci,dnj): Disallow "env" values to
be mixes of string or
I think the prefix section got disappeared in a rebase?

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/api.py:148: continue
micro optimizaesezeze! (move if not v above str)

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
File recipe_modules/context/examples/full.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/examples/full.py:53: assert result.stdout.strip() ==
expected
maybe undo a thing here or something (not check output)

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/step/api.py
File recipe_modules/step/api.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/step/api...
recipe_modules/step/api.py:186: env_prefixes=env_prefixes,
tuple(env_prefixen, pathsep)

[dnj, 2017-06-13 21:46:27]

https://codereview.chromium.org/2933473002/diff/80001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_engine/step_runn...
recipe_engine/step_runner.py:724: if not path_tuple:
On 2017/06/13 20:28:11, iannucci wrote:
> s/path_tuple/paths

Done.

https://codereview.chromium.org/2933473002/diff/80001/recipe_engine/step_runn...
recipe_engine/step_runner.py:728: # If the same key is defined in "overrides",
we need to interact with it.
On 2017/06/13 20:28:12, iannucci wrote:
> incorporate?

Done.

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
File recipe_modules/context/api.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/api.py:103: "/path/to/my/stuff:%(PATH)s"
On 2017/06/13 20:28:12, iannucci wrote:
> nuke this shite

Done.

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/api.py:109: TODO(iannucci,dnj): Disallow "env" values to
be mixes of string or
On 2017/06/13 20:28:12, iannucci wrote:
> I think the prefix section got disappeared in a rebase?

Done.

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/api.py:148: continue
On 2017/06/13 20:28:12, iannucci wrote:
> micro optimizaesezeze! (move if not v above str)

Done.

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
File recipe_modules/context/examples/full.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/context/...
recipe_modules/context/examples/full.py:53: assert result.stdout.strip() ==
expected
On 2017/06/13 20:28:12, iannucci wrote:
> maybe undo a thing here or something (not check output)

Done.

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/step/api.py
File recipe_modules/step/api.py (right):

https://codereview.chromium.org/2933473002/diff/80001/recipe_modules/step/api...
recipe_modules/step/api.py:186: env_prefixes=env_prefixes,
On 2017/06/13 20:28:12, iannucci wrote:
> tuple(env_prefixen, pathsep)

Done.

[dnj, 2017-06-13 21:48:51]

fix typo

[dnj, 2017-06-13 21:49:06]

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

[commit-bot: I haz the power, 2017-06-13 21:49:11]

Dry run: 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, 2017-06-13 21:52:04]

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

[commit-bot: I haz the power, 2017-06-13 21:52:05]

Dry run: This issue passed the CQ dry run.

[dnj, 2017-06-13 21:52:49]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-06-13 21:52:49]

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/2933473002/#ps120001
(title: "fix typo")

[commit-bot: I haz the power, 2017-06-13 21:52:52]

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, 2017-06-13 21:57:34]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497390769163660,
"parent_rev": "a8816c7cb46f98d0becd31d3338b97088fc10c17", "commit_rev":
"d732be8c982f015796f0e620b7b46cff161b137f"}

[commit-bot: I haz the power, 2017-06-13 21:57:37]

Description was changed from

==========
[context] Split "env" values and prefixes.

Currently, "context.env" coalesces the string and prefix values for an
enviornment. This results in the environment flattening prefixes, and
recipes that pull the environment and tweak it having lots of duplicate
prefix values.

Now, we split the "env" property into two properties, "env", which
returns just the environment strings map, and "env_prefixes", which
returns prefix tuples. "env_prefixes" will only be applied when a setep
renders the enviornment to a string, meaning that prefixes will not
accumulate during flattening.

Along these lines, remove improper context handling from "python" module.

BUG=None
TEST=expectations
R=iannucci@chromium.org
==========

to

==========
[context] Split "env" values and prefixes.

Currently, "context.env" coalesces the string and prefix values for an
enviornment. This results in the environment flattening prefixes, and
recipes that pull the environment and tweak it having lots of duplicate
prefix values.

Now, we split the "env" property into two properties, "env", which
returns just the environment strings map, and "env_prefixes", which
returns prefix tuples. "env_prefixes" will only be applied when a setep
renders the enviornment to a string, meaning that prefixes will not
accumulate during flattening.

Along these lines, remove improper context handling from "python" module.

BUG=None
TEST=expectations
R=iannucci@chromium.org

Review-Url: https://codereview.chromium.org/2933473002
Committed:
https://github.com/luci/recipes-py/commit/d732be8c982f015796f0e620b7b46cff161...
==========

[commit-bot: I haz the power, 2017-06-13 21:57:38]

Committed patchset #7 (id:120001) as
https://github.com/luci/recipes-py/commit/d732be8c982f015796f0e620b7b46cff161...