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.

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

Make more option values environment-sensitive #16984

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions src/python/pants/backend/codegen/thrift/apache/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,12 @@ async def generate_apache_thrift_sources(


@rule
async def setup_thrift_tool(apache_thrift: ApacheThriftSubsystem) -> ApacheThriftSetup:
async def setup_thrift_tool(
apache_thrift: ApacheThriftSubsystem,
apache_thrift_env_aware: ApacheThriftSubsystem.EnvironmentAware,
) -> ApacheThriftSetup:
env = await Get(EnvironmentVars, EnvironmentVarsRequest(["PATH"]))
search_paths = apache_thrift.thrift_search_paths(env)
search_paths = apache_thrift_env_aware.thrift_search_paths(env)
all_thrift_binary_paths = await Get(
BinaryPaths,
BinaryPathRequest(
Expand Down
50 changes: 26 additions & 24 deletions src/python/pants/backend/codegen/thrift/apache/subsystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,6 @@ class ApacheThriftSubsystem(Subsystem):
options_scope = "apache-thrift"
help = "Apache Thrift IDL compiler (https://thrift.apache.org/)."

_thrift_search_paths = StrListOption(
default=["<PATH>"],
help=softwrap(
"""
A list of paths to search for Thrift.

Specify absolute paths to directories with the `thrift` binary, e.g. `/usr/bin`.
Earlier entries will be searched first.

The special string `"<PATH>"` will expand to the contents of the PATH env var.
"""
),
)
expected_version = StrOption(
default="0.15",
help=softwrap(
Expand All @@ -42,14 +29,29 @@ class ApacheThriftSubsystem(Subsystem):
),
)

def thrift_search_paths(self, env: EnvironmentVars) -> tuple[str, ...]:
def iter_path_entries():
for entry in self._thrift_search_paths:
if entry == "<PATH>":
path = env.get("PATH")
if path:
yield from path.split(os.pathsep)
else:
yield entry

return tuple(OrderedSet(iter_path_entries()))
class EnvironmentAware:
_thrift_search_paths = StrListOption(
default=["<PATH>"],
help=softwrap(
"""
A list of paths to search for Thrift.

Specify absolute paths to directories with the `thrift` binary, e.g. `/usr/bin`.
Earlier entries will be searched first.

The special string `"<PATH>"` will expand to the contents of the PATH env var.
"""
),
)

def thrift_search_paths(self, env: EnvironmentVars) -> tuple[str, ...]:
def iter_path_entries():
for entry in self._thrift_search_paths:
if entry == "<PATH>":
path = env.get("PATH")
if path:
yield from path.split(os.pathsep)
else:
yield entry

return tuple(OrderedSet(iter_path_entries()))
7 changes: 5 additions & 2 deletions src/python/pants/backend/docker/goals/publish.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ def get_output_data(self) -> PublishOutputData:

@rule
async def push_docker_images(
request: PublishDockerImageRequest, docker: DockerBinary, options: DockerOptions
request: PublishDockerImageRequest,
docker: DockerBinary,
options: DockerOptions,
options_env_aware: DockerOptions.EnvironmentAware,
) -> PublishProcesses:
tags = tuple(
chain.from_iterable(
Expand All @@ -71,7 +74,7 @@ async def push_docker_images(
]
)

env = await Get(EnvironmentVars, EnvironmentVarsRequest(options.env_vars))
env = await Get(EnvironmentVars, EnvironmentVarsRequest(options_env_aware.env_vars))
skip_push = defaultdict(set)
jobs: list[PublishPackages] = []
refs: list[str] = []
Expand Down
7 changes: 5 additions & 2 deletions src/python/pants/backend/docker/goals/run_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@

@rule
async def docker_image_run_request(
field_set: DockerFieldSet, docker: DockerBinary, options: DockerOptions
field_set: DockerFieldSet,
docker: DockerBinary,
options: DockerOptions,
options_env_aware: DockerOptions.EnvironmentAware,
) -> RunRequest:
env, image = await MultiGet(
Get(EnvironmentVars, EnvironmentVarsRequest(options.env_vars)),
Get(EnvironmentVars, EnvironmentVarsRequest(options_env_aware.env_vars)),
Get(BuiltPackage, PackageFieldSet, field_set),
)
tag = cast(BuiltDockerImage, image.artifacts[0]).tags[0]
Expand Down
82 changes: 42 additions & 40 deletions src/python/pants/backend/docker/subsystems/docker_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,48 @@ class DockerOptions(Subsystem):
options_scope = "docker"
help = "Options for interacting with Docker."

class EnvironmentAware:
_env_vars = ShellStrListOption(
help=softwrap(
"""
Environment variables to set for `docker` invocations.

Entries are either strings in the form `ENV_VAR=value` to set an explicit value;
or just `ENV_VAR` to copy the value from Pants's own environment.
"""
),
advanced=True,
)
_executable_search_paths = StrListOption(
default=["<PATH>"],
help=softwrap(
"""
The PATH value that will be used to find the Docker client and any tools required.

The special string `"<PATH>"` will expand to the contents of the PATH env var.
"""
),
advanced=True,
metavar="<binary-paths>",
)

@property
def env_vars(self) -> tuple[str, ...]:
return tuple(sorted(set(self._env_vars)))

@memoized_method
def executable_search_path(self, env: EnvironmentVars) -> tuple[str, ...]:
def iter_path_entries():
for entry in self._executable_search_paths:
if entry == "<PATH>":
path = env.get("PATH")
if path:
yield from path.split(os.pathsep)
else:
yield entry

return tuple(OrderedSet(iter_path_entries()))

_registries = DictOption[Any](
help=softwrap(
"""
Expand Down Expand Up @@ -154,17 +196,6 @@ class DockerOptions(Subsystem):
default=False,
help="Whether to log the Docker output to the console. If false, only the image ID is logged.",
)
_env_vars = ShellStrListOption(
help=softwrap(
"""
Environment variables to set for `docker` invocations.

Entries are either strings in the form `ENV_VAR=value` to set an explicit value;
or just `ENV_VAR` to copy the value from Pants's own environment.
"""
),
advanced=True,
)
run_args = ShellStrListOption(
default=["--interactive", "--tty"] if sys.stdout.isatty() else [],
help=softwrap(
Expand All @@ -187,18 +218,6 @@ class DockerOptions(Subsystem):
"""
),
)
_executable_search_paths = StrListOption(
default=["<PATH>"],
help=softwrap(
"""
The PATH value that will be used to find the Docker client and any tools required.

The special string `"<PATH>"` will expand to the contents of the PATH env var.
"""
),
advanced=True,
metavar="<binary-paths>",
)
_tools = StrListOption(
default=[],
help=softwrap(
Expand All @@ -221,27 +240,10 @@ class DockerOptions(Subsystem):
def build_args(self) -> tuple[str, ...]:
return tuple(sorted(set(self._build_args)))

@property
def env_vars(self) -> tuple[str, ...]:
return tuple(sorted(set(self._env_vars)))

@property
def tools(self) -> tuple[str, ...]:
return tuple(sorted(set(self._tools)))

@memoized_method
def registries(self) -> DockerRegistries:
return DockerRegistries.from_dict(self._registries)

@memoized_method
def executable_search_path(self, env: EnvironmentVars) -> tuple[str, ...]:
def iter_path_entries():
for entry in self._executable_search_paths:
if entry == "<PATH>":
path = env.get("PATH")
if path:
yield from path.split(os.pathsep)
else:
yield entry

return tuple(OrderedSet(iter_path_entries()))
6 changes: 4 additions & 2 deletions src/python/pants/backend/docker/util_rules/docker_binary.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,12 @@ class DockerBinaryRequest:

@rule(desc="Finding the `docker` binary and related tooling", level=LogLevel.DEBUG)
async def find_docker(
docker_request: DockerBinaryRequest, docker_options: DockerOptions
docker_request: DockerBinaryRequest,
docker_options: DockerOptions,
docker_options_env_aware: DockerOptions.EnvironmentAware,
) -> DockerBinary:
env = await Get(EnvironmentVars, EnvironmentVarsRequest(["PATH"]))
search_path = docker_options.executable_search_path(env)
search_path = docker_options_env_aware.executable_search_path(env)
request = BinaryPathRequest(
binary_name="docker",
search_path=search_path,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,14 @@ class DockerBuildEnvironmentRequest:

@rule
async def docker_build_environment_vars(
request: DockerBuildEnvironmentRequest, docker_options: DockerOptions
request: DockerBuildEnvironmentRequest,
docker_options: DockerOptions,
docker_env_aware: DockerOptions.EnvironmentAware,
) -> DockerBuildEnvironment:
build_args = await Get(DockerBuildArgs, DockerBuildArgsRequest(request.target))
env_vars = KeyValueSequenceUtil.from_strings(
*{build_arg for build_arg in build_args if "=" not in build_arg},
*docker_options.env_vars,
*docker_env_aware.env_vars,
)
env = await Get(EnvironmentVars, EnvironmentVarsRequest(tuple(env_vars)))
return DockerBuildEnvironment.create(env)
Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/backend/python/goals/publish.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ def twine_env(env: EnvironmentVars, repo: str) -> EnvironmentVars:
async def twine_upload(
request: PublishPythonPackageRequest,
twine_subsystem: TwineSubsystem,
twine_environment_aware: TwineSubsystem.EnvironmentAware,
global_options: GlobalOptions,
) -> PublishProcesses:
dists = tuple(
Expand Down Expand Up @@ -176,7 +177,7 @@ async def twine_upload(
Get(ConfigFiles, ConfigFilesRequest, twine_subsystem.config_request()),
)

ca_cert_request = twine_subsystem.ca_certs_digest_request(global_options.ca_certs_path)
ca_cert_request = twine_environment_aware.ca_certs_digest_request(global_options.ca_certs_path)
ca_cert = await Get(Snapshot, CreateDigest, ca_cert_request) if ca_cert_request else None
ca_cert_digest = (ca_cert.digest,) if ca_cert else ()

Expand Down
35 changes: 18 additions & 17 deletions src/python/pants/backend/python/subsystems/python_native_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,22 @@ class PythonNativeCodeSubsystem(Subsystem):
options_scope = "python-native-code"
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity: this subsystem has several problems, including #14612

I don't think it blocks this PR, given that environment targets are still experimental. But we should probably fix that ticket before 2.15

help = "Options for building native code using Python, e.g. when resolving distributions."

# TODO(#7735): move the --cpp-flags and --ld-flags to a general subprocess support subsystem.
cpp_flags = StrListOption(
default=safe_shlex_split(os.environ.get("CPPFLAGS", "")),
help="Override the `CPPFLAGS` environment variable for any forked subprocesses.",
advanced=True,
)
ld_flags = StrListOption(
default=safe_shlex_split(os.environ.get("LDFLAGS", "")),
help="Override the `LDFLAGS` environment variable for any forked subprocesses.",
advanced=True,
)
class EnvironmentAware:
# TODO(#7735): move the --cpp-flags and --ld-flags to a general subprocess support subsystem.
cpp_flags = StrListOption(
default=safe_shlex_split(os.environ.get("CPPFLAGS", "")),
help="Override the `CPPFLAGS` environment variable for any forked subprocesses.",
advanced=True,
)
ld_flags = StrListOption(
default=safe_shlex_split(os.environ.get("LDFLAGS", "")),
help="Override the `LDFLAGS` environment variable for any forked subprocesses.",
advanced=True,
)

@property
def environment_dict(self) -> Dict[str, str]:
return {
"CPPFLAGS": safe_shlex_join(self.cpp_flags),
"LDFLAGS": safe_shlex_join(self.ld_flags),
}
@property
def environment_dict(self) -> Dict[str, str]:
return {
"CPPFLAGS": safe_shlex_join(self.cpp_flags),
"LDFLAGS": safe_shlex_join(self.ld_flags),
}
52 changes: 27 additions & 25 deletions src/python/pants/backend/python/subsystems/twine.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,6 @@ class TwineSubsystem(PythonToolBase):
"""
),
)
ca_certs_path = StrOption(
advanced=True,
default="<inherit>",
help=softwrap(
"""
Path to a file containing PEM-format CA certificates used for verifying secure
connections when publishing python distributions.

Uses the value from `[GLOBAL].ca_certs_path` by default. Set to `"<none>"` to
not use the default CA certificate.
"""
),
)

def config_request(self) -> ConfigFilesRequest:
# Refer to https://twine.readthedocs.io/en/latest/#configuration for how config files are
Expand All @@ -95,18 +82,33 @@ def config_request(self) -> ConfigFilesRequest:
check_existence=[".pypirc"],
)

def ca_certs_digest_request(self, default_ca_certs_path: str | None) -> CreateDigest | None:
ca_certs_path: str | None = self.ca_certs_path
if ca_certs_path == "<inherit>":
ca_certs_path = default_ca_certs_path
if not ca_certs_path or ca_certs_path == "<none>":
return None

# The certs file will typically not be in the repo, so we can't digest it via a PathGlobs.
# Instead we manually create a FileContent for it.
ca_certs_content = Path(ca_certs_path).read_bytes()
chrooted_ca_certs_path = os.path.basename(ca_certs_path)
return CreateDigest((FileContent(chrooted_ca_certs_path, ca_certs_content),))
class EnvironmentAware:
ca_certs_path = StrOption(
advanced=True,
default="<inherit>",
help=softwrap(
"""
Path to a file containing PEM-format CA certificates used for verifying secure
connections when publishing python distributions.

Uses the value from `[GLOBAL].ca_certs_path` by default. Set to `"<none>"` to
not use the default CA certificate.
"""
),
)

def ca_certs_digest_request(self, default_ca_certs_path: str | None) -> CreateDigest | None:
ca_certs_path: str | None = self.ca_certs_path
if ca_certs_path == "<inherit>":
ca_certs_path = default_ca_certs_path
if not ca_certs_path or ca_certs_path == "<none>":
return None

# The certs file will typically not be in the repo, so we can't digest it via a PathGlobs.
# Instead we manually create a FileContent for it.
ca_certs_content = Path(ca_certs_path).read_bytes()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not safe, but can be fixed in a follow up #16987

Copy link
Contributor Author

@chrisjrn chrisjrn Sep 26, 2022

Choose a reason for hiding this comment

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

It's also fundamentally not correct :)

Happy to leave it as and fix in follow-up

chrooted_ca_certs_path = os.path.basename(ca_certs_path)
return CreateDigest((FileContent(chrooted_ca_certs_path, ca_certs_content),))


class TwineLockfileSentinel(GeneratePythonToolLockfileSentinel):
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/util_rules/pex_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ async def setup_pex_cli_process(
request: PexCliProcess,
pex_pex: PexPEX,
pex_env: PexEnvironment,
python_native_code_subsystem: PythonNativeCodeSubsystem,
python_native_code_subsystem: PythonNativeCodeSubsystem.EnvironmentAware,
global_options: GlobalOptions,
pex_subsystem: PexSubsystem,
) -> Process:
Expand Down
Loading