[context] Split "env" values and prefixes.

recipe_engine/step_runner.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.
 
 import calendar
 import collections
 import contextlib
 import datetime
 import itertools
 import json
@@ skipping 160 matching lines @@
           return step_stream
 
       return EmptyOpenStep()
 
     try:
       rendered_step = render_step(
           step_config, recipe_test_api.DisabledTestData()
       )
       step_config = None  # Make sure we use rendered step config.
 
-      step_env = _merge_envs(os.environ, (rendered_step.config.env or {}))
+      step_env = _merge_envs(os.environ,
+          rendered_step.config.env, rendered_step.config.env_prefixes,
+          rendered_step.config.pathsep)
       # Now that the step's environment is all sorted, evaluate PATH on windows
       # to find the actual intended executable.
       rendered_step = _hunt_path(rendered_step, step_env)
       self._print_step(step_stream, rendered_step, step_env)
     except:
       with self.stream_engine.make_step_stream('Step Preparation Exception') as s:
         s.set_step_status('EXCEPTION')
         with s.new_log_stream('exception') as l:
           l.write_split(traceback.format_exc())
       raise
@@ skipping 226 matching lines @@
     * copy() returns self
 
   The 'formatted' result can be obtained by looking at .data.
   """
   def __init__(self):
     self.data = {}
 
   def __getitem__(self, key):
     return '<%s>' % key
 
+  def get(self, key, default=None):
+    return self[key]

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

oof, this is starting to get weird; this and __getitem__ don't agree

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

How don't they? This returns __getitem__.

+
   def keys(self):
     return self.data.keys()
 
+  def pop(self, key, default=None):
+    result = self.data.get(key, default)
+    self.data[key] = None
+    return result
+
   def __delitem__(self, key):
     self.data[key] = None
 
   def __contains__(self, key):
     return True
 
   def __setitem__(self, key, value):
     self.data[key] = value
 
   def copy(self):
@@ skipping 17 matching lines @@
   @property
   def stream_engine(self):
     return self._stream_engine
 
   def open_step(self, step_config):
     try:
       test_data_fn = step_config.step_test_data or recipe_test_api.StepTestData
       step_test = self._test_data.pop_step_test_data(step_config.name,
                                                      test_data_fn)
       rendered_step = render_step(step_config, step_test)
-      step_env = _merge_envs(fakeEnviron(), (rendered_step.config.env or {}))
+
+      # Merge our environment. Note that do NOT apply prefixes when rendering
+      # expectations, as they are rendered independently.
+      step_env = _merge_envs(fakeEnviron(), rendered_step.config.env, {}, None)
       rendered_step = rendered_step._replace(
         config=rendered_step.config._replace(env=step_env.data))
       step_config = None  # Make sure we use rendered step config.
 
       # Layer the simulation step on top of the given stream engine.
       step_stream = self._stream_engine.new_step_stream(rendered_step.config)
     except:
       with self.stream_engine.make_step_stream('Step Preparation Exception') as s:
         s.set_step_status('EXCEPTION')
         with s.new_log_stream('exception') as l:
@@ skipping 202 matching lines @@
       if isinstance(ph, util.OutputPlaceholder):
         setattr(step_result, key, ph.result(step_result.presentation, td))
       else:
         assert isinstance(ph, util.InputPlaceholder), (
             '%s(%r) should be an InputPlaceholder.' % (key, ph))
         ph.cleanup(td.enabled)
 
   return step_result
 
 
-def _merge_envs(original, override):
+# Sentinel used by "_merge_envs" to indicate a missing value.
+_MISSING = object()
+
+
+def _merge_envs(original, overrides, prefixes, pathsep):
   """Merges two environments.
 
   Returns a new environment dict with entries from |override| overwriting
   corresponding entries in |original|. Keys whose value is None will completely
   remove the environment variable. Values can contain %(KEY)s strings, which
   will be substituted with the values from the original (useful for amending, as
   opposed to overwriting, variables like PATH).
+
+  See recipe_api.StepConfig for environment construction rules.
   """
   result = original.copy()
   subst = (original if isinstance(original, fakeEnviron)
            else collections.defaultdict(lambda: '', **original))
-  if not override:
+
+  if not any((prefixes, overrides)):
     return result
-  for k, v in override.items():
+
+  merged = set()
+  for k, path_tuple in prefixes.iteritems():
+    if not path_tuple:
+      continue
+    merged.add(k)
+
+    # If the same key is defined in "overrides", we need to interact with it.
+    # We'll do so here, and skip it in the "overrides" construction.
+    val = overrides.get(k, _MISSING)
+    if val is _MISSING:

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

this might be clearer

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

?

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

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.

+      # Not defined. Append "val" iff it is defined in "original" and not empty.
+      val = original.get(k, '')
+    elif val is not None:
+      val = str(val) % subst
+    if val:
+      path_tuple += (val,)
+    result[k] = pathsep.join(str(v) for v in path_tuple)
+
+  for k, v in overrides.iteritems():
+    if k in merged:
+      continue
     if v is None:
-      if k in result:
-        del result[k]
+      result.pop(k, None)
     else:
-      result[str(k)] = str(v) % subst
+      result[k] = str(v) % subst
   return result
 
 
 if sys.platform == "win32":
   _hunt_path_exts = ('.exe', '.bat')
   def _hunt_path(rendered_step, step_env):
     """This takes the lazy cross-product of PATH and ('.exe', '.bat') to find
     what cmd.exe would have found for the command if we used shell=True.
 
     This must be called on the render_step AFTER _merge_envs has produced
@@ skipping 67 matching lines @@
   supplied command, and only uses the |env| kwarg for modifying the environment
   of the child process.
   """
   saved_path = os.environ['PATH']
   try:
     if path is not None:
       os.environ['PATH'] = path
     yield
   finally:
     os.environ['PATH'] = saved_path