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

Cleanup PexInfo and PythonInterpreter. #581

Merged
merged 1 commit into from
Oct 8, 2018

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Oct 8, 2018

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.

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`.
@@ -53,13 +50,15 @@ class PexInfo(object):
@classmethod
def make_build_properties(cls, interpreter=None):
from .interpreter import PythonInterpreter
from pkg_resources import get_platform
from .platforms import Platform
Copy link
Contributor

Choose a reason for hiding this comment

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

This relative import initially made me uncomfortable because I know they can be problematic for things like refactoring, and the Py2 syntax breaks on Py3. But I reviewed the book Python Cookbook and they say it's totally valid + you're copying the precedent, so I think it's fine.

You're also using the Py3 syntax, which resolves the biggest concern I would have.


pi = interpreter or PythonInterpreter.get()
plat = Platform.current()
platform_name = plat.platform
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this variable name. Definitely makes things more clear than platform: plat.platform

@jsirois jsirois merged commit 55e6482 into pex-tool:master Oct 8, 2018
@jsirois jsirois deleted the unuseds/kill branch October 8, 2018 17:09
Laevos pushed a commit to Laevos/pex that referenced this pull request Oct 9, 2018
# 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants