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.

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

'os.path' should not be a frozen module #89435

Open
zooba opened this issue Sep 23, 2021 · 5 comments
Open

'os.path' should not be a frozen module #89435

zooba opened this issue Sep 23, 2021 · 5 comments
Assignees
Labels
3.11 only security fixes build The build process and cross-build type-bug An unexpected behavior, bug, or error

Comments

@zooba
Copy link
Member

zooba commented Sep 23, 2021

BPO 45272
Nosy @gvanrossum, @warsaw, @brettcannon, @jaraco, @ncoghlan, @ericsnowcurrently, @zooba, @corona10, @FFY00
PRs
  • bpo-45272: os.path should not be a frozen module #29329
  • bpo-45850: Implement deep-freeze on Windows #29648
  • bpo-45850: Implement deep-freeze on Windows #29648
  • bpo-45850: Implement deep-freeze on Windows #29648
  • 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/ericsnowcurrently'
    closed_at = None
    created_at = <Date 2021-09-23.16:24:55.449>
    labels = ['type-bug', '3.11']
    title = "'os.path' should not be a frozen module"
    updated_at = <Date 2021-11-20.00:28:14.819>
    user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2021-11-20.00:28:14.819>
    actor = 'gvanrossum'
    assignee = 'eric.snow'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2021-09-23.16:24:55.449>
    creator = 'steve.dower'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45272
    keywords = ['patch']
    message_count = 5.0
    messages = ['402506', '402516', '403327', '403332', '403334']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'barry', 'brett.cannon', 'jaraco', 'ncoghlan', 'eric.snow', 'steve.dower', 'corona10', 'FFY00']
    pr_nums = ['29329', '29648', '29648', '29648']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue45272'
    versions = ['Python 3.11']

    Linked PRs

    @zooba
    Copy link
    Member Author

    zooba commented Sep 23, 2021

    I noticed that Python/frozen.c includes posixpath as 'os.path'.

    This is not correct, and shouldn't be necessary anyway, because os.path is just an attribute in "os" and not a concrete module (see Lib/os.py#L95 for the bit that makes it importable, and Lib/os.py#L61 and Lib/os.py#L81 for the imports).

    @zooba zooba added the 3.11 only security fixes label Sep 23, 2021
    @zooba zooba added type-bug An unexpected behavior, bug, or error 3.11 only security fixes labels Sep 23, 2021
    @zooba zooba added the type-bug An unexpected behavior, bug, or error label Sep 23, 2021
    @ericsnowcurrently
    Copy link
    Member

    The matter here boils down to the design of _imp.is_frozen() [1]. It only checks to see if the given module name shows up in the list of frozen modules in Python/frozen.c. This broke things when I froze os and posixpath/ntpath.

    The simplest solution was to include os.path in the list of modules in frozen.c. The better solution would be to have _imp.is_frozen() check the module in sys.modules.

    [1] see find_frozen() in Python/import.c

    @gvanrossum
    Copy link
    Member

    I'm trying to understand the proposed solution, "have _imp.is_frozen() check the module in sys.modules." Does that mean it would do a dict lookup first? Maybe you should do that in the caller instead? importlib/_bootstrap.py calls it a few times, but I'm not convinced that all call sites can be called with "os.path" -- do you know which path was taken when this failed?

    I worry about the complexity of the importlib bootstrapping mechanism. Grepping through the source for _bootstrap and _bootstrap_external has not given me any understanding of how this works. Do you know if there are docs for this? Or do we just need to ask Brett?

    @ericsnowcurrently
    Copy link
    Member

    On Wed, Oct 6, 2021 at 11:38 AM Guido van Rossum <[email protected]> wrote:

    I'm trying to understand the proposed solution, "have _imp.is_frozen() check the module in sys.modules." Does that mean it would do a dict lookup first?

    Correct. We'd look up the module in sys.modules and, if there, check its loader.

    Maybe you should do that in the caller instead? importlib/_bootstrap.py calls it a few times, but I'm not convinced that all call sites can be called with "os.path" -- do you know which path was taken when this failed?

    Good point. The only place where it matters is the FrozenImporter methods that are wrapped with _requires_frozen(). So the fix can go there instead of _imp.is_frozen().

    I worry about the complexity of the importlib bootstrapping mechanism. Grepping through the source for _bootstrap and _bootstrap_external has not given me any understanding of how this works. Do you know if there are docs for this? Or do we just need to ask Brett?

    Are you talking about the use of _imp.is_frozen() or do you mean the code in _bootstrap.py (and _bootstrap_external.py) as a whole? I don't believe there's any official documentation about the implementation in _bootstrap.py. At best there have been some PyCon talks about how the import system works (but probably not at the level of the implementation) and Brett may have a blog post or two. Keep in mind that I'm quite familiar with the importlib code, though Brett is definitely the mastermind behind the overall implementation.

    @gvanrossum
    Copy link
    Member

    It was a comment about my general lack of understanding of how the importlib bootstrap process works. I should probably start reading the docstrings before complaining more. :-)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel iritkatriel added the build The build process and cross-build label Dec 1, 2023
    FFY00 added a commit to FFY00/cpython that referenced this issue Nov 17, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes build The build process and cross-build type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants