The Wayback Machine - https://web.archive.org/web/20211212124318/https://github.com/RustPython/RustPython/pull/3488
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

import_helper: refactor frozen_modules #3488

Merged
merged 1 commit into from Dec 9, 2021

Conversation

@deantvv
Copy link
Contributor

@deantvv deantvv commented Dec 1, 2021

frozen_modules requires non-trivial changes in import (imp.rs)
and from the doc it is deprecated since 3.3 which makes this feacture
unlikely to be support on this project.

So instead of implementing this feature, I make frozen_modules to
raise exception if frozen is required. Other than that, all implementation
is just like the upstream version (except one TODO comment to remind
us there is something not fully supported)

`frozen_modules` requires non-trivial changes in import (`imp.rs`)
and from the doc it is deprecated since 3.3 which makes this feacture
unlikely to be support on this project.
So instead of implementing this feature, I make `frozen_modules` to
raise exception if `frozen` is required. Other than that, all implementation
is just like the upstream version (except one TODO comment to remind
us there is something not fully supported)
@coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Dec 1, 2021

_imp is actually only deprecated for user code, it's still heavily used in cpython internals. I think it'd probably be good to implement it in _imp (at least at some point)

Loading

@deantvv
Copy link
Contributor Author

@deantvv deantvv commented Dec 2, 2021

@coolreader18
The part that is needed in frozen_module is _imp._override_frozen_modules_for_tests.

From grepping this function (_override_frozen_modules_for_test) in cpython repo, it is only used in frozen_modules in test/support/import_helper.py.

Instead of implementing _override_frozen_modules_for_test right now, I chosed to raise Exception if we actually use this function. And from test result, we didn't use this function at all. Following are the implementation from cpython and this PR.

cpython version

@contextlib.contextmanager
def frozen_modules(enabled=True):
    _imp._override_frozen_modules_for_tests(1 if enabled else -1)
    try:
        yield
    finally:
        _imp._override_frozen_modules_for_tests(0)

This PR

@contextlib.contextmanager
def frozen_modules(enabled=True):
    if enabled:
        raise NotImplemented("frozen_modules is not implemented on RustPython")

    yield

Compared with previous commit, this PR also doesn't change the behavior since the old commit just ignore all function called of frozen_modules. Now we add frozen_modules and check if we need _imp.override_frozen_modules_for_tests. If yes, we raise exception. If not (seems like all tests we're running now doesn't need this), it exhibit the same behavior as cpython.

Loading

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 9, 2021

it sounds reasonable to me

Loading

@coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Dec 9, 2021

Oh yes, me as well, sorry I just didn't have the time to fully review the added code when I first noticed this. LGTM

Loading

@coolreader18 coolreader18 merged commit db3b312 into RustPython:main Dec 9, 2021
10 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants