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

New env() BUILD file function. #17652

Merged
merged 9 commits into from
Jan 11, 2023
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
16 changes: 16 additions & 0 deletions docs/markdown/Using Pants/concepts/targets.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,22 @@ All target types have a `name` field, which is used to identify the target. Targ

You can autoformat `BUILD` files by enabling a `BUILD` file formatter by adding it to `[GLOBAL].backend_packages` in `pants.toml` (such as `pants.backend.build_files.fmt.black` [or others](doc:enabling-backends)). Then to format, run `./pants fmt '**/BUILD'` or `./pants fmt ::` (formats everything).

Environment variables
---------------------

BUILD files are very hermetic in nature with no support for using `import` or other I/O operations. In order to have dynamic data in BUILD files, you may inject values from the local environment using the `env()` function. It takes the variable name and optional default value as arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to snipe this review after merge. Can we document whether sensitive env vars are safe to load? Why/why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like a good idea. I don't know the answer though.. :)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I mean... it just depends where they are being used, I suppose? Technically you could use this to interpolate an env var into a target name or something, at which point your secret would leak via ./pants list.

So it's very use-site specific.


```python helloworld/pkg/BUILD
python_distribution(
name="helloworld-dist",
description=env("DIST_DESC", "Set the `DIST_DESC` env variable to override this value."),
provides=python_artifact(
name="helloworld",
version=env("HELLO_WORLD_VERSION"),
),
)
```

Target addresses
================

Expand Down
78 changes: 74 additions & 4 deletions src/python/pants/engine/internals/build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@

from __future__ import annotations

import ast
import builtins
import itertools
import logging
import os.path
import sys
from dataclasses import dataclass
from pathlib import PurePath
from typing import Any, cast
from typing import Any, Sequence, cast

from pants.build_graph.address import (
Address,
Expand All @@ -19,7 +22,8 @@
ResolveError,
)
from pants.engine.engine_aware import EngineAwareParameter
from pants.engine.fs import DigestContents, GlobMatchErrorBehavior, PathGlobs, Paths
from pants.engine.env_vars import CompleteEnvironmentVars, EnvironmentVars, EnvironmentVarsRequest
from pants.engine.fs import DigestContents, FileContent, GlobMatchErrorBehavior, PathGlobs, Paths
from pants.engine.internals.defaults import BuildFileDefaults, BuildFileDefaultsParserState
from pants.engine.internals.dep_rules import (
BuildFileDependencyRules,
Expand All @@ -28,12 +32,13 @@
)
from pants.engine.internals.mapper import AddressFamily, AddressMap
from pants.engine.internals.parser import BuildFilePreludeSymbols, Parser, error_on_imports
from pants.engine.internals.session import SessionValues
from pants.engine.internals.synthetic_targets import (
SyntheticAddressMaps,
SyntheticAddressMapsRequest,
)
from pants.engine.internals.target_adaptor import TargetAdaptor, TargetAdaptorRequest
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.rules import Get, MultiGet, collect_rules, rule, rule_helper
from pants.engine.target import (
DependenciesRuleApplication,
DependenciesRuleApplicationRequest,
Expand All @@ -45,6 +50,8 @@
from pants.util.frozendict import FrozenDict
from pants.util.strutil import softwrap

logger = logging.getLogger(__name__)


@dataclass(frozen=True)
class BuildFileOptions:
Expand Down Expand Up @@ -169,6 +176,62 @@ async def ensure_address_family(request: OptionalAddressFamily) -> AddressFamily
return request.ensure()


class BUILDFileEnvVarExtractor(ast.NodeVisitor):
def __init__(self, filename: str):
super().__init__()
self.env_vars: set[str] = set()
self.filename = filename

@classmethod
def get_env_vars(cls, file_content: FileContent) -> Sequence[str]:
obj = cls(file_content.path)
obj.visit(ast.parse(file_content.content, file_content.path))
return tuple(obj.env_vars)

def visit_Call(self, node: ast.Call):
is_env = isinstance(node.func, ast.Name) and node.func.id == "env"
for arg in node.args:
if not is_env:
self.visit(arg)
continue

# Only first arg may be checked as env name
is_env = False

if sys.version_info[0:2] < (3, 8):
value = arg.s if isinstance(arg, ast.Str) else None
else:
value = arg.value if isinstance(arg, ast.Constant) else None
if value:
# Found env name in this call, we're done here.
self.env_vars.add(value)
return
kaos marked this conversation as resolved.
Show resolved Hide resolved
else:
logging.warning(
f"{self.filename}:{arg.lineno}: Only constant string values as variable name to "
f"`env()` is currently supported. This `env()` call will always result in "
"the default value only."
)

for kwarg in node.keywords:
self.visit(kwarg)


@rule_helper
async def _extract_env_vars(
file_content: FileContent, env: CompleteEnvironmentVars
) -> EnvironmentVars:
"""For BUILD file env vars, we only ever consult the local systems env."""
env_vars = BUILDFileEnvVarExtractor.get_env_vars(file_content)
return await Get(
EnvironmentVars,
{
EnvironmentVarsRequest(env_vars): EnvironmentVarsRequest,
env: CompleteEnvironmentVars,
},
)


@rule(desc="Search for addresses in BUILD files")
async def parse_address_family(
parser: Parser,
Expand All @@ -178,6 +241,7 @@ async def parse_address_family(
registered_target_types: RegisteredTargetTypes,
union_membership: UnionMembership,
maybe_build_file_dependency_rules_implementation: MaybeBuildFileDependencyRulesImplementation,
session_values: SessionValues,
) -> OptionalAddressFamily:
"""Given an AddressMapper and a directory, return an AddressFamily.

Expand Down Expand Up @@ -235,17 +299,23 @@ async def parse_address_family(
dependents_rules_parser_state = None
dependencies_rules_parser_state = None

all_env_vars = [
await _extract_env_vars(fc, session_values[CompleteEnvironmentVars])
for fc in digest_contents
]

address_maps = [
AddressMap.parse(
fc.path,
fc.content.decode(),
parser,
prelude_symbols,
env_vars,
defaults_parser_state,
dependents_rules_parser_state,
dependencies_rules_parser_state,
)
for fc in digest_contents
for fc, env_vars in zip(digest_contents, all_env_vars)
]

# Freeze defaults and dependency rules
Expand Down
75 changes: 75 additions & 0 deletions src/python/pants/engine/internals/build_files_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from __future__ import annotations

import logging
import re
from textwrap import dedent
from typing import cast
Expand All @@ -13,6 +14,7 @@
from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.core.target_types import GenericTarget
from pants.engine.addresses import Address, AddressInput, BuildFileAddress
from pants.engine.env_vars import CompleteEnvironmentVars, EnvironmentVars, EnvironmentVarsRequest
from pants.engine.fs import DigestContents, FileContent, PathGlobs
from pants.engine.internals.build_files import (
AddressFamilyDir,
Expand All @@ -27,6 +29,7 @@
from pants.engine.internals.parametrize import Parametrize
from pants.engine.internals.parser import BuildFilePreludeSymbols, Parser
from pants.engine.internals.scheduler import ExecutionError
from pants.engine.internals.session import SessionValues
from pants.engine.internals.synthetic_targets import (
SyntheticAddressMaps,
SyntheticAddressMapsRequest,
Expand All @@ -41,6 +44,7 @@
Target,
)
from pants.engine.unions import UnionMembership
from pants.testutil.pytest_util import assert_logged
from pants.testutil.rule_runner import (
MockGet,
QueryRule,
Expand All @@ -49,6 +53,7 @@
run_rule_with_mocks,
)
from pants.util.frozendict import FrozenDict
from pants.util.strutil import softwrap


def test_parse_address_family_empty() -> None:
Expand All @@ -69,6 +74,7 @@ def test_parse_address_family_empty() -> None:
RegisteredTargetTypes({}),
UnionMembership({}),
MaybeBuildFileDependencyRulesImplementation(None),
SessionValues({CompleteEnvironmentVars: CompleteEnvironmentVars({})}),
],
mock_gets=[
MockGet(
Expand All @@ -86,6 +92,11 @@ def test_parse_address_family_empty() -> None:
input_types=(SyntheticAddressMapsRequest,),
mock=lambda _: SyntheticAddressMaps(),
),
MockGet(
output_type=EnvironmentVars,
input_types=(EnvironmentVarsRequest, CompleteEnvironmentVars),
mock=lambda _1, _2: EnvironmentVars({}),
),
],
)
assert optional_af.path == "/dev/null"
Expand Down Expand Up @@ -521,3 +532,67 @@ def uses_undefined():
# Parse the root BUILD file.
address_family = rule_runner.request(AddressFamily, [AddressFamilyDir("")])
assert not address_family.name_to_target_adaptors


def test_build_file_env_vars(target_adaptor_rule_runner: RuleRunner) -> None:
target_adaptor_rule_runner.write_files(
{
"BUILD": dedent(
"""
mock_tgt(
description=env("MOCK_DESC"),
tags=[
env("DEF", "default"),
env("TAG", "default"),
]
)
"""
),
},
)
target_adaptor_rule_runner.set_options([], env={"MOCK_DESC": "from env", "TAG": "tag"})
target_adaptor = target_adaptor_rule_runner.request(
TargetAdaptor,
[TargetAdaptorRequest(Address(""), description_of_origin="tests")],
)
assert target_adaptor.kwargs["description"] == "from env"
assert target_adaptor.kwargs["tags"] == ["default", "tag"]


def test_invalid_build_file_env_vars(caplog, target_adaptor_rule_runner: RuleRunner) -> None:
target_adaptor_rule_runner.write_files(
{
"src/bad/BUILD": dedent(
"""
DOES_NOT_WORK = "var_name1"
DO_THIS_INSTEAD = env("var_name2")

mock_tgt(description=env(DOES_NOT_WORK), tags=[DO_THIS_INSTEAD])
"""
),
},
)
target_adaptor_rule_runner.set_options(
[], env={"var_name1": "desc from env", "var_name2": "tag-from-env"}
)
target_adaptor = target_adaptor_rule_runner.request(
TargetAdaptor,
[TargetAdaptorRequest(Address("src/bad"), description_of_origin="tests")],
)
assert target_adaptor.kwargs["description"] is None
assert target_adaptor.kwargs["tags"] == ["tag-from-env"]
assert_logged(
caplog,
[
(
logging.WARNING,
softwrap(
"""
src/bad/BUILD:5: Only constant string values as variable name to `env()` is
currently supported. This `env()` call will always result in the default value
only.
"""
),
),
],
)
3 changes: 3 additions & 0 deletions src/python/pants/engine/internals/mapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from pants.backend.project_info.filter_targets import FilterSubsystem
from pants.base.exceptions import MappingError
from pants.build_graph.address import Address, BuildFileAddress
from pants.engine.env_vars import EnvironmentVars
from pants.engine.internals.defaults import BuildFileDefaults, BuildFileDefaultsParserState
from pants.engine.internals.dep_rules import (
BuildFileDependencyRules,
Expand Down Expand Up @@ -43,6 +44,7 @@ def parse(
build_file_content: str,
parser: Parser,
extra_symbols: BuildFilePreludeSymbols,
env_vars: EnvironmentVars,
defaults: BuildFileDefaultsParserState,
dependents_rules: BuildFileDependencyRulesParserState | None,
dependencies_rules: BuildFileDependencyRulesParserState | None,
Expand All @@ -57,6 +59,7 @@ def parse(
filepath,
build_file_content,
extra_symbols,
env_vars,
defaults,
dependents_rules,
dependencies_rules,
Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/engine/internals/mapper_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from pants.build_graph.address import Address
from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.core.target_types import GenericTarget
from pants.engine.env_vars import EnvironmentVars
from pants.engine.internals.defaults import BuildFileDefaults, BuildFileDefaultsParserState
from pants.engine.internals.mapper import (
AddressFamily,
Expand Down Expand Up @@ -42,6 +43,7 @@ def parse_address_map(build_file: str, *, ignore_unrecognized_symbols: bool = Fa
build_file,
parser,
BuildFilePreludeSymbols(FrozenDict()),
EnvironmentVars({}),
BuildFileDefaultsParserState.create(
"", BuildFileDefaults({}), RegisteredTargetTypes({}), UnionMembership({})
),
Expand Down
8 changes: 7 additions & 1 deletion src/python/pants/engine/internals/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from pants.base.exceptions import MappingError
from pants.base.parse_context import ParseContext
from pants.build_graph.build_file_aliases import BuildFileAliases
from pants.engine.env_vars import EnvironmentVars
from pants.engine.internals.defaults import BuildFileDefaultsParserState, SetDefaultsT
from pants.engine.internals.dep_rules import BuildFileDependencyRulesParserState
from pants.engine.internals.target_adaptor import TargetAdaptor
Expand Down Expand Up @@ -213,6 +214,7 @@ def parse(
filepath: str,
build_file_content: str,
extra_symbols: BuildFilePreludeSymbols,
env_vars: EnvironmentVars,
defaults: BuildFileDefaultsParserState,
dependents_rules: BuildFileDependencyRulesParserState | None,
dependencies_rules: BuildFileDependencyRulesParserState | None,
Expand All @@ -224,7 +226,11 @@ def parse(
dependencies_rules=dependencies_rules,
)

global_symbols = {**self._symbols, **extra_symbols.symbols}
global_symbols: dict[str, Any] = {
"env": env_vars.get,
**self._symbols,
**extra_symbols.symbols,
}

if self.ignore_unrecognized_symbols:
defined_symbols = set()
Expand Down
Loading