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

wasmtime.bindgen shouldn't be using __file__ #220

Closed
whitequark opened this issue Apr 3, 2024 · 3 comments · Fixed by #224
Closed

wasmtime.bindgen shouldn't be using __file__ #220

whitequark opened this issue Apr 3, 2024 · 3 comments · Fixed by #224

Comments

@whitequark
Copy link
Contributor

whitequark commented Apr 3, 2024

There's actually no guarantee at all that Python source files exist anywhere on the filesystem, and even if they do they might as well be inside a zip archive or even a Rust executable. Because of this using __file__ for anything but debugging or logging, and in particular loading resources, is a fragile practice and packages like wasmtime-py should avoid it.

In practical terms the header of the generated code that looks like this:

from .intrinsics import _decode_utf8, _encode_utf8, _load
import ctypes
import pathlib
import wasmtime

class Root:

    def __init__(self, store: wasmtime.Store) -> None:
        path = pathlib.Path(__file__).parent / ('root.core0.wasm')
        module = wasmtime.Module.from_file(store.engine, path)

should be replaced with:

from .intrinsics import _decode_utf8, _encode_utf8, _load
import ctypes
import pathlib
import wasmtime
try:
    import importlib.resources as importlib_resources
    importlib_resources.files() # check if implicit anchor is supported
except TypeError:
    import importlib_resources

class Root:

    def __init__(self, store: wasmtime.Store) -> None:
        file = importlib_resources.files() / ('root.core0.wasm')
        module = wasmtime.Module.from_file(store.engine, file.read_bytes())

Note that the importlib.resources.files() (the zero argument form) is very new, freshly added in 3.12, and unfortunately consumers of the bindings will have to add the importlib-resources>=5.10 polyfill as a dependency to support all versions of Python.

(Potentially you could have another fallback that uses __file__ if neither the zero argument form is supported nor the polyfill is installed, but I find a three-deep chain of compatibility shims pretty hard to maintain.)

@alexcrichton
Copy link
Member

GIven that the component support is relatively "new" in a sense this seems reasonable to me to add! It'd probably be ok to document this dependency for older Python versions to play better with the ecosystem.

@whitequark
Copy link
Contributor Author

It'd probably be ok to document this dependency for older Python versions to play better with the ecosystem.

I think one really easy way to handle this is to add this dependency to wasmtime itself, since by definition if you have wasmtime.bindgen you also have that. This is the approach I took in #224.

@alexcrichton
Copy link
Member

Excellent point!

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 a pull request may close this issue.

2 participants