[context] Split "env" values and prefixes.

recipe_engine/recipe_api.py

 # Copyright 2016 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 from __future__ import absolute_import
 import bisect
 import collections
 import contextlib
 import copy
 import json
 import keyword
+import os
 import re
 import types
 
 from functools import wraps
 
 from .recipe_test_api import DisabledTestData, ModuleTestData
 from .config import Single
 
 from .util import ModuleInjectionSite
 
@@ skipping 228 matching lines @@
     )
 
   def _render_to_dict(self):
     d = dict((k, v) for k, v in self._asdict().iteritems() if v)
     if d['critical']:
       d.pop('critical')
     return d
 
 
 _StepConfig = collections.namedtuple('_StepConfig',
-    ('name', 'base_name', 'cmd', 'cwd', 'env', 'allow_subannotations',
-     'trigger_specs', 'timeout', 'infra_step', 'stdout', 'stderr', 'stdin',
-     'ok_ret', 'step_test_data', 'nest_level'))
+    ('name', 'base_name', 'cmd', 'cwd', 'env_prefixes', 'env', 'pathsep',

[iannucci, 2017/06/12 19:58:39]

pathsep is necessary for simulations I guess?

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

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

+     'allow_subannotations', 'trigger_specs', 'timeout', 'infra_step', 'stdout',
+     'stderr', 'stdin', 'ok_ret', 'step_test_data', 'nest_level'))
 
 class StepConfig(_StepConfig):
   """
   StepConfig is the representation of a raw step as the recipe_engine sees it.
   You should use the standard 'step' recipe module, which will construct and
   pass this data to the engine for you, instead. The only reason why you would
   need to worry about this object is if you're modifying the step module itself.
 
   The optional "env" parameter provides optional overrides for environment
   variables. Each value is % formatted with the entire existing os.environ. A
   value of `None` will remove that envvar from the environ. e.g.
 
     {
         "envvar": "%(envvar)s;%(envvar2)s;extra",
         "delete_this": None,
         "static_value": "something",
     }
+
+  The optional "env_prefix" parameter contains values that, if specified, will
+  transform an environment variable into a "pathsep"-delimited sequence of
+  items:
+    - If an environment variable is also specified for this key, it will be
+      appended as the last element: <prefix0>:...:<prefixN>:ENV
+    - If no enviornment variable is specified, the current environment's value
+      will be appended, unless it's empty: <prefix0>:...:<prefixN>[:ENV]?
+    - If an enviornment variable with a value of None (delete) is specified,
+      nothing will be appeneded: <prefix0>:...:<prefixN>

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

"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."

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

Done.

   """
 
   _RENDER_WHITELIST=frozenset((
     'cmd',
   ))
 
   _RENDER_BLACKLIST=frozenset((
     'base_name',
     'nest_level',
     'ok_ret',
+    'pathsep',
     'step_test_data',
   ))
 
   @classmethod
-  def create(cls, name, base_name=None, cmd=None, cwd=None, env=None,
-             allow_subannotations=None, trigger_specs=None, timeout=None,
-             infra_step=None, stdout=None, stderr=None, stdin=None,
-             ok_ret=None, step_test_data=None, step_nest_level=None):
+  def create(cls, name, base_name=None, cmd=None, cwd=None, env_prefixes=None,
+             pathsep=None, env=None, allow_subannotations=None,
+             trigger_specs=None, timeout=None, infra_step=None, stdout=None,
+             stderr=None, stdin=None, ok_ret=None, step_test_data=None,
+             step_nest_level=None):
     """
     Initializes a new StepConfig step API dictionary.
 
     Args:
       name (str): name of the step, will appear in buildbots waterfall
       base_name (str): the base name of the step. If the step has a derived
           name (e.g., nested may be concatenated with its parent), this is the
           name component of just this step. If None, this will be set to "name".
       cmd: command to run. Acceptable types: str, Path, Placeholder, or None.
       cwd (str or None): absolute path to working directory for the command
+      env_prefixes (dict): environment prefix variables, described above.
       env (dict): overrides for environment variables, described above.
+      pathsep (str): the path separator string.
       allow_subannotations (bool): if True, lets the step emit its own
           annotations. NOTE: Enabling this can cause some buggy behavior. Please
           strongly consider using step_result.presentation instead. If you have
           questions, please contact infra-dev@chromium.org.
       trigger_specs: a list of trigger specifications, see also _trigger_builds.
       timeout: if not None, a datetime.timedelta for the step timeout.
       infra_step: if True, this is an infrastructure step. Failures will raise
           InfraFailure instead of StepFailure.
       stdout: Placeholder to put step stdout into. If used, stdout won't appear
           in annotator's stdout (and |allow_subannotations| is ignored).
       stderr: Placeholder to put step stderr into. If used, stderr won't appear
           in annotator's stderr.
       stdin: Placeholder to read step stdin from.
       ok_ret (iter): set of return codes allowed. If the step process returns
           something not on this list, it will raise a StepFailure (or
           InfraFailure if infra_step is True). If omitted, {0} will be used.
       step_test_data (func -> recipe_test_api.StepTestData): A factory which
           returns a StepTestData object that will be used as the default test
           data for this step. The recipe author can override/augment this object
           in the GenTests function.
       step_nest_level (int): the step's nesting level.
     """
     return cls(
         name=name,
         base_name=(base_name or name),
         cmd=cmd,
         cwd=cwd,
-        env=env,
+        env_prefixes={k: tuple(v) for k, v in (env_prefixes or {}).iteritems()},
+        env=env or {},
+        pathsep=(pathsep or os.pathsep),

[iannucci, 2017/06/12 19:58:39]

this should just be required... os.pathsep might accidentally cause a sadness I
think?

[dnj, 2017/06/13 19:31:20]

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.

         allow_subannotations=bool(allow_subannotations),
         trigger_specs=[TriggerSpec._create(**trig)
                        for trig in (trigger_specs or ())],
         timeout=timeout,
         infra_step=bool(infra_step),
         stdout=stdout,
         stderr=stderr,
         stdin=stdin,
         ok_ret=frozenset(ok_ret or (0,)),
         step_test_data=step_test_data,
         nest_level=int(step_nest_level or 0),
     )
 
   def render_to_dict(self):
     self = self._replace(
+        env_prefixes={k: list(v) for k, v in self.env_prefixes.iteritems()},

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

map(str, v)?

[dnj, 2017/06/13 19:31:20]

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.

         trigger_specs=[trig._render_to_dict()
                        for trig in (self.trigger_specs or ())],
     )
     return dict((k, v) for k, v in self._asdict().iteritems()
                 if (v or k in self._RENDER_WHITELIST)
                 and k not in self._RENDER_BLACKLIST)
 
 
 class StepFailure(Exception):
   """
@@ skipping 713 matching lines @@
   def bind(self, name, property_type, full_decl_name):
     """
     Gets the BoundProperty version of this Property. Requires a name.
     """
     return BoundProperty(
       self._default, self.help, self.kind, name, property_type, full_decl_name,
       self.param_name)
 
 class UndefinedPropertyException(TypeError):
   pass