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

Rely on the init-injection rules to trigger errors for un-depended-on and non-empty __init__.py files #10524

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/python/pants/backend/python/dependency_inference/rules.py
Expand Up @@ -52,10 +52,10 @@ def register_options(cls, register):
type=bool,
help=(
"Infer a target's dependencies on any __init__.py files existing for the packages "
"it is located in (recursively upward in the directory structure). Note that if "
"inference is disabled, empty ancestor __init__.py files will still be included "
"even without an explicit dependency, but ones containing any code (even just "
"comments) will not, and must be brought in via an explicit dependency."
"it is located in (recursively upward in the directory structure). Regardless of "
"whether inference is disabled, empty ancestor __init__.py files will still be "
"included even without an explicit dependency, but ones containing any code (even "
"just comments) will not, and must be brought in via an explicit dependency."
),
)
register(
Expand Down Expand Up @@ -138,9 +138,11 @@ async def infer_python_init_dependencies(
)

# And add dependencies on their owners.
# NB: Because the python_sources rules always locate __init__.py files, and will trigger an
# error for files that have content but have not already been included via a dependency, we
# don't need to error for unowned files here.
owners = await MultiGet(
Get(Owners, OwnersRequest((f,), OwnersNotFoundBehavior.error))
for f in extra_init_files.snapshot.files
Get(Owners, OwnersRequest((f,))) for f in extra_init_files.snapshot.files
)
return InferredDependencies(itertools.chain.from_iterable(owners))

Expand All @@ -164,6 +166,7 @@ async def infer_python_conftest_dependencies(
)

# And add dependencies on their owners.
# NB: Because conftest.py files effectively always have content, we require an owning target.
owners = await MultiGet(
Get(Owners, OwnersRequest((f,), OwnersNotFoundBehavior.error))
for f in extra_conftest_files.snapshot.files
Expand Down
20 changes: 15 additions & 5 deletions src/python/pants/backend/python/rules/missing_init.py
Expand Up @@ -3,9 +3,10 @@

from dataclasses import dataclass

from pants.backend.python.dependency_inference.rules import PythonInference
from pants.backend.python.rules.ancestor_files import AncestorFiles, AncestorFilesRequest
from pants.engine.fs import Digest, DigestContents, Snapshot
from pants.engine.rules import Get, rule
from pants.engine.rules import Get, collect_rules, rule


@dataclass(frozen=True)
Expand All @@ -26,7 +27,9 @@ class MissingNonEmptyInitFiles(Exception):


@rule
async def find_missing_empty_init_files(request: MissingInitRequest) -> MissingInit:
async def find_missing_empty_init_files(
request: MissingInitRequest, python_inference: PythonInference,
) -> MissingInit:
"""Find missing empty __init__.py files.

This is a convenience hack, so that repos that aren't using dep inference don't have
Expand All @@ -46,10 +49,17 @@ async def find_missing_empty_init_files(request: MissingInitRequest) -> MissingI
extra_init_files_contents = await Get(DigestContents, Digest, extra_init_files.snapshot.digest)
non_empty = [fc.path for fc in extra_init_files_contents if fc.content]
if non_empty:
inference_hint = (
""
if python_inference.inits
else (
", and then add explicit dependencies (or enable `--python-infer-inits` to "
"automatically add the dependencies)"
)
)
err_msg = (
f"Missing dependencies on non-empty __init__.py files: {','.join(non_empty)}. "
f"To fix, either enable dependency inference or add explicit dependencies on these "
f"files."
f"To fix: ensure that targets own each of these files{inference_hint}."
)
# TODO: Note that in the stripped case these paths will be missing their source roots,
# which makes this error message slightly less useful to the end user.
Expand All @@ -60,4 +70,4 @@ async def find_missing_empty_init_files(request: MissingInitRequest) -> MissingI


def rules():
return [find_missing_empty_init_files]
return collect_rules()
7 changes: 6 additions & 1 deletion src/python/pants/backend/python/rules/missing_init_test.py
Expand Up @@ -49,7 +49,12 @@ def assert_injected(
self.make_snapshot({fp: "" for fp in original_declared_files}),
sources_stripped=declared_files_stripped,
)
bootstrapper = create_options_bootstrapper(args=[f"--source-root-patterns={source_roots}"])
bootstrapper = create_options_bootstrapper(
args=[
"--backend-packages=pants.backend.python",
f"--source-root-patterns={source_roots}",
]
)
result = self.request_single_product(MissingInit, Params(request, bootstrapper)).snapshot
assert list(result.files) == sorted(expected_discovered)

Expand Down
2 changes: 2 additions & 0 deletions src/python/pants/backend/python/rules/python_sources_test.py
Expand Up @@ -75,6 +75,7 @@ def get_stripped_sources(
),
create_options_bootstrapper(
args=[
"--backend-packages=pants.backend.python",
f"--source-root-patterns={source_roots or ['src/python']}",
*(extra_args or []),
]
Expand All @@ -99,6 +100,7 @@ def get_unstripped_sources(
),
create_options_bootstrapper(
args=[
"--backend-packages=pants.backend.python",
f"--source-root-patterns={source_roots or ['src/python']}",
*(extra_args or []),
]
Expand Down