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

hygienic BUILD file modifications (like buildozer) with libCST #9434

Closed
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
3 changes: 2 additions & 1 deletion 3rdparty/python/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ coverage>=4.5,<4.6
dataclasses==0.6
docutils==0.14
fasteners==0.15.0
libcst==0.3.3
Markdown==2.1.1
packaging==16.8
parameterized==0.6.1
Expand All @@ -17,7 +18,7 @@ pyopenssl==17.3.0
pystache==0.5.3
python-Levenshtein==0.12.0
pywatchman==1.4.1
PyYAML==5.1.2
PyYAML==5.2
py_zipkin==0.18.4
requests[security]>=2.20.1
responses==0.10.4
Expand Down
1 change: 1 addition & 0 deletions pants.toml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ backend_packages2 = [
"pants.backend.python.lint.flake8",
"pants.backend.python.lint.isort",
"pants.backend.native",
"pants.rules.build_file_manipulation",
"internal_backend.rules_for_testing",
]

Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/base/exception_sink.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ def handle_sigterm(self, signum, _frame):
class ExceptionSink:
"""A mutable singleton object representing where exceptions should be logged to."""

original_excepthook = sys.excepthook

# NB: see the bottom of this file where we call reset_log_location() and other mutators in order
# to properly setup global state.
_log_dir = None
Expand Down
124 changes: 0 additions & 124 deletions src/python/pants/build_graph/build_file_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,127 +61,3 @@ def root_dir(self):
def registered_aliases(self):
"""Returns a copy of the registered build file aliases this build file parser uses."""
return self._build_configuration.registered_aliases()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is all dead code. It was dead code before this PR, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please break this out into a dedicated PR?

This change looks incredible, although the size of the diff makes it a little overwhelming to review. It’d be hugely helpful to break out any possible prework PRs like this so that this PR is smaller.

def address_map_from_build_files(self, build_files):
family_address_map_by_build_file = self.parse_build_files(build_files)
address_map = {}
for build_file, sibling_address_map in family_address_map_by_build_file.items():
address_map.update(sibling_address_map)
return address_map

def parse_build_files(self, build_files):
family_address_map_by_build_file = {} # {build_file: {address: addressable}}
for bf in build_files:
bf_address_map = self.parse_build_file(bf)
for address, addressable in bf_address_map.items():
for (
sibling_build_file,
sibling_address_map,
) in family_address_map_by_build_file.items():
if address in sibling_address_map:
raise self.SiblingConflictException(
"Both {conflicting_file} and {addressable_file} define the same address: "
"'{target_name}'".format(
conflicting_file=sibling_build_file,
addressable_file=address.rel_path,
target_name=address.target_name,
)
)
family_address_map_by_build_file[bf] = bf_address_map
return family_address_map_by_build_file

def parse_build_file(self, build_file):
"""Capture Addressable instances from parsing `build_file`.

Prepare a context for parsing, read a BUILD file from the filesystem, and return the
Addressable instances generated by executing the code.
"""

def _format_context_msg(lineno, offset, error_type, message):
"""Show the line of the BUILD file that has the error along with a few line of
context."""
build_contents = build_file.source().decode()
context = "While parsing {build_file}:\n".format(build_file=build_file)
curr_lineno = 0
for line in build_contents.split("\n"):
line = line.encode("ascii", "backslashreplace")
curr_lineno += 1
if curr_lineno == lineno:
highlight = "*"
else:
highlight = " "
if curr_lineno >= lineno - 3:
context += "{highlight}{curr_lineno:4d}: {line}\n".format(
highlight=highlight, line=line, curr_lineno=curr_lineno
)
if lineno == curr_lineno:
if offset:
context += " {caret:>{width}} {error_type}: {message}\n\n".format(
caret="^", width=int(offset), error_type=error_type, message=message
)
else:
context += " {error_type}: {message}\n\n".format(
error_type=error_type, message=message
)
if curr_lineno > lineno + 3:
break
return context

logger.debug("Parsing BUILD file {build_file}.".format(build_file=build_file))

try:
build_file_code = build_file.code()
except SyntaxError as e:
raise self.ParseError(_format_context_msg(e.lineno, e.offset, e.__class__.__name__, e))
except Exception as e:
raise self.ParseError(
"{error_type}: {message}\n while parsing BUILD file {build_file}".format(
error_type=e.__class__.__name__, message=e, build_file=build_file
)
)

parse_state = self._build_configuration.initialize_parse_state(build_file)
try:
with warnings.catch_warnings(record=True) as warns:
exec(build_file_code, parse_state.parse_globals)
for warn in warns:
logger.warning(
_format_context_msg(
lineno=warn.lineno,
offset=None,
error_type=warn.category.__name__,
message=warn.message,
)
)
except Exception as e:
raise self.ExecuteError(
"{message}\n while executing BUILD file {build_file}".format(
message=e, build_file=build_file
)
)

name_map = {}
for addressable in parse_state.objects:
name = addressable.addressed_name
logger.debug(
"Adding {addressable} to the BuildFileParser address map for {build_file} with {name}".format(
addressable=addressable, build_file=build_file, name=name
)
)
if name in name_map:
raise self.AddressableConflictException(
"File {conflicting_file} defines address '{target_name}' more than once.".format(
conflicting_file=build_file, target_name=name
)
)
name_map[name] = addressable

logger.debug(
"{build_file} produced the following Addressables:".format(build_file=build_file)
)
address_map = {}
for name, addressable in name_map.items():
address_map[BuildFileAddress(build_file=build_file, target_name=name)] = addressable
logger.debug(" * {name}: {addressable}".format(name=name, addressable=addressable))

return address_map
8 changes: 7 additions & 1 deletion src/python/pants/engine/build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,14 @@ def _raise_did_you_mean(address_family: AddressFamily, name: str, source=None) -
raise resolve_error


@rule
async def find_address_family(address: Address) -> AddressFamily:
return await Get[AddressFamily](Dir(address.spec_path))


@rule
async def find_build_file(address: Address) -> BuildFileAddress:
address_family = await Get[AddressFamily](Dir(address.spec_path))
address_family = await Get[AddressFamily](Address, address)
if address not in address_family.addressables:
_raise_did_you_mean(address_family=address_family, name=address.target_name)
return next(
Expand Down Expand Up @@ -293,6 +298,7 @@ def address_mapper_singleton() -> AddressMapper:
# BUILD file parsing.
hydrate_struct,
parse_address_family,
find_address_family,
find_build_file,
find_build_files,
# AddressSpec handling: locate directories that contain build files, and request
Expand Down
32 changes: 30 additions & 2 deletions src/python/pants/engine/objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
from abc import ABC, abstractmethod
from collections import namedtuple
from collections.abc import Iterable
from typing import Generic, Iterator, TypeVar
from dataclasses import dataclass
from typing import Dict, Generic, Iterator, Tuple, TypeVar

from pants.util.meta import decorated_type_checkable
from pants.util.memo import memoized_property
from pants.util.meta import decorated_type_checkable, frozen_after_init


class SerializationError(Exception):
Expand Down Expand Up @@ -213,3 +215,29 @@ def non_member_error_message(subject):
return union.define_instance_of(
cls, non_member_error_message=staticmethod(non_member_error_message)
)


_K = TypeVar("_K")
_V = TypeVar("_V")


@frozen_after_init
@dataclass(unsafe_hash=True)
class HashableDict(Generic[_K, _V]):
Copy link
Contributor

Choose a reason for hiding this comment

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

See FrozenDict from Pants.util.frozendict. I think it does what you’re after here.

_tuple_mapping: Tuple[Tuple[_K, _V], ...]

def __init__(self, mapping: Dict[_K, _V]) -> None:
self._tuple_mapping = tuple(mapping.items())

def keys(self) -> Iterable:
return tuple(entry[0] for entry in self._tuple_mapping)

def values(self) -> Iterable:
return tuple(entry[1] for entry in self._tuple_mapping)

def items(self) -> Tuple[Tuple[_K, _V], ...]:
return self._tuple_mapping

@memoized_property
def as_dict(self) -> Dict[_K, _V]:
return dict(self._tuple_mapping)
5 changes: 3 additions & 2 deletions src/python/pants/engine/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,9 @@ def _run_and_return_roots(self, session, execution_request):

if remaining_runtime_exceptions_to_capture:
raise ExecutionError(
"Internal logic error in scheduler: expected elements in "
"`self._native._peek_cffi_extern_method_runtime_exceptions()`."
"Internal logic error in scheduler: expected any elements in "
"`self._native._peek_cffi_extern_method_runtime_exceptions()`. ",
wrapped_exceptions=remaining_runtime_exceptions_to_capture,
)
return roots

Expand Down
6 changes: 5 additions & 1 deletion src/python/pants/goal/run_tracker.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,11 @@ def new_workunit(self, name, labels=None, cmd="", log_config=None):

:API: public
"""
parent = self._threadlocal.current_workunit
try:
parent = self._threadlocal.current_workunit
except AttributeError:
parent = self._main_root_workunit

with self.new_workunit_under_parent(
name, parent=parent, labels=labels, cmd=cmd, log_config=log_config
) as workunit:
Expand Down
12 changes: 12 additions & 0 deletions src/python/pants/rules/build_file_manipulation/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

python_library(
dependencies=[
'3rdparty/python:dataclasses',
'3rdparty/python:libcst',
'src/python/pants/engine:goal',
'src/python/pants/engine:rules',
],
tags={'partially_type_checked'},
)
Loading