Skip to content

Commit

Permalink
Fix the dep inference scripts source root bug. (pantsbuild#18164)
Browse files Browse the repository at this point in the history
The dep inference script used to be a single file that did no importing (except from the stdlib).
And we treated the script as both a resource, for writing into the dep inference sandbox at Pants
run time, and as a Python source, so we can fmt/lint/typecheck it at Pants build time.

But pantsbuild#17997 split up that script to make it more modular, so that it now imports other script code, 
relative to the scripts dir. Now, for the fmt/lint/typecheck to succeed, those imports have to correspond
to a source root, so we added `src/python/pants/backend/python/dependency_inference/scripts/`
as a source root.

But this means that when packaging the `pantsbuild.pants` wheel, those scripts will be placed relative to
the root package, rather than nested in `pants.backend.python.dependency_inference.scripts`. And we 
loading the script contents as resources relative to that package.

This PR:

- Fixes this by removing that nested source root (which is confusing anyway). So those inter-script
imports must now specify the entire `pants.backend.python.dependency_inference.scripts` prefix.
- Gets rid of the `_pants_dep_parser` script package, which was intended to namespace the scripts under
the nested source root, since we now have `pants.backend.python.dependency_inference.scripts` as
a namespace.
- Ensures that all the intermediate `__init__.py` are present in the scripts sandbox, so the scripts
can import on Python 2.
- Gets rid of the resource/code duality in the BUILD file. Python can load source file contents as
resources, so just depending on the scripts as python_sources is fine.
  • Loading branch information
benjyw authored Feb 5, 2023
1 parent efc1c2e commit 3da5ae5
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 28 deletions.
1 change: 0 additions & 1 deletion pants.toml
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ root_patterns = [
"test/*",
"tests/*",
"3rdparty/*",
"src/python/pants/backend/python/dependency_inference/scripts",
"/build-support/bin",
"/build-support/flake8",
"/build-support/migration-support",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import json
import os
from dataclasses import dataclass
from typing import Iterable

Expand Down Expand Up @@ -61,7 +62,7 @@ class PythonDependencyVisitorRequest:

@dataclass(frozen=True)
class PythonDependencyVisitor:
"""Wraps a subclass of _pants_dep_parser.DependencyVisitorBase."""
"""Wraps a subclass of DependencyVisitorBase."""

digest: Digest # The file contents for the visitor
classname: str # The full classname, e.g., _my_custom_dep_parser.MyCustomVisitor
Expand All @@ -74,13 +75,22 @@ class ParserScript:
env: FrozenDict[str, str]


_scripts_module = "pants.backend.python.dependency_inference.scripts"


@rule_helper
async def _get_script_digest(relpaths: Iterable[str]) -> Digest:
scripts = [read_resource(__name__, f"scripts/{relpath}") for relpath in relpaths]
scripts = [read_resource(_scripts_module, relpath) for relpath in relpaths]
assert all(script is not None for script in scripts)
path_prefix = _scripts_module.replace(".", os.path.sep)
digest = await Get(
Digest,
CreateDigest([FileContent(relpath, script) for relpath, script in zip(relpaths, scripts)]),
CreateDigest(
[
FileContent(os.path.join(path_prefix, relpath), script)
for relpath, script in zip(relpaths, scripts)
]
),
)
return digest

Expand All @@ -94,12 +104,27 @@ async def get_parser_script(union_membership: UnionMembership) -> ParserScript:
)
utils = await _get_script_digest(
[
"_pants_dep_parser/__init__.py",
"_pants_dep_parser/dependency_visitor_base.py",
"_pants_dep_parser/main.py",
"dependency_visitor_base.py",
"main.py",
]
)
digest = await Get(Digest, MergeDigests([utils, *(dv.digest for dv in dep_visitors)]))

# Python 2 requires all the intermediate __init__.py to exist in the sandbox.
init_contents = []
module = _scripts_module
while module:
init_contents.append(
FileContent(
os.path.join(module.replace(".", os.path.sep), "__init__.py"),
read_resource(module, "__init__.py"),
)
)
module = module.rpartition(".")[0]
init_scaffolding = await Get(Digest, CreateDigest(init_contents))

digest = await Get(
Digest, MergeDigests([utils, init_scaffolding, *(dv.digest for dv in dep_visitors)])
)
env = {
"VISITOR_CLASSNAMES": "|".join(dv.classname for dv in dep_visitors),
"PYTHONPATH": ".",
Expand Down Expand Up @@ -131,10 +156,11 @@ async def general_parser_script(
python_infer_subsystem: PythonInferSubsystem,
request: GeneralPythonDependencyVisitorRequest,
) -> PythonDependencyVisitor:
script_digest = await _get_script_digest(["_pants_dep_parser/general_dependency_visitor.py"])
script_digest = await _get_script_digest(["general_dependency_visitor.py"])
classname = f"{_scripts_module}.general_dependency_visitor.GeneralDependencyVisitor"
return PythonDependencyVisitor(
digest=script_digest,
classname="_pants_dep_parser.general_dependency_visitor.GeneralDependencyVisitor",
classname=classname,
env=FrozenDict(
{
"STRING_IMPORTS": "y" if python_infer_subsystem.string_imports else "n",
Expand Down Expand Up @@ -168,7 +194,7 @@ async def parse_python_dependencies(
Process(
argv=[
python_interpreter.path,
"./_pants_dep_parser/main.py",
"pants/backend/python/dependency_inference/scripts/main.py",
file,
],
input_digest=input_digest,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,4 @@
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

# Scripts are consumed at runtime as resources.
resources(
sources=["**/*.py"],
)

# Also expose the scripts as python sources so they get formatted/linted/checked.
python_sources(
name="py_sources",
sources=["**/*.py"],
)
python_sources()
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
import re
import tokenize

from _pants_dep_parser.dependency_visitor_base import DependencyVisitorBase
from pants.backend.python.dependency_inference.scripts.dependency_visitor_base import (
DependencyVisitorBase,
)


class GeneralDependencyVisitor(DependencyVisitorBase):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@
# NB: This must be compatible with Python 2.7 and 3.5+.
# NB: If you're needing to debug this, an easy way is to just invoke it on a file.
# E.g.
# $ export ROOT=src/python/pants/backend/python/dependency_inference/scripts/
# $ PYTHONPATH=$ROOT STRING_IMPORTS=y python $ROOT/_pants_dep_parser/main.py FILE
# $ PYTHONPATH=src/python STRING_IMPORTS=y python \
# src/python/pants/backend/python/dependency_inference/scripts/main.py FILE
# Or
# $ ./pants --no-python-infer-imports run \
# src/python/pants/backend/python/dependency_inference/scripts/_pants_dep_parser/main.py \
# -- src/python/pants/base/specs.py
# src/python/pants/backend/python/dependency_inference/scripts/main.py -- FILE

from __future__ import print_function, unicode_literals

Expand All @@ -21,7 +20,9 @@
import sys
from io import open

from _pants_dep_parser.dependency_visitor_base import FoundDependencies
from pants.backend.python.dependency_inference.scripts.dependency_visitor_base import (
FoundDependencies,
)


def main(filename):
Expand All @@ -35,7 +36,7 @@ def main(filename):
package_parts = os.path.dirname(filename).split(os.path.sep)
visitor_classnames = os.environ.get(
"VISITOR_CLASSNAMES",
"_pants_dep_parser.general_dependency_visitor.GeneralDependencyVisitor",
"pants.backend.python.dependency_inference.scripts.general_dependency_visitor.GeneralDependencyVisitor",
).split("|")
visitors = []
found_dependencies = FoundDependencies()
Expand Down

0 comments on commit 3da5ae5

Please sign in to comment.