Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Have bootstrap_pex_env pull in contents of pexrc files #747

Closed
wants to merge 8 commits into from
Closed

Have bootstrap_pex_env pull in contents of pexrc files #747

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7451a62
Added optional arg to bootstrap_pex_env which will cause pexrc config…
rtkjliviero Jul 25, 2019
00d34af
Adding list to list, instead of string to list
rtkjliviero Jul 25, 2019
812c822
Style fix (line length)
rtkjliviero Jul 26, 2019
e03d719
Added option to pass in dict or json, for overriding specific vars in…
rtkjliviero Jul 26, 2019
7d3691c
PR feedback; mostly Style cleanup
rtkjliviero Aug 21, 2019
a6ccc62
Reverted unnecessary style change
rtkjliviero Aug 21, 2019
ee4e27c
Added some unit tests for pex_bootstrapper and an empty .pex file as …
rtkjliviero Aug 21, 2019
64bd609
Style fixes
rtkjliviero Aug 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions pex/pex_bootstrapper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from __future__ import absolute_import, print_function

import json
import os
import sys

Expand Down Expand Up @@ -173,9 +174,22 @@ def maybe_reexec_pex(compatibility_constraints=None):
os.execv(target_binary, cmdline)


def _bootstrap(entry_point):
def _bootstrap(entry_point, include_pexrc=False, extra_vars=None):
from .pex_info import PexInfo
pex_info = PexInfo.from_pex(entry_point)

# Pull in the default ENV, which is an instance of pex.variables.Variables, which reads
# from the default pexrc locations.
if include_pexrc:
pex_info.update(PexInfo.from_env())

# Override env settings using extra passed-in vars
if extra_vars:
if isinstance(extra_vars, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just accepting **extra_vars and pushing json back to the user that needs it. It's terrifying that we have a consumer of this API at all, and as a relunctant / conservative library maintainer I'd much rather keep API surface area minimal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure what you mean here, would you mind clarifying?

Are you saying this should only allow properly formatted json here, and require the user to convert from a dict if necessary?

Copy link
Author

@rtkjliviero rtkjliviero Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsirois, if you could clarify I'm more than happy to update this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean support the minimal api, which is allowing the user to pass environment variables via **extra_vars. If code that call this wants to support a json encoding of the env vars that code can decode the json to a dict to pass in as **extra_vars.

Concreteley, just:

def _bootstrap(entry_point, include_pexrc=False, **extra_vars):
  from .pex_info import PexInfo
  pex_info = PexInfo.from_pex(entry_point)
  env = dict(PEX_IGNORE_RCFILES=str(not include_pexrc), **extra_vars)
  pex_info.update(PexInfo.from_env(env))

Calling code in an application can do this if it want to unburdening Pex from supporting json in this API in perpetuity:

_bootstrap(entry_point, **json.loads(extra_vars_json_string))

And, of course, where this is really important is in the public API function, so the **extra_vars style should propagate up to bootstrap_pex_env.

pex_info.update(PexInfo.from_json(json.dumps(extra_vars)))
elif isinstance(extra_vars, str):
pex_info.update(PexInfo.from_json(extra_vars))

pex_warnings.configure_warnings(pex_info)

from .finders import register_finders
Expand All @@ -199,9 +213,13 @@ def is_compressed(entry_point):
return os.path.exists(entry_point) and not os.path.exists(os.path.join(entry_point, PexInfo.PATH))


def bootstrap_pex_env(entry_point):
"""Bootstrap the current runtime environment using a given pex."""
pex_info = _bootstrap(entry_point)
def bootstrap_pex_env(entry_point, include_pexrc=False, extra_vars=None):
"""
rtkjliviero marked this conversation as resolved.
Show resolved Hide resolved
Bootstrap the current runtime environment using a given pex. Optionally, pexrc files from
'standard' locations can be included - see pex.variables.DEFAULT_PEXRC_LOCATIONS - or
runtime-specific overrides (dict or json)
"""
pex_info = _bootstrap(entry_point, include_pexrc=include_pexrc, extra_vars=extra_vars)

from .environment import PEXEnvironment
from pex.environment import PEXEnvironment
rtkjliviero marked this conversation as resolved.
Show resolved Hide resolved
PEXEnvironment(entry_point, pex_info).activate()
10 changes: 7 additions & 3 deletions pex/variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@

__all__ = ('ENV', 'Variables')

DEFAULT_PEXRC_LOCATIONS = [
rtkjliviero marked this conversation as resolved.
Show resolved Hide resolved
'/etc/pexrc',
'~/.pexrc'
]


class Variables(object):
"""Environment variables supported by the PEX runtime."""
Expand Down Expand Up @@ -44,9 +49,8 @@ def from_rc(cls, rc=None):
:rtype: dict
"""
ret_vars = {}
rc_locations = ['/etc/pexrc',
'~/.pexrc',
os.path.join(os.path.dirname(sys.argv[0]), '.pexrc')]
rc_locations = DEFAULT_PEXRC_LOCATIONS + [os.path.join(os.path.dirname(sys.argv[0]), '.pexrc')]

if rc:
rc_locations.append(rc)
for filename in rc_locations:
Expand Down