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

Only python_binary's constraint should be included in a built pex #7776

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

commented May 21, 2019

Problem

See the problem described in #7775.

Solution

Compute and validate the transitive constraints, but only include the python_binary's constraint in the built pex.

Result

Fixes #7775, but leaves a TODO about supporting building a binary for an interpreter for which we do not have a valid interpreter.

@illicitonion
Copy link
Contributor

left a comment

Thanks!

@illicitonion illicitonion merged commit ebf5716 into pantsbuild:master May 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:stuhood/validate-but-do-not-include-transitive-constraints branch May 21, 2019

illicitonion added a commit that referenced this pull request May 21, 2019

Only python_binary's constraint should be included in a built pex (#7776
)

### Problem

See the problem described in #7775.

### Solution

Compute and validate the transitive constraints, but only include the `python_binary`'s constraint in the built pex.

### Result

Fixes #7775, but leaves a TODO about supporting building a binary for an interpreter for which we do not have a valid interpreter.
@jsirois
Copy link
Member

left a comment

No harm done, but the only production change needed was the one pex_builder.add_interpreter_constraints_from([binary_tgt]) line.

@@ -142,7 +156,8 @@ def _create_binary(self, binary_tgt, results_dir):

# Add interpreter compatibility constraints to pex info. This will first check the targets for any
# constraints, and if they do not have any will resort to the global constraints.
pex_builder.add_interpreter_constraints_from(constraint_tgts)
self._validate_interpreter_constraints(constraint_tgts)

This comment has been minimized.

Copy link
@jsirois

jsirois May 21, 2019

Member

This is redundant. interpreter = self.context.products.get_data(PythonInterpreter) above already was produced by a task that checked an even more stringent requirement - namely that all targets in the context with constraints have a local satisfying interpreter - the one we're building the pex with.

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jun 3, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jun 3, 2019

Eric-Arellano added a commit that referenced this pull request Jun 4, 2019

Cleanup unnecessary code from #7776 for `./pants binary` interpreter …
…constraints (#7842)

As pointed out in #7776 (comment), #7776 had an unnecessary use of `InterpreterCache` to re-validate the constraints, even though that was already done by upstream tasks.

This results in less code and slightly faster performance by avoiding running the validation check twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.