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.

Sign up for GitHub

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

Repair unhandled AttributeError during pex bootstrapping. #599

Merged
merged 2 commits into from
Nov 15, 2018
Merged

Repair unhandled AttributeError during pex bootstrapping. #599

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
306fd6c
Repair unhandled AttributeError during pex bootstrapping.
kwlzn Oct 12, 2018
25d4885
Comment.
kwlzn Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions pex/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,19 @@ def update_module_paths(cls, new_code_path):
TRACER.log('Adding to the head of sys.path: %s' % new_code_path)
sys.path.insert(0, new_code_path)
for name, module in sys.modules.items():
if hasattr(module, "__path__"):
if hasattr(module, '__path__'):
module_dir = os.path.join(new_code_path, *name.split("."))
TRACER.log('Adding to the head of %s.__path__: %s' % (module.__name__, module_dir))
module.__path__.insert(0, module_dir)
try:
module.__path__.insert(0, module_dir)
except AttributeError:
TRACER.log(
Copy link
Member

Choose a reason for hiding this comment

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

Why is it ok to log and not?:
# Force subsequent imports to come from the .pex directory rather than the .pex file.

This begs the question as to whether the enclosing update_module_paths function is needed at all.

Copy link
Contributor Author

@kwlzn kwlzn Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

the aim is purely to queue a quick bandaid in the pex->pants->internal release lifecycle to unblock our use of pex here, tbh. I think implicitly allowing breakage of the ~2/100+ affected modules w/ logging beats a hard crash preventing any use until a deeper fix can be made (and subsequently released etc).

this may or may not be reasonable, but just citing my current tack. :)

Copy link
Member

Choose a reason for hiding this comment

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

The stance is reasonable, the state the code is left in does not read as reasonable. Can you call the shot in a comment with the real fix issue pointed to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, of course.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW - I think I sussed what's going on here. Details added to #598. I may be able to get a real fix in here. I'll stare at this code in light of PEP-420 and report an estimate.

Copy link
Member

Choose a reason for hiding this comment

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

This looks like it will take a few hours to fix the underlying issue with a test coverging the implicit namespace package case.

Copy link
Contributor Author

@kwlzn kwlzn Oct 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

sgtm - thank you! added a concrete minimal repro there.

'Failed to insert %s: %s.__path__ of type %s does not support insertion!' % (
module_dir,
module.__name__,
type(module.__path__)
)
)

@classmethod
def write_zipped_internal_cache(cls, pex, pex_info):
Expand Down