Start creating binary goal with support for python
#8399
Conversation
src/python/pants/backend/python/rules/interpreter_constraints.py
Outdated
Show resolved
Hide resolved
src/python/pants/backend/python/rules/interpreter_constraints.py
Outdated
Show resolved
Hide resolved
gshuflin
force-pushed
the
v2-binary
branch
2 times, most recently
from
cb17e91 to
7ab2758
Compare
| for target_adaptor in all_targets | ||
| ] | ||
|
|
||
| #TODO This way of calculating the entry point works but is a bit hackish. |
There was a problem hiding this comment.
Would you mind expanding on this before landing? or opening a ticket?
There was a problem hiding this comment.
Opened a ticket for this.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class CreatePex: | ||
| """Represents a generic request to create a PEX from its inputs.""" | ||
| output_filename: str | ||
| requirements: Tuple[str] = () | ||
| interpreter_constraints: Tuple[str] = () | ||
| requirements: PexRequirements = PexRequirements() |
There was a problem hiding this comment.
What about instead making this Optional[PexRequirements] = None? That more clearly signals to me that there are no requirements by default, rather than having to rely on knowing that PexRequirement() will be empty by default.
There was a problem hiding this comment.
Hm, I have a slight preference for letting the type of this member be non-optional PexRequirements, since it means there's not two ways to represent "no requirements" (None and empty PexRequirements()) with identical semantics. Same for PexInterpreterConstraints.
There was a problem hiding this comment.
Yep. @Eric-Arellano : this is the "null object" pattern.
| requirements: Tuple[str] = () | ||
| interpreter_constraints: Tuple[str] = () | ||
| requirements: PexRequirements = PexRequirements() | ||
| interpreter_constraints: PexInterpreterContraints = PexInterpreterContraints() |
There was a problem hiding this comment.
Ditto on Optional[PexInterpreterContraints] = None
| interpreter_constraint_args = [] | ||
| for constraint in request.interpreter_constraints: | ||
| interpreter_constraint_args.extend(["--interpreter-constraint", constraint]) | ||
| interpreter_constraint_args = request.interpreter_constraints.generate_pex_arg_list() |
There was a problem hiding this comment.
You can inline this to line 107 so that line 107 becomes:
argv.extend(request.interpreter_constraints.generate_pex_arg_list())| def create_pex_and_get_all_data(self, *, requirements=None, entry_point=None, interpreter_constraints=None, | ||
| def create_pex_and_get_all_data(self, *, requirements=PexRequirements(), | ||
| entry_point=None, | ||
| interpreter_constraints=PexInterpreterContraints(), |
There was a problem hiding this comment.
If you take my recommendation about interpreter constraints being optional, I would still recommend this being None
| path = 'dist/', | ||
| directory_digest = binary.digest, | ||
| ) | ||
| output = workspace.materialize_directories((dtm,)) |
There was a problem hiding this comment.
If materialize_directories takes a tuple, why not make a tuple of the directories to materialize and do them in one call in parallel, rather than in a loop?
| all_target_requirements = [] | ||
| for maybe_python_req_lib in adaptors: | ||
| # This is a python_requirement()-like target. | ||
| if hasattr(maybe_python_req_lib, 'requirement'): |
There was a problem hiding this comment.
Not for this PR, but in general RE #4535 it would be nice if this were maybe detected by superclass (and then a property access) rather than by grabbing around for raw field names...
There was a problem hiding this comment.
Absolutely, but yeah I don't want to handle that chunk of work until this PR is merged.
Problem
We need a version of the 'binary' goal that runs on the v2 engine.
Solution
This code creates a
@console_rulefor thebinarygoal, the necessary type boilerplate, implements treating a pants-created Python PEX as a binary, and writes it to thedistdirectory.