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.

Sign up for GitHub

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

Jump to bottom

Support pex_binary addresses in setup_py(entry_points) along with dep… #12414

Merged
merged 7 commits into from
Jul 27, 2021
Merged

Support pex_binary addresses in setup_py(entry_points) along with dep… #12414

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8e90cc
Support pex_binary addresses in setup_py(entry_points) along with dep…
kaos Jul 23, 2021
4ab55a1
fix missing resolve of target addresses in setup_py(entry_points)
kaos Jul 23, 2021
fc73526
Update src/python/pants/backend/python/target_types_rules.py
kaos Jul 27, 2021
262b4e6
apply review feedback.
kaos Jul 27, 2021
43197b1
Merge remote-tracking branch 'upstream/main' into setup_py_entry_poin…
kaos Jul 27, 2021
85b02c7
cleanup `.with_binaries()` deprecation message.
kaos Jul 27, 2021
0bcccc7
rename test
kaos Jul 27, 2021
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
103 changes: 37 additions & 66 deletions src/python/pants/backend/python/goals/setup_py.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,11 @@
from pants.backend.python.macros.python_artifact import PythonArtifact
from pants.backend.python.subsystems.setuptools import Setuptools
from pants.backend.python.target_types import (
PexEntryPointField,
PythonDistributionEntryPointsField,
PythonProvidesField,
PythonRequirementsField,
PythonSources,
ResolvedPexEntryPoint,
ResolvedPythonDistributionEntryPoints,
ResolvePexEntryPointRequest,
ResolvePythonDistributionEntryPointsRequest,
SetupPyCommandsField,
)
Expand Down Expand Up @@ -563,72 +560,46 @@ async def generate_chroot(request: SetupPyChrootRequest) -> SetupPyChroot:
}
)

# Collect any `pex_binary` targets from `setup_py().with_binaries()`
key_to_binary_spec = exported_target.provides.binaries
binaries = await Get(
Targets, UnparsedAddressInputs(key_to_binary_spec.values(), owning_address=target.address)
)
entry_point_requests = []
for binary in binaries:
if not binary.has_field(PexEntryPointField):
raise InvalidEntryPoint(
"Expected addresses to `pex_binary` targets in `.with_binaries()` for the "
f"`provides` field for {exported_addr}, but found {binary.address} with target "
f"type {binary.alias}."
)
entry_point = binary[PexEntryPointField].value
url = "https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-console-scripts-entry-point"
if not entry_point.function:
raise InvalidEntryPoint(
"Every `pex_binary` used in `with_binaries()` for the `provides()` field for "
f"{exported_addr} must end in the format `:my_func` for the `entry_point` field, "
f"but {binary.address} set it to {entry_point.spec!r}. For example, set "
f"`entry_point='{entry_point.module}:main'. See {url}."
)
entry_point_requests.append(ResolvePexEntryPointRequest(binary[PexEntryPointField]))

binary_entry_points = await MultiGet(
Get(ResolvedPexEntryPoint, ResolvePexEntryPointRequest, request)
for request in entry_point_requests
)

entry_points_from_with_binaries = {
key: binary_entry_point.val.spec
for key, binary_entry_point in zip(key_to_binary_spec.keys(), binary_entry_points)
if binary_entry_point.val is not None
}

# Collect entry points from `python_distribution(entry_points=...)`
entry_points_from_field = {}
if exported_target.target.has_field(PythonDistributionEntryPointsField):
entry_points_field = await Get(
# Resolve entry points from python_distribution(entry_points=...) and from
# python_distribution(provides=setup_py(entry_points=...).with_binaries(...)
entry_points_field, provides_field = await MultiGet(
Get(
ResolvedPythonDistributionEntryPoints,
ResolvePythonDistributionEntryPointsRequest(
exported_target.target[PythonDistributionEntryPointsField]
entry_points_field=exported_target.target.get(PythonDistributionEntryPointsField)
),
)
Copy link
Member Author

Choose a reason for hiding this comment

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

got to clean up a lot of code, as that functionality moved to resolve entry points rule.

),
Get(
ResolvedPythonDistributionEntryPoints,
ResolvePythonDistributionEntryPointsRequest(
provides_field=exported_target.target.get(PythonProvidesField)
),
),
)

entry_points_from_field = {
category: {ep_name: ep_val.entry_point.spec for ep_name, ep_val in entry_points.items()}
for category, entry_points in entry_points_field.val.items()
}
entry_points_from_field = {
category: {ep_name: ep_val.entry_point.spec for ep_name, ep_val in entry_points.items()}
for category, entry_points in entry_points_field.val.items()
}

# Collect any entry points from the setup_py() object. Note that this was already normalized in
# `python_artifacts.py`.
entry_points_from_provides = setup_kwargs.get("entry_points", {})
entry_points_from_provides = {
category: {ep_name: ep_val.entry_point.spec for ep_name, ep_val in entry_points.items()}
for category, entry_points in provides_field.val.items()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally, we prefer comprehensions like this. But here, it looks like you're iterating over entry_points.items() more than necessary, and so the style guide says:

If constructing multiple collections by iterating over the same original collection, use a for loop for performance.

Here, I think you would have for ep_name, ep_val in entry_points.items() in your outer loop. Then two inner loops over provides_field.val.items() and entry_points_field.val.items().

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh.. it is not the same entry_points.items(), the first comes from entry_points_field.val, the other from provides_field.val. ;)

Could however have a little helper function to reduce code duplication, and this would become super clear :)


# Gather entry points with source description for any error messages when merging them.
entry_point_sources = {
f"{exported_addr}'s field `provides=setup_py().with_binaries()`": {
"console_scripts": entry_points_from_with_binaries
},
f"{exported_addr}'s field `entry_points`": entry_points_from_field,
f"{exported_addr}'s field `provides=setup_py(..., entry_points={...})`": entry_points_from_provides,
Copy link
Member Author

Choose a reason for hiding this comment

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

The with_binaries() are now merged during resolve of the provides field, so we're left with the two resolves for each field, entry points and provides, respectively. The downside is, that when there is a conflict with a entry point from the distributions field, we can't say if it is with setup_py().with_binaries or with setup_py(entry_points) any more..

Copy link
Contributor

Choose a reason for hiding this comment

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

That is totally reasonable, esp. becauase with_binaries() is now deprecated. Thanks for the disclaimer!

f"{exported_addr}'s field `provides=setup_py()`": entry_points_from_provides,
}

# Merge all collected entry points and add them to the dist's entry points.
entry_points = merge_entry_points(*list(entry_point_sources.items()))
if entry_points:
setup_kwargs["entry_points"] = entry_points
all_entry_points = merge_entry_points(*list(entry_point_sources.items()))
if all_entry_points:
setup_kwargs["entry_points"] = {
category: [f"{name} = {entry_point}" for name, entry_point in entry_points.items()]
for category, entry_points in all_entry_points.items()
}

# Generate the setup script.
setup_py_content = SETUP_BOILERPLATE.format(
Expand Down Expand Up @@ -1011,7 +982,7 @@ def has_args(call_node: ast.Call, required_arg_ids: Tuple[str, ...]) -> bool:

def merge_entry_points(
*all_entry_points_with_descriptions_of_source: Tuple[str, Dict[str, Dict[str, str]]]
) -> Dict[str, List[str]]:
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed signature, to stay true to our "normalized" structure for entry points. The List[str] form for entry points is applied at the last moment when outputting for the setup kwargs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good change!

) -> Dict[str, Dict[str, str]]:
"""Merge all entry points, throwing ValueError if there are any conflicts."""
merged = cast(
# this gives us a two level deep defaultdict with the inner values being of list type
Expand All @@ -1024,22 +995,22 @@ def merge_entry_points(
for ep_name, entry_point in entry_points.items():
merged[category][ep_name].append((description_of_source, entry_point))

def _merge_entry_point(
Copy link
Member Author

Choose a reason for hiding this comment

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

adjusted name to be true to what it does

def _check_entry_point_single_source(
category: str, name: str, entry_points_with_source: List[Tuple[str, str]]
):
) -> Tuple[str, str]:
if len(entry_points_with_source) > 1:
raise ValueError(
f"Multiple entry_points registered for {category} {name} in: "
f"{', '.join(ep_source for ep_source, _ in entry_points_with_source)}"
)
for _, entry_point in entry_points_with_source:
return f"{name}={entry_point}"
_, entry_point = entry_points_with_source[0]
return name, entry_point
Comment on lines -1035 to +1009
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent change! This is much more obvious what it does. Nice :D

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the most rewarding part to me, tbh. Learning to write comprehensible code. Normally, it's just I that have to grok my code, and my peers are lost in the mist of what it is supposed to do in the first place, so there is no real pressure to be super clear with the intentions. But, it will be a lot easier to maintain in the long run, so well worth spending a few minutes to get there. So, that's what I'm happy about, to be able to get there with more ease than before.. ;)


return {
category: [
_merge_entry_point(category, name, entry_points_with_source)
category: dict(
_check_entry_point_single_source(category, name, entry_points_with_source)
for name, entry_points_with_source in merged_entry_points.items()
]
)
for category, merged_entry_points in merged.items()
}

Expand Down
42 changes: 22 additions & 20 deletions src/python/pants/backend/python/goals/setup_py_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,15 +230,15 @@ def test_merge_entry_points() -> None:
},
}
expect = {
"console_scripts": [
"foo_tool=foo.bar.baz:Tool.main",
"foo_qux=foo.baz.qux",
"foo_main=foo.qux.bin:main",
],
"foo_plugins": [
"qux=foo.qux",
"foo-bar=foo.bar:plugin",
],
"console_scripts": {
"foo_tool": "foo.bar.baz:Tool.main",
"foo_qux": "foo.baz.qux",
"foo_main": "foo.qux.bin:main",
},
"foo_plugins": {
"qux": "foo.qux",
"foo-bar": "foo.bar:plugin",
},
}
assert merge_entry_points(*list(sources.items())) == expect

Expand Down Expand Up @@ -346,7 +346,7 @@ def test_generate_chroot(chroot_rule_runner: RuleRunner) -> None:
"namespace_packages": ("foo",),
"package_data": {"foo": ("resources/js/code.js",)},
"install_requires": ("baz==1.1.1",),
"entry_points": {"console_scripts": ["foo_main=foo.qux.bin:main"]},
"entry_points": {"console_scripts": ["foo_main = foo.qux.bin:main"]},
},
Address("src/python/foo", target_name="foo-dist"),
)
Expand Down Expand Up @@ -384,7 +384,8 @@ def test_generate_chroot_entry_points(chroot_rule_runner: RuleRunner) -> None:
name='foo', version='1.2.3',
entry_points={
"console_scripts":{
"foo_qux":"foo.baz.qux",
"foo_qux":"foo.baz.qux:main",
Copy link
Member Author

Choose a reason for hiding this comment

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

Good to see that we catch bugs in the test suite :P

Copy link
Contributor

Choose a reason for hiding this comment

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

So, before it was inferring a dep on the python_library and now it does it on the pex_binary? Cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, there are too many foo, bar, baz and qux in this test. But no, I don't think so. It refers to a module (that is not part of any target, so "orphan") but being a console script, it must point at a method, not just a module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, there are too many foo, bar, baz and qux in this test

Yep, agreed. I personally don't like foo and friends. https://www.techyourchance.com/never-use-foo-bar-baz/ Feel free to update in another change if you'd like :)

"foo_bin":":foo-bin",
Copy link
Member Author

@kaos kaos Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

second commit was to pick this up, which it wasn't before..
that is, target address to pex_binary in entry points of setup_py..

},
"foo_plugins":[
"foo-bar=foo.bar:plugin",
Expand Down Expand Up @@ -422,16 +423,17 @@ def test_generate_chroot_entry_points(chroot_rule_runner: RuleRunner) -> None:
"install_requires": tuple(),
"entry_points": {
"console_scripts": [
"foo_main=foo.qux.bin:main",
"foo_tool=foo.bar.baz:Tool.main",
"bin_tool=foo.qux.bin:main",
"bin_tool2=foo.qux.bin:main",
"hello=foo.bin:main",
"foo_qux=foo.baz.qux",
"foo_tool = foo.bar.baz:Tool.main",
"bin_tool = foo.qux.bin:main",
"bin_tool2 = foo.qux.bin:main",
"hello = foo.bin:main",
"foo_qux = foo.baz.qux:main",
"foo_bin = foo.bin:main",
"foo_main = foo.qux.bin:main",
],
"foo_plugins": [
"qux=foo.qux",
"foo-bar=foo.bar:plugin",
"qux = foo.qux",
"foo-bar = foo.bar:plugin",
],
},
},
Expand Down Expand Up @@ -516,7 +518,7 @@ def test_binary_shorthand(chroot_rule_runner: RuleRunner) -> None:
"namespace_packages": (),
"install_requires": (),
"package_data": {},
"entry_points": {"console_scripts": ["foo=project.app:func"]},
"entry_points": {"console_scripts": ["foo = project.app:func"]},
},
Address("src/python/project", target_name="dist"),
)
Expand Down
14 changes: 11 additions & 3 deletions src/python/pants/backend/python/target_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ class PythonDistributionDependencies(Dependencies):
supports_transitive_excludes = True


class PythonProvidesField(ScalarField, ProvidesField):
class PythonProvidesField(ScalarField, ProvidesField, AsyncFieldMixin):
expected_type = PythonArtifact
expected_type_help = "setup_py(name='my-dist', **kwargs)"
value: PythonArtifact
Expand Down Expand Up @@ -808,9 +808,17 @@ def pex_binary_addresses(self) -> Addresses:
@dataclass(frozen=True)
class ResolvePythonDistributionEntryPointsRequest:
"""Looks at the entry points to see if it is a setuptools entry point, or a BUILD target address
that should be resolved into a setuptools entry point."""
that should be resolved into a setuptools entry point.

entry_points_field: PythonDistributionEntryPointsField
If the `entry_points_field` is present, inspect the specified entry points.
If the `provides_field` is present, inspect the `provides_field.kwargs["entry_points"]`.

This is to support inspecting one or the other depending on use case, using the same
logic for resolving pex_binary addresses etc.
"""

entry_points_field: Optional[PythonDistributionEntryPointsField] = None
provides_field: Optional[PythonProvidesField] = None
Copy link
Member Author

Choose a reason for hiding this comment

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

I extended the support for fields to look at for resolving entry points, so we get the same logic applied consistently across all different ways to provide entry points.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be extra defensive, you could add a def __post__init() that raises AssertionError if both are unspecified.



class SetupPyCommandsField(StringSequenceField):
Expand Down
Loading