[context] Add path prefix/suffix manipulation.

[context] Add path prefix/suffix manipulation.

Add the ability to manipulate pathsep-delimited environment variables
more easily with specific helper functions targeting the prefixing and
suffixing of values to those variables.

Previously, the supported way to augment path environment variables was
to manually construct them, referencing the current value:

new_path = "%s%%(PATH)s%s" % (api.path.pathsep, api.path.pathsep)

Not only is this very verbose, but it is also error-prone, as the user
may not use OS-specific path separators, can add the same entries
multiple times, and can forget the double-percent to escape the
inner substitition.

This patch adds the concept of path prefixes and suffixes to the
"context" recipe module. Path elements can be added to either, and will
be compiled into properly-delimited prefix and suffix values on the
context's environment.

BUG=None
TEST=expectations,run

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

[dnj, 2017-06-03 15:35:22]

advise

[dnj, 2017-06-03 15:35:36]

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

[dnj, 2017-06-03 15:35:37]

PTAL. Seemed like a good idea.

[dnj, 2017-06-03 15:37:34]

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

https://codereview.chromium.org/2925453002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:710: result[str(k)] = str(v) % subst
This fixes a bug where an environment variable that is referenced (e.g.,
"IDONTEXIST") will actually cause a KeyError instead of just being substituted
as an empty string for real runs. I think we're just lucky so far that most
referenced environment variables are "PATH" / "PYTHONPATH", and that both are
always defined.

[dnj, 2017-06-03 20:55:30]

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

[commit-bot: I haz the power, 2017-06-03 20:55:39]

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-03 20:58:25]

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

[commit-bot: I haz the power, 2017-06-03 20:58:26]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-06-05 19:23:05]

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

https://codereview.chromium.org/2925453002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:710: result[str(k)] = str(v) % subst
On 2017/06/03 15:37:33, dnj wrote:
> This fixes a bug where an environment variable that is referenced (e.g.,
> "IDONTEXIST") will actually cause a KeyError instead of just being substituted
> as an empty string for real runs. I think we're just lucky so far that most
> referenced environment variables are "PATH" / "PYTHONPATH", and that both are
> always defined.

Yeah, and with this CL I'd like to eventually eliminate the substitution
entirely (so that it's a straight assign/delete, and the pre/suffix path stuff).

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

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:78: * env (dict) - Environmental variable
overrides. See below for more info.
wdyt about something like:

  with api.context(env={"PATH": api.context.Prefix(a, b, c)}):

Then we don't need to additional kwargs, just Prefix and Suffix. I would also
give Prefix and Suffix an __add__ so you could do `Prefix(...)+Suffix(...)`. I
would strongly consider banning mixed use (i.e. an envvar is either
Prefix/Suffix, or it's a string, not both).

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:84: instead of directly manipulating this value.
I don't like the helper function idea; we should keep __call__ as the only way
to set stuff in the context module. I have a plan to clean up the api.step.nest
thing later too, in case that's what you're modeling this after.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:203: check_type(k, paths, collections.Iterable)
check_type("KEY %s[%r]" % (name, k), ...)

so then the error looks like:

  TypeError('KEY path_prefix["foobar"] is not ....')

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:206: k = k[1:]
let's remove the removal functionality for now

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:214: check_type('%s element #%d' % (k, i), path,
Path)
'%s[%r]' % (name, k)

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:272: return env
I'm not sure how I feel about this; I think that step.__call__ should be the
only thing assembling this, and that all other uses of env should just deal with
env. In fact, I would go so far as to make it an error to have the same envvar
key manipulated via env and also the prefix/suffix methods.

Then we should add a prefix_paths and suffix_paths @property to keep this
class's API uniform.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:343: def path_remove(self, key, *paths):
let's remove this functionality for now. I can't think of any use that wouldn't
be a horrible encapsulation violation. It would be easy to add later if
something needed it. The additive-only API is easier to reason about.

[iannucci, 2017-06-05 19:24:05]

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

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:266: cur = cur.split(os.pathsep) if cur else []
this will fail in production/simulation; use self.m.path.pathsep.

[dnj, 2017-06-07 02:59:29]

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

https://codereview.chromium.org/2925453002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:710: result[str(k)] = str(v) % subst
On 2017/06/05 19:23:04, iannucci wrote:
> On 2017/06/03 15:37:33, dnj wrote:
> > This fixes a bug where an environment variable that is referenced (e.g.,
> > "IDONTEXIST") will actually cause a KeyError instead of just being
substituted
> > as an empty string for real runs. I think we're just lucky so far that most
> > referenced environment variables are "PATH" / "PYTHONPATH", and that both
are
> > always defined.
> 
> Yeah, and with this CL I'd like to eventually eliminate the substitution
> entirely (so that it's a straight assign/delete, and the pre/suffix path
stuff).

ACK, but I think for now we still need it for backwards compatibility. Would be
nice to remove, make env either full append or full prescribe. The substitution
is def one of the most obscure and ugly part of this API.

OTOH referencing environment might be necessary, e.g.,

LOGDOG_STREAM_BASE=foo/bar

"Oh I want my step to log to a substream!"

env={'LOGDOG_STREAM_BASE': '%(LOGDOG_STREAM_BASE)s/baz'}

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

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:78: * env (dict) - Environmental variable
overrides. See below for more info.
On 2017/06/05 19:23:04, iannucci wrote:
> wdyt about something like:
> 
>   with api.context(env={"PATH": api.context.Prefix(a, b, c)}):
> 
> Then we don't need to additional kwargs, just Prefix and Suffix. I would also
> give Prefix and Suffix an __add__ so you could do `Prefix(...)+Suffix(...)`. I
> would strongly consider banning mixed use (i.e. an envvar is either
> Prefix/Suffix, or it's a string, not both).

That seems fine. For now I think we need mixed-use to transition to the new
notation, but I'll add a TODO to forbid mixed-use once that's done.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:84: instead of directly manipulating this value.
On 2017/06/05 19:23:04, iannucci wrote:
> I don't like the helper function idea; we should keep __call__ as the only way
> to set stuff in the context module. I have a plan to clean up the
api.step.nest
> thing later too, in case that's what you're modeling this after.

SGTM

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:203: check_type(k, paths, collections.Iterable)
On 2017/06/05 19:23:04, iannucci wrote:
> check_type("KEY %s[%r]" % (name, k), ...)
> 
> so then the error looks like:
> 
>   TypeError('KEY path_prefix["foobar"] is not ....')

No longer needed.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:206: k = k[1:]
On 2017/06/05 19:23:04, iannucci wrote:
> let's remove the removal functionality for now

Done.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:214: check_type('%s element #%d' % (k, i), path,
Path)
On 2017/06/05 19:23:04, iannucci wrote:
> '%s[%r]' % (name, k)

Also no longer needed.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:266: cur = cur.split(os.pathsep) if cur else []
On 2017/06/05 19:24:04, iannucci wrote:
> this will fail in production/simulation; use self.m.path.pathsep.

Done.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:272: return env
On 2017/06/05 19:23:04, iannucci wrote:
> I'm not sure how I feel about this; I think that step.__call__ should be the
> only thing assembling this, and that all other uses of env should just deal
with
> env. In fact, I would go so far as to make it an error to have the same envvar
> key manipulated via env and also the prefix/suffix methods.
> 
> Then we should add a prefix_paths and suffix_paths @property to keep this
> class's API uniform.

Done.

https://codereview.chromium.org/2925453002/diff/20001/recipe_modules/context/...
recipe_modules/context/api.py:343: def path_remove(self, key, *paths):
On 2017/06/05 19:23:04, iannucci wrote:
> let's remove this functionality for now. I can't think of any use that
wouldn't
> be a horrible encapsulation violation. It would be easy to add later if
> something needed it. The additive-only API is easier to reason about.

Done.

[iannucci, 2017-06-07 23:17:59]

lgtm w/ comments

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

https://codereview.chromium.org/2925453002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:702: else collections.defaultdict(lambda: '',
**original))
let's separate this into another cl, but also just tweak fakeEnviron to
implement like a .fake = True flag or something.

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

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:49: 'str', 'prefixes', 'suffixes'))
prefixes, str, suffixes?

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:101: environment variable to have. The value is
either:
s/either/one of

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:119: # suppose the OS's envar $OTHER is set to
"yes"
can we update this Example?

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:182: ev =
ev._replace(prefixes=tuple(_uniquify(v.paths, ev.prefixes)))
let's just make it append

[dnj, 2017-06-07 23:48:47]

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

https://codereview.chromium.org/2925453002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:702: else collections.defaultdict(lambda: '',
**original))
On 2017/06/07 23:17:59, iannucci wrote:
> let's separate this into another cl, but also just tweak fakeEnviron to
> implement like a .fake = True flag or something.

Tried it, was ugly.

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

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:49: 'str', 'prefixes', 'suffixes'))
On 2017/06/07 23:17:59, iannucci wrote:
> prefixes, str, suffixes?

Done.

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:101: environment variable to have. The value is
either:
On 2017/06/07 23:17:59, iannucci wrote:
> s/either/one of

Done.

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:119: # suppose the OS's envar $OTHER is set to
"yes"
On 2017/06/07 23:17:59, iannucci wrote:
> can we update this Example?

Done.

https://codereview.chromium.org/2925453002/diff/40001/recipe_modules/context/...
recipe_modules/context/api.py:182: ev =
ev._replace(prefixes=tuple(_uniquify(v.paths, ev.prefixes)))
On 2017/06/07 23:17:59, iannucci wrote:
> let's just make it append

Done.

[dnj, 2017-06-07 23:50:02]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-06-07 23:50:02]

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/2925453002/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2017-06-07 23:50:12]

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-07 23:53:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496879402127740,
"parent_rev": "db920908bf51fcdb0e30f686a528b25cd4876ca7", "commit_rev":
"c40412cbc4e1b8ee1e6eba2e88328ca71c6ec1e4"}

[commit-bot: I haz the power, 2017-06-07 23:53:22]

Description was changed from

==========
[context] Add path prefix/suffix manipulation.

Add the ability to manipulate pathsep-delimited environment variables
more easily with specific helper functions targeting the prefixing and
suffixing of values to those variables.

Previously, the supported way to augment path environment variables was
to manually construct them, referencing the current value:

new_path = "%s%%(PATH)s%s" % (api.path.pathsep, api.path.pathsep)

Not only is this very verbose, but it is also error-prone, as the user
may not use OS-specific path separators, can add the same entries
multiple times, and can forget the double-percent to escape the
inner substitition.

This patch adds the concept of path prefixes and suffixes to the
"context" recipe module. Path elements can be added to either, and will
be compiled into properly-delimited prefix and suffix values on the
context's environment.

BUG=None
TEST=expectations,run
==========

to

==========
[context] Add path prefix/suffix manipulation.

Add the ability to manipulate pathsep-delimited environment variables
more easily with specific helper functions targeting the prefixing and
suffixing of values to those variables.

Previously, the supported way to augment path environment variables was
to manually construct them, referencing the current value:

new_path = "%s%%(PATH)s%s" % (api.path.pathsep, api.path.pathsep)

Not only is this very verbose, but it is also error-prone, as the user
may not use OS-specific path separators, can add the same entries
multiple times, and can forget the double-percent to escape the
inner substitition.

This patch adds the concept of path prefixes and suffixes to the
"context" recipe module. Path elements can be added to either, and will
be compiled into properly-delimited prefix and suffix values on the
context's environment.

BUG=None
TEST=expectations,run

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

[commit-bot: I haz the power, 2017-06-07 23:53:22]

Committed patchset #4 (id:60001) as
https://github.com/luci/recipes-py/commit/c40412cbc4e1b8ee1e6eba2e88328ca71c6...

[iannucci, 2017-06-08 00:46:59]

fix https://codereview.chromium.org/2920223011

https://codereview.chromium.org/2925453002/diff/60001/recipe_modules/python/e...
File recipe_modules/python/examples/full.expected/basic.json (right):

https://codereview.chromium.org/2925453002/diff/60001/recipe_modules/python/e...
recipe_modules/python/examples/full.expected/basic.json:35: "PYTHONUNBUFFERED":
""
damn I missed this >_<