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

Jump to bottom

Update runpy for PEP 451 #63899

Closed
brettcannon opened this issue Nov 22, 2013 · 17 comments
Closed

Update runpy for PEP 451 #63899

brettcannon opened this issue Nov 22, 2013 · 17 comments
Assignees
@ncoghlan
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@brettcannon
Copy link
Member

BPO 19700
Nosy @brettcannon, @ncoghlan, @larryhastings, @ericsnowcurrently
Files
  • issue19700_runpy_spec.diff: First draft at spec based runpy
  • issue19700_runpy_spec_v2.diff: Fixed namespace package handling
  • issue19700_runpy_spec_v3.diff: Also checks specs, fails on the zipimport loader checks.
  • issue19700_runpy_spec_v4.diff: Skip checking the loader for the zipimport case
  • issue19700_runpy_spec_v5.diff: Partial docs update, broken "script without suffix" test case
  • issue19700_runpy_spec_v6.diff: Final version with docs updates and no-suffix compatibility
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ncoghlan'
closed_at = <Date 2013-12-17.12:30:37.477>
created_at = <Date 2013-11-22.16:31:23.570>
labels = ['type-feature', 'library']
title = 'Update runpy for PEP 451'
updated_at = <Date 2013-12-19.06:08:24.977>
user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2013-12-19.06:08:24.977>
actor = 'eric.snow'
assignee = 'ncoghlan'
closed = True
closed_date = <Date 2013-12-17.12:30:37.477>
closer = 'ncoghlan'
components = ['Library (Lib)']
creation = <Date 2013-11-22.16:31:23.570>
creator = 'brett.cannon'
dependencies = []
files = ['33046', '33132', '33133', '33134', '33135', '33145']
hgrepos = []
issue_num = 19700
keywords = ['patch']
message_count = 17.0
messages = ['203814', '204187', '205356', '205556', '205563', '205579', '205806', '205882', '206204', '206207', '206208', '206210', '206221', '206222', '206223', '206425', '206581']
nosy_count = 6.0
nosy_names = ['brett.cannon', 'ncoghlan', 'larry', 'Arfrever', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19700'
versions = ['Python 3.4']
    @brettcannon brettcannon added the stdlib Python modules in the Lib dir label Nov 22, 2013
    @ericsnowcurrently
    Copy link
    Member

    This is closely related to issue bpo-19697.

    @ncoghlan
    Copy link
    Contributor

    D'oh, I forgot there was a runpy API change I planned to offer as part of the PEP-451 integration: exposing a "target" parameter in both run_path and run_module.

    http://www.python.org/dev/peps/pep-0451/#the-target-parameter-of-find-spec

    I guess that slips to 3.5 now, since there's no way it will be in for feature freeze :(

    @ericsnowcurrently
    Copy link
    Member

    _run_module_as_main() is particularly related to bpo-19697.

    @ncoghlan ncoghlan self-assigned this Dec 8, 2013
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 8, 2013

    OK, I just noticed that importlib.find_spec copies the annoying-as-hell behaviour of importlib.find_loader where it doesn't work with dotted names. Can we fix that please? It's stupid that pkgutil.get_loader has to exist to workaround that design flaw for find_loader, and I'd hate to see it replicated in a new public facing API.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 8, 2013

    Deleted a bunch of code, and runpy now correctly sets both __file__ and __cached__ (runpy previously never set the latter properly).

    You can also see the reason for my rant above in the form of runpy._fixed_find_spec. importlib.find_loader was always kind of useless in end user code that needed to handle arbitrary modules, you needed to use the pkgutil.get_loader wrapper instead. It would be nice if importlib.find_spec "just worked", instead of only working for top level modules. At the very least, it should fail noisily if a dotted name is passed in without specifying a path argument.

    I may have argued in favour of the side effect free find_loader in the past, if I have, consider this me admitting I was wrong after wasting a bunch of time hunting down unexpected import errors in test_runpy after the initial conversion to using find_spec.

    There's one final change needed to address the pickle-in-main compatibility issue: runpy has to alias the module under its real name as well as __main__. Once it does that, then using __spec__.name (when available) should ensure pickle does the right thing.

    @brettcannon
    Copy link
    Member Author

    Can you file a separate bug, Nick, for the importlib.find_spec() change you are after? I don't want to lose track of it (and I get what you are after but we should discuss why importlib.find_loader() was designed the way it was initially).

    @ncoghlan
    Copy link
    Contributor

    bpo-19944 now covers moving the current importlib.find_spec to importlib.find_spec_on_path and having importlib.find_spec behave like runpy._fixed_find_spec in my patch.

    @ericsnowcurrently
    Copy link
    Member

    patch LGTM

    @ncoghlan
    Copy link
    Contributor

    I added some unit tests for the interactions between runpy and namespace packages, which showed that I was doing the check for __main__ submodules and the check for "no loader" in the wrong order.

    Last missing piece is to ensure that __spec__ is being populated appropriately, then I'll check this in.

    @ncoghlan
    Copy link
    Contributor

    So close!

    importlib.util.spec_from_file_location failed me (unsurprisingly) when it came to the zipfile execution tests. I'm thinking I'll just hack in a way to avoid checking the loader when the expected loader is set to None.

    @ncoghlan
    Copy link
    Contributor

    Latest version simply doesn't check the loader type for the zipimport tests. (Note that just using find_spec doesn't work, since the directory and zipfile execution tests include implicit sys.path manipulation)

    I also removed the separated "precompiled" flag that was in earlier patches, since importlib.util.spec_from_file_location deals with that for us.

    If the full regression test suite passes, I'll be checking this version in.

    @ncoghlan
    Copy link
    Contributor

    Working on the docs updates made me realise the test cases didn't cover the "no suffix" case that is causing grief in bpo-19946.

    So I've added a test case for that now, but haven't fixed it yet (will need to deal with the __spec__ = None case for such scripts)

    @ncoghlan
    Copy link
    Contributor

    Final patch that reflects the version I'm about to commit. It includes appropriate docs updates, and ensures __main__.__spec__ is None in the cases where the import system isn't involved in initialising main.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 15, 2013

    New changeset 51dddfead80a by Nick Coghlan in branch 'default':
    Issue bpo-19700: set __spec__ appropriately in runpy
    http://hg.python.org/cpython/rev/51dddfead80a

    @ncoghlan
    Copy link
    Contributor

    Final review of the patch showed it *wasn't* quite done, it's still missing the "both __name__ and __spec__.name are configured in sys.modules" change that is needed to get more pickle friendly behaviour from __main__.

    However, I wanted to commit this version to help unblock bpo-19946.

    @ncoghlan
    Copy link
    Contributor

    On second thoughts, I'm going to close this one - if further runpy changes are needed to resolve bpo-19702 (using __spec__.name for pickle when appropriate), let's deal with them there.

    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label Dec 17, 2013
    @ericsnowcurrently
    Copy link
    Member

    Thanks for working this out, Nick!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @ncoghlan ncoghlan

    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @brettcannon @ncoghlan @ericsnowcurrently