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

Fix interpreter resolution when using --complete-platform with --resolve-local-platforms #2031

Merged
merged 4 commits into from Jan 10, 2023

Conversation

shalabhc
Copy link
Contributor

@shalabhc shalabhc commented Jan 9, 2023

Fixes #2026.

Previously the interpreter resolution was more strict than necessary. The supported_tags were only checked if the most specific tag of the complete platform matched the platform of the local interpreter. This prevented resolution in cases when it was possible and safe (ie all supported_tags of the interpreter as present in the supported_tags of the complete-platform).

Added an additional test for the above case. Fixed existing code and also improved various existing tests by adding expected_complete_platforms.

…l-platforms

Added an extra test for interpreter resolution when the interpreter supported_tags are a subset of the requested complete-platform's supported_tags. Tweaked the code to ensure this test passes.
Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

Thanks @shalabhc - this looks good. The other shard errors look like network flakes; just the fmt shard error is legit. I'll get this merged and a release out with the fix once you get it green.

"Searching for local interpreters matching {}".format(
", ".join(map(str, all_platforms))
)
"Searching for local interpreters matching ".format(", ".join(platform_strs))
Copy link
Member

Choose a reason for hiding this comment

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

The fmt shard error caught this - missing {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pinpointing @jsirois - fix committed - waiting for tests.

@jsirois jsirois mentioned this pull request Jan 9, 2023
2 tasks
@shalabhc shalabhc requested a review from jsirois January 9, 2023 22:07
@jsirois jsirois merged commit 2ed43a5 into pex-tool:main Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants