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

Fix issue in inferred caller when resourcse package has no source. #314

Merged
merged 7 commits into from
Aug 17, 2024

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Aug 17, 2024

Closes python/cpython#123085

  • gh-121735: Fix inferring caller when resolving importlib.resources.files()
  • Adapt changes for new fixtures.
  • Extract a function for computing 'this filename' once.
  • Just do a simple textual substitution to avoid under matching when the file is compiled.

@jaraco
Copy link
Member Author

jaraco commented Aug 17, 2024

@Gatsik I've cherry-picked your commit here and adapted it for the recent changes and to work with importlib_resources, since I can iterate much faster in this codebase. I've added some additional commits, one which explored extracting a function for resolving the filename, and the last one which explores just doing a textual substitution on __file__. This last option seems the simplest and does allow the tests to pass. What's your opinion?

@Gatsik
Copy link
Contributor

Gatsik commented Aug 17, 2024

@jaraco Great! Thanks for including me as an author of one of the commits!

My concern is that compiled python files 'freeze' their source filename, and this 'frozen' filename is referenced in frame, but not in __file__. Thus, if 'source' and 'compiled' directories of importlib differ, then simple string replacement won't work

Reproducer, which fails with textual substitution:

import importlib
import os
import pathlib
import py_compile
import shutil
import sys
import tempfile
import textwrap


def compile(tempdir):
    orig_importlib = pathlib.Path(importlib.__file__).parent
    source_dir = pathlib.Path(tempdir) / "source_importlib"
    target_dir = pathlib.Path(tempdir) / 'cimportlib'

    shutil.copytree(orig_importlib, source_dir, ignore=lambda *_: ['__pycache__'])

    for dirpath, _, filenames in os.walk(source_dir):
        for filename in filenames:
            source_path = pathlib.Path(dirpath) / filename
            cfilename = source_path.with_suffix('.pyc').name
            cfile = target_dir / pathlib.Path(dirpath).name / cfilename
            py_compile.compile(source_path, cfile)


def create_package(tempdir):
    package_dir = pathlib.Path(tempdir) / 'somepkg'
    package_dir.mkdir()
    contents = {
        "__init__.py": textwrap.dedent(
            """
            import cimportlib.resources as res
            val = res.files().joinpath('resource.txt').read_text(encoding='utf-8')
            """
        ),
        "resource.txt": "data",
    }

    for file, content in contents.items():
        path = pathlib.Path(package_dir) / file
        path.write_text(content)


def main():
    with tempfile.TemporaryDirectory() as tempdir:
        compile(tempdir)
        create_package(tempdir)
        sys.path.insert(0, str(tempdir))
        print(importlib.import_module('somepkg').val)


if __name__ == "__main__":
    raise SystemExit(main())

@jaraco jaraco force-pushed the gh-123085/inferred-compiled branch from 792da5a to 4ea81bf Compare August 17, 2024 11:40
@jaraco
Copy link
Member Author

jaraco commented Aug 17, 2024

Thanks for clarifying. That makes sense. I've pushed that last commit to refs/archive/314-text-sub and force-pushed the previous commit (retaining the frame-based check).

@jaraco
Copy link
Member Author

jaraco commented Aug 17, 2024

The next thing I need to determine - is this a bugfix or a new feature. On one hand, it's the correction of a behavior that was presumed to work broadly but didn't. On the other hand, it's expanding the scope of tested/supported scenarios. I see you've tagged the downstream issue as "type-bug". Would you like to advocate for it being a bugfix and not a feature? What use cases would be affected if this behavior were only available here and in Python 3.14+?

@jaraco jaraco force-pushed the gh-123085/inferred-compiled branch from 5c3e385 to d618902 Compare August 17, 2024 12:06
@jaraco jaraco merged commit 0ecbc3b into main Aug 17, 2024
24 checks passed
@jaraco jaraco deleted the gh-123085/inferred-compiled branch August 17, 2024 12:11
@jaraco
Copy link
Member Author

jaraco commented Aug 17, 2024

Releasing as v6.4.3. Please give it a try and let me know if you encounter any issues. This will get ported to CPython as well.

jaraco added a commit to jaraco/cpython that referenced this pull request Aug 17, 2024
@Gatsik
Copy link
Contributor

Gatsik commented Aug 17, 2024

Would you like to advocate for it being a bugfix and not a feature?

In my opinion, it's a bug, because previously files worked both for non-compiled and compiled code, and update, which allows to omit anchor, broke behaviour for compiled code

What use cases would be affected if this behavior were only available here and in Python 3.14+?

Applications, which use libraries, which use bare files with importlib will stop working correctly. Currently I have no idea how I would workaround this issue with irc==20.5.0 rather than downgrading it to the version that doesn't use bare files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

importlib.resources.files() doesn't work correctly when importlib library is compiled
2 participants