Fix current platform handling. #801

jsirois · 2019-11-21T05:57:30Z

Three aspects of current platform handling are fixed:

Ensure that Platform.supported_tags returns the full range of tags
when the platform is current. This uses the special no-arg form of
pep425tags.get_supported which consults the local interpreter to
return the broadest possible set of tags. Also ensure that we always
pass all compatible versions when the platform is not current to
generate the broadest possible set of tags for the foreign platform.
Ensure that the resolver interprets 'current' to mean any local
interpreter instead of just the current runtime interpreter. This is
faithful to prior behavior where a PEX built for the 'current' platform
would always work on the current machine.
Fix the PEX cli when run with a fully foreign set of --platforms.
Previously the attempt to warn the user would error on an invalid log
call that was missing a V argument.

Tests of all of the above are added and platform parsing is pushed up to
the CLI and resolver API layers for cleaner code under those layers and
earlier failures for invalid platform specifiers.

Three aspects of current platform handling are fixed: 1. Ensure that `Platform.supported_tags` returns the full range of tags when the platform is current. This uses the special no-arg form of `pep425tags.get_supported` which consults the local interpreter to return the broadest possible set of tags. Also ensure that we always pass all compatible versions when the platform is not current to generate the broadest possible set of tags for the foreign platform. 2. Ensure that the resolver interprets 'current' to mean any local interpreter instead of just the current runtime interpreter. This is faithful to prior behavior where a PEX built for the 'current' platform would always work on the current machine. 3. Fix the PEX cli when run with a fully foreign set of `--platform`s. Previously the attempt to warn the user would error on an invalid log call that was missing a `V` argument. Tests of all of the above are added and platform parsing is pushed up to the CLI and resolver API layers for cleaner code under those layers and earlier failures for invalid platform specifiers.

jsirois · 2019-11-21T23:30:37Z

N.B.: These issues were discovered trying to upgrade Pants to Pex 2.0.0.

Eric-Arellano · 2019-11-22T16:20:59Z

tests/test_resolver.py

+  assert 1 == len(resolved_current)
+  assert resolved_other != resolved_current
+  assert resolved_current == resolve_current(interpreters=[current_python])
+  assert resolved_current == resolve_current(interpreters=[current_python, current_python])


I don't understand this line. When would you ever have two instances of the same interpreter?

You wouldn't, but primarily this is test of de-duping and at the unit level, interpreters are handed in as a list, so an API user could hand in dups. More towards reality, given an interpreter constraint (that whole concept has been broken from the get-go really), you might collect a python 2.7.15 - say from pyenv, and a 2.7.17, say from the system. Since wheels / platform specifiers resolve only down to major/minor, these two different interpreters would produce the same wheels in which case we'd want to dedup.

Eric-Arellano · 2019-11-22T16:21:36Z

tests/test_resolver.py

+  # Here we have 2 local interpreters satisfying current but with different platforms and thus
+  # different dists for 2 total dists.


Thank you for the explanation!

Eric-Arellano · 2019-11-22T16:22:54Z

tests/test_resolver.py

+@pytest.mark.skipif(PY_VER < (3, 5) or IS_PYPY,
+                    reason="The p537 distribution only builds for CPython 3.5+")
+def test_resolve_current_and_foreign_platforms(p537_resolve_cache):
+  foreign_platform = 'macosx-10.13-x86_64-cp-37-m' if IS_LINUX else 'manylinux1_x86_64-cp-37-m'


Check on my understanding: the cp-37-m piece is irrelevant here, even if running the test with a CP37 interpreter, correct? The reason for that is that the OS difference is enough for them to be different.

It's relevant in that we need the test to work and cp-37-m is one of the pre-built wheels available. We need to have pre-built wheels available since wheel building is turned off & impossible when platforms are foreign: https://pypi.org/project/p537/#files

Eric-Arellano · 2019-11-22T16:25:15Z

tests/test_platform.py

+  assert_tags(
+    'linux-x86_64-cp-37-m',
+    [
+      ('cp37', 'cp37m', 'linux_x86_64'),


Should cp38 be added now that it's officially released?

Perhaps, but then the platform would need to change. The algorithm / correct answer is a countdown from 3.minor to 3.2 for abi3 wheels. Counting up to some arbitrary future minor ceiling is not a thing.

I'll defer 3.8 support to another PR, there is more that needs to be done in CI and tox.

Eric-Arellano · 2019-11-22T16:28:00Z

tests/test_pex_binary.py

+def test_build_pex():
+  with temporary_dir() as sandbox:
+    pex_path = os.path.join(sandbox, 'pex')
+    results = run_pex_command(['ansicolors==1.1.8', '--output-file', pex_path])


Huh...all this time I thought 1.0.2 was the latest. Going to see if this newer version (still 2 years old) fixes the regex DeprecationWarning Pants CI is filled with.

Eric-Arellano · 2019-11-22T16:32:11Z

pex/platforms.py

+        Not a valid platform specifier: {}
+
+        Platform strings must be in one of two forms:
+        1. Canonical: <platform>-<python impl abbr>-<python version>-<abi>
+        2. Abbreviated: <platform>-<python impl abbr>-<python version>-<abbr abi>
+
+        Given a canonical platform string for CPython 3.7.5 running on 64 bit linux of:
+          linux-x86_64-cp-37-cp37m
+
+        Where the fields above are:
+        + <platform>: linux-x86_64 
+        + <python impl abbr>: cp
+        + <python version>: 37
+        + <abi>: cp37m
+
+        The abbreviated platform string is:
+          linux-x86_64-cp-37-m
+
+        These fields stem from wheel name conventions as outlined in
+        https://www.python.org/dev/peps/pep-0427#file-name-convention and influenced by
+        https://www.python.org/dev/peps/pep-0425.


Fantastic explanation. Thank you.

jsirois force-pushed the resolver/fix_current_platform_handling branch from b59ade8 to 1629be8 Compare November 21, 2019 20:45

jsirois force-pushed the resolver/fix_current_platform_handling branch from 1629be8 to a97468d Compare November 21, 2019 23:24

jsirois requested review from Eric-Arellano, benjyw and kwlzn November 21, 2019 23:25

jsirois changed the title ~~WIP: Fix platform 'current' handling.~~ Fix current platform handling. Nov 21, 2019

jsirois mentioned this pull request Nov 21, 2019

Release 2.0.2 #800

Closed

3 tasks

benjyw approved these changes Nov 22, 2019

View reviewed changes

Eric-Arellano approved these changes Nov 22, 2019

View reviewed changes

jsirois merged commit 5fe0bc0 into pex-tool:master Nov 22, 2019

jsirois deleted the resolver/fix_current_platform_handling branch November 22, 2019 18:42

jsirois mentioned this pull request Nov 22, 2019

Fix tests/test_integration.py. #808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix current platform handling. #801

Fix current platform handling. #801

jsirois commented Nov 21, 2019 •

edited

jsirois commented Nov 21, 2019

Eric-Arellano Nov 22, 2019

jsirois Nov 22, 2019

Eric-Arellano Nov 22, 2019

Eric-Arellano Nov 22, 2019

jsirois Nov 22, 2019

Eric-Arellano Nov 22, 2019

jsirois Nov 22, 2019 •

edited

Eric-Arellano Nov 22, 2019

Eric-Arellano Nov 22, 2019

		# Here we have 2 local interpreters satisfying current but with different platforms and thus
		# different dists for 2 total dists.

Fix current platform handling. #801

Fix current platform handling. #801

Conversation

jsirois commented Nov 21, 2019 • edited

jsirois commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois Nov 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois commented Nov 21, 2019 •

edited

jsirois Nov 22, 2019 •

edited