Repair unhandled AttributeError during pex bootstrapping. #599
Conversation
| try: | ||
| module.__path__.insert(0, module_dir) | ||
| except AttributeError: | ||
| TRACER.log( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This looks like it will take a few hours to fix the underlying issue with a test coverging the implicit namespace package case.
There was a problem hiding this comment.
sgtm - thank you! added a concrete minimal repro there.
|
@jsirois yes, this addresses an issue showing up internally that we haven't been able to nail down yet. I was going to handle the internal release part, and maybe the pants part, whatever makes sense. |
|
@jsirois propped this up purely as a quick drive-by bandaid to help expedite internal unblocking (and ticketed/discussed internally) - but no strong preference on course of action here. |
|
I dropped the ball on making sure this made it into a pex release, and this is an active blocker for an internal use case -- are there any blockers to merging this as of now? @kwlzn @jsirois I would be willing to do whatever is necessary to make the pex release work, or I could just handle the pants part. |
### Problem See pex-tool/pex#623 for the pex release ticket. This release incorporates two changes, one of which (pex-tool/pex#599) was necessary to fix the issue in pex-tool/pex#598. ### Solution Bump the pex version to `1.5.3`.
See pex-tool/pex#623 for the pex release ticket. This release incorporates two changes, one of which (pex-tool/pex#599) was necessary to fix the issue in pex-tool/pex#598. Bump the pex version to `1.5.3`.
A quick, surface level fix for #598.