New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 727493 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Recipe engine bug: unexpected failure - Percentage in environment fails in prod but not in testing

Project Member Reported by tansell@chromium.org, May 30 2017

Issue description

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/467070
---------------------------------------------
Traceback (most recent call last):
  File "/b/rr/tmpp0nlQx/rw/checkout/scripts/slave/.recipe_deps/recipe_engine/recipe_engine/step_runner.py", line 181, in open_step
    step_env = _merge_envs(os.environ, (rendered_step.config.env or {}))
  File "/b/rr/tmpp0nlQx/rw/checkout/scripts/slave/.recipe_deps/recipe_engine/recipe_engine/step_runner.py", line 670, in _merge_envs
    result[str(k)] = str(v) % original
TypeError: float argument required, not instance
---------------------------------------------

This was caused by https://chromium-review.googlesource.com/c/517847 which did a;

    ninja_env['NINJA_STATUS'] = (
        '%e [%s/%t %p - %r running, current %c p/s, overall %o p/s] ')

This passed the training / testing - but failed on production.

This is caused by _merge_env in step_runner.py;
---------------------------------------------
def _merge_envs(original, override):
  """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).
  """
  result = original.copy()
  if not override:
    return result
  for k, v in override.items():
    if v is None:
      if k in result:
        del result[k]
    else:
      result[str(k)] = str(v) % original
  return result
---------------------------------------------

 
Any idea why this didn't cause failure in the training stage?
Labels: -Infra-Troopers
Labels: Type-Bug
Status: Available (was: Untriaged)
As mentioned in chat, but for posterity:

_merge_env is use to merge the recipe's concept of the environment with the real OS's environment. It %-formats the env value with the current environ to allow things like `%(PATH)s` to expand inside the environment values, as documented here: https://github.com/luci/recipes-py/blob/master/recipe_modules/context/api.py#L79

We could add some checks to the context module to ensure that the value is %-formattable in this way (maybe by passing in some fake dictionary-like object that is able to supply empty values for any conceivable key).

That said, I'd like to get rid of this %-format hack. The only thing it's really used for is manipulating $PATH and $PYTHONPATH; they would be better served by having explicit support for path-like environment variables.
Labels: -Infra
I added some error checks and tests and such: https://codereview.chromium.org/2913203002
Status: Fixed (was: Available)

Sign in to add a comment