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

Sharpen Platform / PythonIdentity relationship #579

Closed
jsirois opened this issue Oct 8, 2018 · 1 comment
Closed

Sharpen Platform / PythonIdentity relationship #579

jsirois opened this issue Oct 8, 2018 · 1 comment
Assignees
@jsirois
Labels
enhancement

Comments

@jsirois
Copy link
Member

jsirois commented Oct 8, 2018

Currently these two classes contain an overlap of platform information as highlighted by Resolver._maybe_expand_platform:
https://github.com/pantsbuild/pex/blob/d4a3c9db271a336b1a734a950eee453ee02f787f/pex/resolver.py#L167-L213

In particular:

Platform PythonIdentity
impl abbr_impl
version impl_ver
abi abi_tag

This leads to some awkwardness and, as a result, subtle bugs. For example - in the same file as _maybe_expand_platform, there is a bug here:
https://github.com/pantsbuild/pex/blob/d4a3c9db271a336b1a734a950eee453ee02f787f/pex/resolver.py#L560

Passing Platform.current() masquerades as the user specifying extended platform information in _maybe_expand_platform, preventing the selected interpreter from being paired with its proper Platform. The fix to pass 'current' instead is simple but a sign of the current fragility.
jsirois added a commit to jsirois/pex that referenced this issue Oct 8, 2018
@jsirois
Fix resolve regressions introduced by the 1.4.8.
e6dc6d0 
PR pex-tool#571 regressed the half-broken state of having
`--interpreter_constraint` selected interpreters not setup to also
having `--python` selected interpreters also not setup. In addition,
PR pex-tool#568 incorrectly classified the current Platform passed by
`resolve_multi` as a user-specified extended platform specification
breaking custom interpreter resolution. Fix both and add tests that
failed prior to this combination of fixes.

A more comprehensive fix is tracked in part by pex-tool#579.
@jsirois jsirois mentioned this issue Oct 8, 2018
Merged
jsirois added a commit that referenced this issue Oct 8, 2018
@jsirois
Fix resolve regressions introduced by the 1.4.8. (#580)
b9229d7 
PR #571 regressed the half-broken state of having
`--interpreter_constraint` selected interpreters not setup to also
having `--python` selected interpreters also not setup. In addition,
PR #568 incorrectly classified the current Platform passed by
`resolve_multi` as a user-specified extended platform specification
breaking custom interpreter resolution. Fix both and add tests that
failed prior to this combination of fixes.

A more comprehensive fix is tracked in part by #579.
Laevos pushed a commit to Laevos/pex that referenced this issue Oct 9, 2018
# This is a combination of 8 commits.
e338eb3 
# The first commit's message is:
pex-tool#572: Allow import of ctypes to be skipped if use_manylinux is false

# This is the 2nd commit message:

Narrow the env marker test. (pex-tool#578)

The jupyter dist is just a meta-dist with fully unconstrained deps on ~6
other dists. This test was added to test environment marker support in
pex, which ipython - not jupyter - leverages heavily.

# This is the 3rd commit message:

Fix resolve regressions introduced by the 1.4.8. (pex-tool#580)

PR pex-tool#571 regressed the half-broken state of having
`--interpreter_constraint` selected interpreters not setup to also
having `--python` selected interpreters also not setup. In addition,
PR pex-tool#568 incorrectly classified the current Platform passed by
`resolve_multi` as a user-specified extended platform specification
breaking custom interpreter resolution. Fix both and add tests that
failed prior to this combination of fixes.

A more comprehensive fix is tracked in part by pex-tool#579.

# This is the 4th commit message:

Cleanup `PexInfo` and `PythonInterpreter`. (pex-tool#581)

Kill an unused type in `PexInfo` as well as our last remaining use of
`pkg_resources.get_platform`. Also kill unused `COMPATIBLE_SETUPTOOLS`
constants in `PythonInterpreter`.

# This is the 5th commit message:

Support environment markers during pex activation. (pex-tool#582)

We've had support for environment markers on the resolve side for a
while and with just a little plumbing we can now support multi-python
pexes with environment-specific requirements.

Fixes pex-tool#456

# This is the 6th commit message:

Revert "Support environment markers during pex activation. (pex-tool#582)"

This reverts commit 5f1f00f.

We want to do a 1.4.9 bugfix release before this ~API change.

# This is the 7th commit message:

Prepare the 1.4.9 release. (pex-tool#588)

Work towards pex-tool#583

# This is the 8th commit message:

Revert "Revert "Support environment markers during pex activation. (pex-tool#582)""

This reverts commit 44ff463.

This restores pex-tool#582 for the 1.5.0 release tracked by pex-tool#585.
@jsirois jsirois self-assigned this Aug 23, 2024
@jsirois
Copy link
Member Author

jsirois commented Aug 23, 2024

With the introduction of Target and its LocalInterpreter, AbbreviatedPlatform and CompletePlatform implementations, the relationship between PythonIdentity (LocalInterpreter) and Platform (AbbreviatedPlatform) is now clear in the code base.

@jsirois jsirois closed this as completed Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jsirois jsirois

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@jsirois