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 pkg resource early import #750

Merged
merged 6 commits into from
Aug 7, 2019

Conversation

jhamet93
Copy link
Contributor

As referenced in #749 , I ran into a situation where pkg_resources was imported too early before sys.path was finalized, occurring in the situation where a pex in my PEX_PATH needed to invoke: pkg_resources.get_distribution. This was failing because a namespace package caused pkg_resources to be imported before the other pexes were merged into the environment

Therefore, wait until all deps from all pexes (i.e. pexes in PEX_PATH) have been resolved before attempting to declare the namespace packages. This ensures sys.path is finalized before pkg_resources is possibly imported.

@jsirois
Copy link
Member

jsirois commented Aug 5, 2019

The CI error is reproducible locally. I'm taking a look...

Remove a duplicate and buggy use of `pex_path` in `Pex.patch_sys`,
fixup tests and "privatize" `PEXEnvironment` methods as appropriate for
clarity.
@jsirois
Copy link
Member

jsirois commented Aug 6, 2019

@jhamet93 this one was not trivial to explain. I took the liberty of pushing an addtional commit in ef0a13e with the fix for CI + cleanups.

+ Ensure pexes are added to the sys.path exactly once.
+ Surface explicit handling for the empty ns package list behavior seen
  in the wild - only consider non-empty lists.
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.

I'm good with this as it now stands. Since I added changes significant enough to fix failing tests, this should really get one more review before merging.

@jsirois jsirois requested a review from stuhood August 6, 2019 14:59
@jsirois
Copy link
Member

jsirois commented Aug 6, 2019

@jhamet93 could you please review the changes I made on top of yours? They are these: https://github.com/pantsbuild/pex/pull/750/files/62af44be964f6f916810a3ac4622241da1b34ff0..0489225aaa65cd41250b708e9210dd47a7e20695

Any comments would be great and if all looks good a LGTM would be good.

Thanks for your contribution!

@jhamet93
Copy link
Contributor Author

jhamet93 commented Aug 7, 2019

LGTM.

Thanks for the help finishing this one!

@jsirois jsirois merged commit 5ccaba7 into pex-tool:master Aug 7, 2019
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