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 third_party.exposed(). #2330

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Jan 16, 2024

This cleans up #2328 by fixing the root cause of the interpreter
identification leak of exposed attrs. The API to report exposed
sys.path elements, third_party.exposed(), worked at build time,
with a full Pex distribution available, but returned an empty iterator
at run time, with the subset of Pex available in the runtime
.bootstrap/.

This cleans up pex-tool#2328 by fixing the root cause of the interpreter
identification leak of exposed `attrs`. The API to report exposed
`sys.path` elements, `third_party.exposed()`, worked at build time,
with a full Pex distribution available, but returned an empty iterator
at run time, with the subset of Pex available in the runtime
`.bootstrap/`.
@jsirois jsirois requested review from zmanji, benjyw and huonw January 16, 2024 18:01
return False

sys_path = OrderedSet(entry for entry in sys.path if entry and not is_internal_entry(entry))
sys_path = OrderedSet(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the simpler check that existed pre-#2328. It was always the right check; it's just that third_party.exposed() was broken at PEX runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

And all is well here. Performing this change 1st led to a failing test_boot_identification_leak test, and then fixing below got that passing again.

for importer in cls._iter_all_installed_vendor_importers():
# All Importables for a given VendorImporter will have the same prefix.
if importer._importables and importer._importables[0].prefix == prefix:
if importer._root == root:
if {importable.path for importable in importer._importables} == vendored_paths:
yield importer
yield importer
Copy link
Member Author

Choose a reason for hiding this comment

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

This comparison of the vendored paths at runtime (from the PEX .bootstrap/) would always fail since Pex only vendors a subset of all available vendored distributions in the runtime bootstrap of a PEX. The vendored Pip, for one, is never vendored in the runtime; it is only used at build time.

I'm not sure why I added this check in the 1st place back in #624 when I introduced vendoring / the pex.third_party import hook. Checking the root and prefix is enough to ensure the discovered VendorImporter is the one in question.

@jsirois
Copy link
Member Author

jsirois commented Jan 16, 2024

The setuptools build backend introduced to handle pre-testing Python 3.13 in CI is quite flaky. Yet another instance: https://github.com/pantsbuild/pex/actions/runs/7545643289/job/20541741101?pr=2330#step:5:1847

I'm working on switching the build system in a separate PR to handle dynamic Requires-Python metadata without using setuptools to solve the files it leaks to the source root during the build process problems.

Copy link
Collaborator

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Nice!

@jsirois jsirois merged commit 58c6e8e into pex-tool:main Jan 16, 2024
26 checks passed
@jsirois jsirois deleted the pull/2328/follow-up-cleanup branch January 16, 2024 23:26
@jsirois
Copy link
Member Author

jsirois commented Jan 17, 2024

The setuptools build backend introduced to handle pre-testing Python 3.13 in CI is quite flaky...

Fix in #2331

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