bpo-32691: Use mod_spec.parent when running modules with pdb

[mariocj89]

At the moment the module name is used, which breaks relative imports when pdb is run against a plain module or submodule.

Fixes running python3 -m pdb -m plain.module when plain_module has a relative import

I've changed the relative import test to use the plain module to cover it. If prefered I can split into two test cases (just felt there are too many test cases for the run as module functionality).

Reported by @jaraco

https://bugs.python.org/issue32691

[ncoghlan]

We do need an additional test case to ensure that the executable package case is still handled correctly, but it would be reasonable to make that a runpy level unit test (of runpy._get_module_details behaviour), rather than a pdb level test.

(Note: I'm going to be away for a few days, so I'll review any PR updates after I get back next week)

[ncoghlan]

Huh, I initially thought this was wrong, but I'd forgotten that it's runpy._get_module_details that handles the recursive search for pkg.__main__, so mod_spec.parent == module_name in those cases.

[ncoghlan]

I think we do want to explicitly test both paths through runpy._get_module_details here, as otherwise we're exposed to an internal refactoring in runpy breaking the above assumption about how executable packages are handled.

Alternatively, you could add some unit tests for runpy._get_module_details directly to Lib/test/test_runpy.py that ensure that this behaviour is retained (that approach would also be a good foundation for potentially converting this to a public API in 3.8 that not only returns the current module details, but also the values to use to update the module namespace prior to execution)

[mariocj89]

I've duplicated the test (added one for the plain modules situation).
If you find it appropriate I can add a helper function to create those "scripts" as it is quite duplicated and change the existing code to use it.

Additionally, if you find interesting the idea of making _get_module_details public I can file an issue and draft something out, as it will be useful for the other PRs on running as module

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[mariocj89]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.

[miss-islington]

Thanks @mariocj89 for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[bedevere-bot]

GH-5510 is a backport of this pull request to the 3.7 branch.

[ncoghlan]

Thanks!

I do think we should make a module-details-like API available, probably in combination with another helper API to initialise a namespace based on a spec, so if you were willing to work on that, that would be great.

[mariocj89]

@ncoghlan I've totally left this by. Apologies about that.

I was having a look at it and I am not fully sure about what you'd like to expose or what you exactly mean with:

Expose a module-details-like API

What were you thinking on? The code in _get_module_details seems to be just importlib.util.find_spec and spec.loader.get_code(spec.name) plus handling the quirks of preparing it to be run and looking at __main__ recursively if present.
What were you thinking to expose and do you think it would be useful outside of the standard library? I can expose _get_module_details but I am not sure how would you name it :/ and if we remove the specifics of the runpy module and getting ready to run, there is no much "code" there.
That said, if you find value and can describe what it should do I can work on it.

API to initilise a namespace basde on a spec

Sure! Do you mean something that just does:

namespace = { '__name__': mod_spec.name, # This should then be replaced to just '__main__' to run it, right? '__file__': mod_spec.origin, # This won't work for things like builtins or any other *non-locatable* '__package__': mod_spec.parent, '__loader__': mod_spec.loader, '__spec__': mod_spec, '__cached__': None, } 

Where would you like such function to live and how would you name it?

If you want any of these two I'll create the appropiate issues. Thanks and sorry to bother with quesitons but I dont have fully clear what you expected :/

[ncoghlan]

@mariocj89 I'm not sure myself - the import-sig mailing list would be the best place to kick some ideas around before we turn them into enhancement proposals on bugs.python.org.

[mariocj89]

Yeah... I'm not sure I agree with my own comment here hehe. After having looked through it I am not sure it adds any value to the user to expose that method given that people should be just using importlib.util.find_spec. If users need to retrieve something to run it, which is quite a specific requirement, I think it is ok having to write some code for it.

The only issue I see is depending in the internal API of runpy but I'd personally keep it like that.

[jaraco]

Better late than never -- thank you @mariocj89 for the patch!