Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Vendor untangle and pydevd #99

Merged
merged 5 commits into from
Feb 21, 2018
Merged

Vendor untangle and pydevd #99

merged 5 commits into from
Feb 21, 2018

Conversation

int19h
Copy link
Contributor

@int19h int19h commented Feb 21, 2018

You'll probably want to look at just the first two individual commits here, so that you don't have to scroll past all the pydevd code.

@stevdo, @brettcannon - I'm doing some unspeakable things here to package up pydevd as is (so that we can easily merge changes from upstream, and later push them back). It seems to work fine, but please take a look, in case there's some corner case here that I'm missing.

I think that adding a subpath to sys.path should work reliably even with eggs etc, but I wonder if there's any case where just mucking around with paths won't work, and I need an import hook and pkg_resources instead.

@zooba
Copy link
Member

zooba commented Feb 21, 2018

That all looks fine. There shouldn't be a need to use any import hooks, but it might be worthwhile importing all the top-level packages from the pydevd folder eagerly and then removing the sys.path entry. (Submodule imports don't rely on sys.path)

I'd also make that assert 'pydevd' not in sys.modules check a proper message explaining what is wrong. Inevitably someone is going to hit it, and the more useful info we can provide the better.

@zooba
Copy link
Member

zooba commented Feb 21, 2018

Also, can you email me details of where untangle came from so I can register it in our OSS system?

@brettcannon
Copy link
Member

Maybe I missed the discussion, but why do we need to vendor pydevd?

Improve error reporting when pydevd is imported before ptvsd.
@int19h
Copy link
Contributor Author

int19h commented Feb 21, 2018

I cleaned up pydevd import as suggested, so that we don't leave anything in sys.path.

@int19h int19h merged commit a050961 into microsoft:master Feb 21, 2018
@int19h int19h mentioned this pull request Feb 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants